feat: add address poisoning detection to PhishingController - #8171
Conversation
| try { | ||
| if (patches.some((patch) => patch.path[0] === 'transactions')) { | ||
| try { | ||
| this.#setKnownRecipientsFromTransactionState(_state); |
There was a problem hiding this comment.
Is this rebuilding the known recipients by iterating through every confirmed transaction every time there's a patch? If yes, surely there's a more reasonable alternative -- I can see this being expensive for power users.
There was a problem hiding this comment.
Good callout. I changed this to update transaction recipients incrementally from the relevant transaction patch instead of rebuilding from every confirmed transaction on each update. It also tracks recipients per transaction id with counts, so removals and duplicate recipients are handled correctly.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4df8176. Configure here.
…ts (MetaMask#9699) ## Explanation The `PhishingController` address poisoning check ([MetaMask#8171](MetaMask#8171)) builds its known-recipients set from confirmed transactions in `TransactionController` state, using `txParams.to` as the recipient. For ERC-20/ERC-721/ERC-1155 token transfer methods (`transfer`, `transferFrom`, `safeTransferFrom`), `txParams.to` is the **token contract**, not the address receiving the tokens — the actual recipient is encoded in the calldata. As a result: * A lookalike of the actual token recipient did **not** trigger a poisoning warning (the address the user actually pays is missing from the set). * A lookalike of the token contract **did** trigger a warning (the contract is not a recipient the user pays). This was validated live against a dev build of the extension: after a confirmed ERC-20 `transfer`, the token contract address appeared in the known-recipients set while the decoded calldata recipient did not. ### Solution `TransactionController` already solves this exact problem for first-time-interaction checks with a private `getEffectiveRecipient` helper that decodes the recipient from calldata for token transfer types (falling back to `txParams.to` otherwise). This PR: * Extracts `getEffectiveRecipient` from `utils/first-time-interaction.ts` into a new shared `utils/recipient.ts` and exports it from `@metamask/transaction-controller` (behavior unchanged for the first-time-interaction flow). * Uses it in `PhishingController.#getRecipientAddressesFromTransaction` so confirmed token transfers contribute the **decoded recipient** to the known-recipients set, replacing the token contract address. Generic contract interactions are intentionally unchanged: their `txParams.to` is still added, since there is no decodable recipient to substitute. ## Manual testing 1. In `metamask-extension/package.json`, add both Core preview packages to `resolutions`: ```json "@metamask/phishing-controller": "npm:@metamask-previews/phishing-controller@17.3.0-preview-5da60b6", "@metamask/transaction-controller": "npm:@metamask-previews/transaction-controller@69.3.0-preview-5da60b6" ``` 2. Run `yarn install` and start a development build of MetaMask Extension. 3. Submit and confirm a standard ERC-20 `transfer` to a test recipient address. 4. Inspect the address-poisoning known recipients after the transaction is confirmed, or check lookalike candidates through `PhishingController.checkAddressPoisoning`. 5. Confirm that a lookalike of the recipient encoded in the transfer calldata matches the actual recipient and triggers address-poisoning detection. 6. Confirm that a lookalike of the ERC-20 contract address does not match the contract as a known recipient from this transfer. 7. Submit and confirm a generic contract interaction, then confirm its `txParams.to` contract address is still added as a known recipient. ## References * Related to [MetaMask#8171](MetaMask#8171) (original address poisoning detection) * Related to PSAFE-555 ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/processes/updating-changelogs.md) - [ ] I've introduced [breaking changes](https://github.com/MetaMask/core/tree/main/docs/processes/breaking-changes.md) in this PR and have prepared draft pull requests for clients and consumer packages to resolve them <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes security-sensitive address-poisoning recipient hydration; logic is covered by tests and reuses existing decoding with fallbacks to `txParams.to`. > > **Overview** > **Address poisoning** now builds known recipients from the real payee on confirmed ERC-20/ERC-721/ERC-1155 transfers, not the token contract in `txParams.to`. > > `getEffectiveRecipient` is moved out of first-time-interaction into shared `utils/recipient.ts`, exported from `@metamask/transaction-controller`, and wired into `PhishingController.#getRecipientAddressesFromTransaction`. First-time-interaction behavior is unchanged; generic contract interactions still use `txParams.to`. Tests cover decoding and the phishing integration case. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 5da60b6. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->

Explanation
This PR adds address poisoning detection to
@metamask/phishing-controllerby:findSimilarAddressesutility that compares a candidate address against known recipients using prefix/suffix matching heuristicsPhishingController:checkAddressPoisoningmessenger action that clients can call to check if a recipient address looks like a poisoning attemptTransactionControllerstateAddressBookControllerstateReferences
Checklist
Note
Medium Risk
Adds new recipient-hydration logic driven by
TransactionController/AddressBookControllerstate changes plus a new messenger-exposed method, so bugs could impact performance or client warning behavior though no funds-moving logic is changed.Overview
Adds address-poisoning detection to
@metamask/phishing-controllerby introducingfindSimilarAddressesand a new messenger action/methodPhishingController:checkAddressPoisoningthat returns similarity metadata (prefix/suffix match lengths, score, diff indices).PhishingControllernow depends on@metamask/address-book-controller, hydrates a set of known recipient addresses from confirmed transactions and address-book entries, and keeps it updated by subscribing to both controllers’stateChangeevents (with patch-aware transaction updates and rebuild fallbacks).Includes new unit tests for the address similarity utility and extensive controller tests covering hydration, incremental updates, and failure-path logging; also tightens API error-type checks in a few existing scan methods.
Reviewed by Cursor Bugbot for commit 306647c. Bugbot is set up for automated code reviews on this repo. Configure here.