feat(phishing-controller): export extractSignatureAddresses for EIP-712 typed-data scanning - #9875
Conversation
|
|
||
| // Reduce into the 20-byte address space, matching how the signer encodes an | ||
| // `address` field, so non-canonical encodings resolve to the signed address. | ||
| numeric %= ADDRESS_MODULUS; |
There was a problem hiding this comment.
The %= ADDRESS_MODULUS is backwards relative to the signer. encodeField returns reallyStrangeAddressToBytes(value).subarray(0, 20), and since padStart never truncates, an oversized value keeps its leading 20 bytes. Modulo keeps precisely the low bytes the signer discards.
This is exploitable: verified against TypedDataUtils.encodeData, decimal(ADDR_A * 256 + 0x42) signs as exactly ADDR_A but normalizes here to 0x1111…1142. An attacker appends one byte, the real spender never reaches the scanner, and overflow stays false.
Suggested fix: take the leading 20 bytes rather than reducing, plus a test asserting agreement with TypedDataUtils.encodeData so the two can't drift again.
|
|
||
| ### Added | ||
|
|
||
| - Add `extractSignatureAddresses` utility, plus `ExtractedSignatureAddresses` and `ExtractSignatureAddressesOptions` types, to collect the `address`-typed values from an EIP-712 typed-data message for real-time address scanning ([#9999](https://github.com/MetaMask/core/pull/9999)) |
| // The signer reduces an `address` mod 2^160, so `value + 2^160` signs as | ||
| // `value`. The extractor must resolve it to the same address. |
There was a problem hiding this comment.
The premise in this comment is inverted: the signer does not reduce an address mod 2^160.
For a string that isn't strict hex, encodeField's address branch falls through to the last path:
return ['address', reallyStrangeAddressToBytes(value).subarray(0, 20)];
| const excludedFields = new Set( | ||
| (options.excludeFields ?? []).map((field) => field.toLowerCase()), | ||
| ); |
There was a problem hiding this comment.
EIP-712 field names are case-sensitive. encodeData reads data[field.name] verbatim and this file does too at L243, so lowercasing only in the exclusion check means excludeFields: ['spender'] silently drops Spender too.
A Permit declaring both commits both addresses, and the handler this option defers to reads message?.spender exactly while gating only on primaryType. So Spender is scanned by nobody.
Suggested fix: compare exactly and document that entries must match the declared name verbatim. Over-excluding fails silently; under-excluding only costs a duplicate scan.
| } | ||
|
|
||
| // Handle one array dimension at a time, e.g. `address[]` or `Type[][]`. | ||
| const arrayMatch = type.match(/^(.*)\[\d*\]$/u); |
There was a problem hiding this comment.
visitField checks types in a different order than the signer and accepts fewer array shapes, so some address values get signed without being scanned.
encodeField resolves custom types first and arrays last via endsWith(']'); visitField inverts that and requires /^(.)[\d]$/u.
Two cases I verified via TypedDataUtils.eip712Hash. With type: 'address[abc]' the signer slices to address and commits, while the regex rejects it and no later branch matches, so we collect nothing. With a type declared as address[] the signer treats it as a struct and commits the inner address, while we array-match first and bail on the Array.isArray(value) guard. Neither sets overflow.
Suggested fix: mirror the signer's precedence (custom type, primitives, then endsWith(']') with lastIndexOf('[') slicing), and set overflow = true when an address-bearing type can't be walked, so silence stops looking like "no addresses"
…natureAddresses Address review: take leading 20 bytes (not mod 2^160), exact excludeFields, signer type-dispatch order, and changelog MetaMask#9875.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Reviewed by Cursor Bugbot for commit fdd3509. Configure here.
…util Document that 0x-hex (odd or even) is isStrictHexString; oversized hex fails to encode rather than signing as a different address.
| // Cap the number of addresses returned for a single signature. A legitimate | ||
| // signature references far fewer; exceeding this is treated as unusual and | ||
| // surfaced to the caller (via `overflow`) rather than scanned in full. | ||
| const MAX_SIGNATURE_ADDRESSES = 10; |
There was a problem hiding this comment.
Is 10 the right cap, and should callers be able to override it?
Using the Permit2 PermitBatch fixture from the extension's own test/integration/confirmations/signatures/signature-helpers.ts, overflow goes true at 11 tokens under the extension's excludeFields: ['spender'] config, and at 10 under mobile's, which passes no exclusion so spender counts against the cap. Both clients render overflow as a user-visible "scan incomplete" warning, so a batch that size gets a caution with nothing actually wrong.
Could we add a maxAddresses option (or raise the default), and export the constant so callers can word that message accurately? I don't have data on how common batches that large are, so this may be rare in practice, but it seems easier to settle before two clients ship against the current behaviour.
There was a problem hiding this comment.
Agreed, I’ll add an optional maxAddresses (default stays 10), export the constant so copy/telemetry can stay accurate, and put a hard ceiling so callers cannot set an unbounded value. Clients can keep omitting the option and get 10.
…ddresses Resolves the `packages/phishing-controller/CHANGELOG.md` conflict caused by the 17.4.0 and 17.4.1 releases landing on main. Keeps the unreleased `extractSignatureAddresses` entry under `[Unreleased]` and restores the released sections beneath it. No source changes; `signature-address-extraction.ts` is untouched.
Two failures surfaced by the first full CI run on this branch: - `lint:misc:check`: reformat two assertions in `signature-address-extraction.test.ts` per oxfmt. No semantic change. - `constraints`: align the `@metamask/eth-sig-util` devDependency with the rest of the monorepo (`^9.0.0`), which eth-json-rpc-middleware, keyring-controller, message-manager and signature-controller all use. The 41 tests in the suite cross-check directly against `TypedDataUtils.encodeData` and pass unchanged under 9.0.0.
adonesky1
left a comment
adonesky1
left a comment
There was a problem hiding this comment.
Just one more non-blocking comment. Otherwise LGTM
… cap Callers can pass maxAddresses (default 10, ceiling 50) so clients can raise the budget later without a core change.
…ler-extract-signature-addresses # Conflicts: # packages/phishing-controller/CHANGELOG.md
|
|
||
| - name: Get pull request branch | ||
| id: pr-branch | ||
| - name: Get pull request head ref |
There was a problem hiding this comment.
Are these changes necessary to merge this PR? They seem unrelated.
There was a problem hiding this comment.
These are not necessary. Dropping these now.
| with: | ||
| base-branch: ${{ github.event.pull_request.base.ref }} | ||
| head-ref: ${{ github.head_ref }} | ||
| head-ref: refs/pull/${{ github.event.pull_request.number }}/head |
There was a problem hiding this comment.
Dropping this as well.
Those .github changes are unrelated to extractSignatureAddresses and are not required to merge this PR.
…ler-extract-signature-addresses
The earlier automated conflict resolution also dropped a blank line in a released section. Restore that section verbatim from main so the only changelog delta on this branch is the Unreleased entry.
…12 typed-data scanning (MetaMask#10170) ## Explanation Ticket: [PSAFE-441](https://consensyssoftware.atlassian.net/browse/PSAFE-441) Supersedes [MetaMask#9875](MetaMask#9875) (same change, opened from a core branch so changelog CI can resolve the head). Review comments on that PR were addressed there. MetaMask validates `eth_signTypedData_v3` and `eth_signTypedData_v4` requests with PPOM, which inspects the address fields of a signature. PPOM's threat data is refreshed on a delay, so an address that has been flagged recently can still be reported as benign, and it does not cover every chain or protocol. Separately, the real-time per-address scan (`/address/evm/scan`) is only applied to a few fields today: the token `verifyingContract`, permit `spender`, and delegation `delegate`. Addresses carried in any other field — recipients, makers, tokens, custom protocol fields — receive no real-time scan at all. This change adds a schema-driven extractor that returns up to 10 distinct `address`-typed values found in a typed-data message (including nested structs and arrays, matched by declared type rather than field name), and reports when the message could not be fully walked because the address cap, depth limit, or work budget was reached. When PPOM has not already flagged the request, the extracted addresses are passed to the real-time address scan. Results reuse the existing address cache, so fields already scanned elsewhere are not requested again. ### What this exports - `extractSignatureAddresses(typedData, options)` — walks `types`/`primaryType`/`message`, returns `{ addresses, fields, overflow, maxAddresses }` - `ExtractedSignatureAddresses` — return type - `ExtractSignatureAddressesOptions` — options: `exclude` (addresses to skip, e.g. the signer), `excludeFields` (top-level field names to skip, e.g. `spender` when already covered by an existing scan), `maxAddresses` (optional cap override; default 10, hard ceiling 50) - `DEFAULT_MAX_SIGNATURE_ADDRESSES` and `MAX_SIGNATURE_ADDRESSES_CEILING` — exported so clients can word overflow copy and stay aligned with the default/ceiling ### Safeguards - Cap of 10 distinct addresses per message by default, overridable via `maxAddresses` up to 50 - Max traversal depth of 12 - Max 5000 nodes visited - `overflow: true` is returned whenever the walk is incomplete so callers can surface a caution ## References - Prior fork PR (same change): MetaMask#9875 - MetaMask Extension (client wiring): MetaMask/metamask-extension#45521 - MetaMask Mobile (client wiring): MetaMask/metamask-mobile#34786 - Prior implementation with local extractor copy (Extension): MetaMask/metamask-extension#45058 - Prior implementation with local extractor copy (Mobile): MetaMask/metamask-mobile#34428 ## Changelog See `packages/phishing-controller/CHANGELOG.md`. ## Checklist - [x] Tests written and passing - [x] Changelog updated - [x] Exports added to `index.ts` - [ ] Version bump (pending release cut) [PSAFE-441]: https://consensyssoftware.atlassian.net/browse/PSAFE-441?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Security-adjacent signing flow support: incorrect normalization or missed addresses could weaken typed-data scanning when clients adopt the export, though behavior is heavily tested and bounded with overflow signaling. > > **Overview** > Adds **`extractSignatureAddresses`**, a schema-driven helper that walks EIP-712 `types`/`primaryType`/`message` and collects distinct **`address`-typed** values (including nested structs and arrays) for downstream real-time address scanning—not hard-coded field names like `spender` or `to`. > > The function **normalizes** values the same way signing does (hex/decimal, leading 20 bytes, lowercase dedupe), supports **`exclude`**, top-level **`excludeFields`**, and a configurable distinct-address cap (**default 10**, ceiling **50**). It returns **`addresses`**, per-address **`fields`** for alert copy, and **`overflow`** when traversal hits caps, depth (12), or a 5000-node budget so clients can warn when not every address was collected. **`DEFAULT_MAX_SIGNATURE_ADDRESSES`** and **`MAX_SIGNATURE_ADDRESSES_CEILING`** are exported from **`index.ts`** alongside the new types. > > Ships **`signature-address-extraction.ts`**, a large Jest suite (including **`@metamask/eth-sig-util`** parity checks), changelog entry, and **`eth-sig-util`** as a **devDependency** only—no **`PhishingController`** wiring in this PR. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 732e1c3. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Alex Donesky <adonesky@gmail.com>
3 participants
Explanation
Ticket: PSAFE-441
MetaMask validates `eth_signTypedData_v3` and `eth_signTypedData_v4` requests with PPOM, which inspects the address fields of a signature. PPOM's threat data is refreshed on a delay, so an address that has been flagged recently can still be reported as benign, and it does not cover every chain or protocol.
Separately, the real-time per-address scan (`/address/evm/scan`) is only applied to a few fields today: the token `verifyingContract`, permit `spender`, and delegation `delegate`. Addresses carried in any other field — recipients, makers, tokens, custom protocol fields — receive no real-time scan at all.
This change adds a schema-driven extractor that returns up to 10 distinct `address`-typed values found in a typed-data message (including nested structs and arrays, matched by declared type rather than field name), and reports when the message could not be fully walked because the address cap, depth limit, or work budget was reached. When PPOM has not already flagged the request, the extracted addresses are passed to the real-time address scan. Results reuse the existing address cache, so fields already scanned elsewhere are not requested again.
What this exports
Safeguards
References
Changelog
See `packages/phishing-controller/CHANGELOG.md`.
Checklist
Note
Medium Risk
New security-adjacent parsing logic must stay aligned with EIP-712 signing and cap/overflow behavior; incomplete walks could miss addresses if clients ignore
overflow.Overview
Adds a shared
extractSignatureAddresseshelper to@metamask/phishing-controllerso clients can feed real-time address scans frometh_signTypedDatapayloads without hard-coding permit/spender-style field names.The walker follows EIP-712
types/primaryType/message, collecting values for fields declared asaddress(including nested structs and arrays), normalizing encodings to match signer behavior, and returningaddresses, per-addressfields, andoverflowwhen caps or traversal limits stop a full walk. Callers canexcludeaddresses (e.g. signer), skip top-levelexcludeFields, and tunemaxAddresses(default 10, ceiling 50).DEFAULT_MAX_SIGNATURE_ADDRESSESandMAX_SIGNATURE_ADDRESSES_CEILINGare exported alongside the types.Also documents the feature in the changelog, adds
@metamask/eth-sig-utilas a devDependency for normalization tests, and bumps@metamask/transaction-controllerto^69.8.1.Reviewed by Cursor Bugbot for commit 541dff0. Bugbot is set up for automated code reviews on this repo. Configure here.