fix: use decoded token recipient for address poisoning known recipients - #9699
Merged
Conversation
For ERC-20/ERC-721/ERC-1155 token transfers, txParams.to is the token contract rather than the address receiving the tokens. PhishingController was adding the token contract to the known-recipients set used by checkAddressPoisoning, missing the actual recipient encoded in the calldata. Extract the existing getEffectiveRecipient helper from the first-time-interaction util into a shared exported util in @metamask/transaction-controller, and use it in PhishingController so token transfers contribute the decoded recipient instead of the token contract address.
Contributor
Author
|
@metamaskbot preview-builds |
Contributor
Author
|
@metamaskbot publish-previews |
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
jpuri
approved these changes
Jul 30, 2026
imblue-dabadee
approved these changes
Jul 30, 2026
|
Inucustoms@gmail.com is the correct email we're is my money goinig |
pull Bot
pushed a commit
to Reality2byte/core
that referenced
this pull request
Jul 31, 2026
## Explanation Phishing controller ## [17.3.1] ### Changed - Bump `@metamask/transaction-controller` from `^69.0.0` to `^69.4.0` ([MetaMask#9568](MetaMask#9568), [MetaMask#9589](MetaMask#9589), [MetaMask#9593](MetaMask#9593), [MetaMask#9693](MetaMask#9693), [MetaMask#9735](MetaMask#9735)) ### Fixed - Address poisoning known recipients now use the actual token recipient decoded from calldata for confirmed ERC-20/ERC-721/ERC-1155 token transfers, instead of the token contract address from `txParams.to` ([MetaMask#9699](MetaMask#9699)) ### Notes for reviewers **Why a patch.** Nothing here extends the package's surface area, so per [reviewing-release-prs.md](https://github.com/MetaMask/core/blob/main/docs/processes/reviewing-release-prs.md#4-review-new-version-strings) this lands on patch. The only code change since `17.3.0` is one line in `PhishingController` (`transaction.txParams.to` → `getEffectiveRecipient(transaction)`); the rest of the diff to this package is `.js` import-extension churn from MetaMask#9620 and the Jest 30 migration. No new exports, types, or arguments. The `@metamask/transaction-controller` range bump doesn't push this to a minor either — the same doc lists "adding or upgrading a runtime dependency" under "Everything else" rather than as an addition. Note that `checkAddressPoisoning` output does shift in both directions: lookalikes of the real ERC-20/721/1155 recipient now match, and lookalikes of the token contract address no longer do. That's captured in the `Fixed` entry above. **`@metamask/transaction-controller` needs no release here.** The `getEffectiveRecipient` export this fix depends on already shipped in `69.4.0`, published as part of release 1163.0.0. **Intentionally skipped.** `@metamask/address-book-controller`, `@metamask/base-controller`, `@metamask/controller-utils`, and `@metamask/messenger` all have unreleased changes and are dependencies of `@metamask/phishing-controller`, so the release tool flagged them. Their pending entries are pre-existing chore/refactor/deprecation work unrelated to this fix (and still uncategorized), so releasing them is a separate call for their codeowners. **Dependent bumps.** `@metamask/assets-controller` and `@metamask/assets-controllers` had their `@metamask/phishing-controller` range bumped to `^17.3.1` by the release tooling; both are recorded in their changelogs under `Unreleased`. ## References - Releases the fix from MetaMask#9699 - Unblocks pulling the fix into the mobile and extension clients ## 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/contributing.md#updating-changelogs), highlighting breaking changes as necessary - [x] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > The released phishing-controller fix alters security-sensitive address-poisoning heuristics for token transfers; this PR is dependency/version churn but ships that behavior to extension/mobile consumers. > > **Overview** > **Monorepo release 1167.0.0** publishes **`@metamask/phishing-controller@17.3.1`** and wires consumers to that version. > > The new phishing-controller patch (documented in its changelog, not in this diff’s source) fixes **address-poisoning “known recipient” tracking** for confirmed ERC-20/721/1155 transfers: recipients are taken from **decoded calldata via `getEffectiveRecipient`** instead of **`txParams.to`** (the token contract). That can change which lookalike warnings fire—real transfer recipients match; token contract addresses no longer do. > > **`@metamask/assets-controller`** and **`@metamask/assets-controllers`** bump their **`@metamask/phishing-controller`** dependency from **`^17.3.0`** to **`^17.3.1`**, with matching **Unreleased** changelog entries. **`yarn.lock`** and root **`package.json`** version are updated accordingly. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 1f778f9. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
This was referenced Jul 31, 2026
adonesky1
added a commit
to MetaMask/metamask-extension
that referenced
this pull request
Jul 31, 2026
Picks up the address-poisoning fix from MetaMask/core#9699, so known recipients use the token recipient decoded from calldata rather than the token contract address for confirmed ERC-20/721/1155 transfers. Deduped so the 69.x `@metamask/transaction-controller` range resolves to a single 69.4.0 entry rather than adding a parallel copy alongside 69.3.0.
adonesky1
added a commit
to MetaMask/metamask-mobile
that referenced
this pull request
Jul 31, 2026
Picks up the address-poisoning fix from MetaMask/core#9699, so known recipients use the token recipient decoded from calldata rather than the token contract address for confirmed ERC-20/721/1155 transfers. `@metamask/transaction-controller` moves 69.3.0 -> 69.4.0 because phishing-controller 17.3.1 requires ^69.4.0 for the `getEffectiveRecipient` export. The exact `resolutions` pin has to move too, otherwise it collapses phishing-controller back onto 69.3.0, where that export does not exist.
pull Bot
pushed a commit
to firas9941/metamask-extension
that referenced
this pull request
Aug 3, 2026
## **Description** Bumps `@metamask/phishing-controller` from `^17.3.0` to `^17.3.1` to pick up the address-poisoning fix from [MetaMask/core#9699](MetaMask/core#9699). Before that fix, the known-recipients list used to detect address poisoning took `txParams.to` from confirmed transactions. For ERC-20/721/1155 transfers `txParams.to` is the *token contract*, not the address receiving the tokens, so two things were wrong: - a lookalike of the real token recipient did **not** trigger address-poisoning detection - a lookalike of the token contract address **did** trigger it, which is meaningless noise `17.3.1` decodes the actual recipient from calldata via the new `getEffectiveRecipient` utility, so token transfers now contribute the real recipient. Plain sends and other contract interactions are unchanged. ### Why `@metamask/transaction-controller` moves in the lockfile `phishing-controller@17.3.1` declares `@metamask/transaction-controller: ^69.4.0` (that's where `getEffectiveRecipient` is exported from), so the 69.x range resolves to `69.4.0` instead of `69.3.0`. I ran a scoped `yarn dedupe` so this stays a **single** 69.x entry rather than installing `69.4.0` alongside `69.3.0`. The same dedupe collapses the `core-backend`, `gas-fee-controller`, and `polling-controller` ranges that `69.4.0` nudges forward. Two notes for reviewers on the dependency graph: - **`network-controller` is unaffected.** `transaction-controller@69.4.0` declares `network-controller: ^35.0.0`, but this repo's `resolutions` pin holds it at `34.0.0` and the lockfile still has exactly one `34.0.0` entry. That pin is safe here: the published `69.3.0` and `69.4.0` bundles are byte-identical apart from the `getEffectiveRecipient` export, an optional `strategy?: string` field on `MetamaskPayMetadata`, and the moved helper file. No compiled module in `69.4.0` references `network-controller`, `NetworkClient`, `BuiltInNetworkClientId`, or `analyticsOptions`, so its `^35.0.0` range is just the mechanical release-wide bump from MetaMask/core#9735 rather than an adoption of v35 APIs. - **`remote-feature-flag-controller@5.0.0` is added next to the existing `4.2.2`.** These can't be deduped, since `^4.2.x` cannot accept `5.0.0`. `transaction-controller@69.4.0` only references it from type declarations (`dist/TransactionController.d.cts`) and never from runtime JS, so nothing new gets pulled into the bundle. Happy to add a `resolutions` pin instead if the team would rather avoid the duplicate entry. ## **Changelog** CHANGELOG entry: Fixed address-poisoning detection so lookalikes of the actual recipient of a token transfer are flagged, and lookalikes of the token contract address are no longer flagged. ## **Related issues** Fixes: - Consumes [MetaMask/core#9699](MetaMask/core#9699) - Released in [MetaMask/core#9746](MetaMask/core#9746) (`@metamask/phishing-controller@17.3.1`) ## **Manual testing steps** 1. Pull this branch and run `yarn install`, then start a development build. 2. Add a test ERC-20 token, then send some of it to a recipient address you control. Let the transaction confirm. 3. Start a new send of that same token, and enter a recipient that is a lookalike of the address from step 2 — same first four and last four hex characters, different in the middle. 4. Confirm the address-poisoning warning now appears. On `main` it does not. 5. Repeat step 3, but use a lookalike of the *token contract* address instead. Confirm no address-poisoning warning appears, since the contract is no longer treated as a known recipient. 6. Send and confirm a plain ETH transfer and a generic contract interaction, and confirm a lookalike of each recipient still triggers the warning — those paths should be unchanged. ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** A lookalike of the real ERC-20 recipient produced no warning, while a lookalike of the token contract produced one. ### **After** A lookalike of the real ERC-20 recipient produces a warning; a lookalike of the token contract does not. ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I've included tests if applicable - [x] I've documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I've applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.
pull Bot
pushed a commit
to Reality2byte/metamask-mobile
that referenced
this pull request
Aug 3, 2026
## **Description** Bumps `@metamask/phishing-controller` from `^17.3.0` to `^17.3.1` to pick up the address-poisoning fix from [MetaMask/core#9699](MetaMask/core#9699). Before that fix, the known-recipients list used to detect address poisoning took `txParams.to` from confirmed transactions. For ERC-20/721/1155 transfers `txParams.to` is the *token contract*, not the address receiving the tokens, so two things were wrong: - a lookalike of the real token recipient did **not** trigger address-poisoning detection - a lookalike of the token contract address **did** trigger it, which is meaningless noise `17.3.1` decodes the actual recipient from calldata via the new `getEffectiveRecipient` utility, so token transfers now contribute the real recipient. Plain sends and other contract interactions are unchanged. ### Why `@metamask/transaction-controller` moves to 69.4.0 `phishing-controller@17.3.1` declares `@metamask/transaction-controller: ^69.4.0`, because `getEffectiveRecipient` is exported from `69.4.0`. This repo pins `@metamask/transaction-controller` to an exact version in `resolutions`, which collapses every range in the tree onto that one version. **That pin has to move as well** — leaving it at `69.3.0` would silently resolve `phishing-controller@17.3.1` against `69.3.0`, where `getEffectiveRecipient` exists only as an internal helper inside `first-time-interaction` and is *not* exported from the package entry point. The import would be `undefined` and would throw on every confirmed transaction. So this PR moves the `resolutions` pin and the declared range together, and the lockfile still has exactly one `transaction-controller` entry. Two notes for reviewers on the dependency graph: - **`network-controller` is unaffected.** `transaction-controller@69.4.0` declares `network-controller: ^35.0.0`, but this repo's `resolutions` pin holds it at `34.0.0` and the lockfile still has exactly one `34.0.0` entry. That pin is safe here: the published `69.3.0` and `69.4.0` bundles are byte-identical apart from the `getEffectiveRecipient` export, an optional `strategy?: string` field on `MetamaskPayMetadata`, and the moved helper file. No compiled module in `69.4.0` references `network-controller`, `NetworkClient`, `BuiltInNetworkClientId`, or `analyticsOptions`, so its `^35.0.0` range is just the mechanical release-wide bump from MetaMask/core#9735 rather than an adoption of v35 APIs. `accounts-controller` is likewise held at `39.0.3` by its existing pin. - **`remote-feature-flag-controller@5.0.0` is added next to the existing `4.2.2`.** These can't be deduped, since `^4.2.x` cannot accept `5.0.0`. `transaction-controller@69.4.0` only references it from type declarations (`dist/TransactionController.d.cts`) and never from runtime JS, so nothing new gets pulled into the bundle. Happy to add a `resolutions` pin instead if the team would rather avoid the duplicate entry. A scoped `yarn dedupe` collapses the `core-backend`, `gas-fee-controller`, and `polling-controller` ranges that `69.4.0` nudges forward, so those don't gain parallel copies either. ## **Changelog** CHANGELOG entry: Fixed address-poisoning detection so lookalikes of the actual recipient of a token transfer are flagged, and lookalikes of the token contract address are no longer flagged. ## **Related issues** Fixes: - Consumes [MetaMask/core#9699](MetaMask/core#9699) - Released in [MetaMask/core#9746](MetaMask/core#9746) (`@metamask/phishing-controller@17.3.1`) ## **Manual testing steps** Feature: address-poisoning detection for token transfer recipients Scenario: user is warned about a lookalike of a real token recipient Given the user has previously sent an ERC-20 token to an address and that transaction is confirmed When user starts a new transfer of that token and enters a recipient that matches the previous recipient's first four and last four hex characters but differs in the middle Then the address-poisoning warning is shown Scenario: user is not warned about a lookalike of the token contract Given the user has previously sent an ERC-20 token and that transaction is confirmed When user enters a recipient that is a lookalike of the token contract address Then no address-poisoning warning is shown, because the contract is no longer treated as a known recipient Scenario: plain sends and contract interactions are unchanged Given the user has a confirmed plain ETH transfer and a confirmed generic contract interaction When user enters a lookalike of either of those recipients Then the address-poisoning warning is still shown ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** A lookalike of the real ERC-20 recipient produced no warning, while a lookalike of the token contract produced one. ### **After** A lookalike of the real ERC-20 recipient produces a warning; a lookalike of the token contract does not. ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I've included tests if applicable - [x] I've documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I've applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches security-sensitive address-poisoning logic and requires the transaction-controller pin to match; runtime impact is limited to dependency behavior, but incorrect resolution would break confirmed-tx handling. > > **Overview** > Bumps **`@metamask/phishing-controller`** from `^17.3.0` to **`^17.3.1`** so address-poisoning detection uses the real token transfer recipient (via **`getEffectiveRecipient`**) instead of **`txParams.to`**, which for ERC-20/721/1155 is the token contract. Lookalikes of the actual recipient should warn; lookalikes of the contract should not. > > Because **`17.3.1`** depends on an exported **`getEffectiveRecipient`** from **`@metamask/transaction-controller`**, this PR also moves the **`resolutions`** pin and dependency from **`69.3.0`** to **`69.4.0`** and refreshes **`yarn.lock`** (including transitive bumps such as **`remote-feature-flag-controller@5.0.0`** alongside the existing **`4.2.2`**). No application source files change—behavior comes from the upgraded packages. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 6155857. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
Bigshmow
pushed a commit
to MetaMask/metamask-extension
that referenced
this pull request
Aug 5, 2026
## **Description** Bumps `@metamask/phishing-controller` from `^17.3.0` to `^17.3.1` to pick up the address-poisoning fix from [MetaMask/core#9699](MetaMask/core#9699). Before that fix, the known-recipients list used to detect address poisoning took `txParams.to` from confirmed transactions. For ERC-20/721/1155 transfers `txParams.to` is the *token contract*, not the address receiving the tokens, so two things were wrong: - a lookalike of the real token recipient did **not** trigger address-poisoning detection - a lookalike of the token contract address **did** trigger it, which is meaningless noise `17.3.1` decodes the actual recipient from calldata via the new `getEffectiveRecipient` utility, so token transfers now contribute the real recipient. Plain sends and other contract interactions are unchanged. ### Why `@metamask/transaction-controller` moves in the lockfile `phishing-controller@17.3.1` declares `@metamask/transaction-controller: ^69.4.0` (that's where `getEffectiveRecipient` is exported from), so the 69.x range resolves to `69.4.0` instead of `69.3.0`. I ran a scoped `yarn dedupe` so this stays a **single** 69.x entry rather than installing `69.4.0` alongside `69.3.0`. The same dedupe collapses the `core-backend`, `gas-fee-controller`, and `polling-controller` ranges that `69.4.0` nudges forward. Two notes for reviewers on the dependency graph: - **`network-controller` is unaffected.** `transaction-controller@69.4.0` declares `network-controller: ^35.0.0`, but this repo's `resolutions` pin holds it at `34.0.0` and the lockfile still has exactly one `34.0.0` entry. That pin is safe here: the published `69.3.0` and `69.4.0` bundles are byte-identical apart from the `getEffectiveRecipient` export, an optional `strategy?: string` field on `MetamaskPayMetadata`, and the moved helper file. No compiled module in `69.4.0` references `network-controller`, `NetworkClient`, `BuiltInNetworkClientId`, or `analyticsOptions`, so its `^35.0.0` range is just the mechanical release-wide bump from MetaMask/core#9735 rather than an adoption of v35 APIs. - **`remote-feature-flag-controller@5.0.0` is added next to the existing `4.2.2`.** These can't be deduped, since `^4.2.x` cannot accept `5.0.0`. `transaction-controller@69.4.0` only references it from type declarations (`dist/TransactionController.d.cts`) and never from runtime JS, so nothing new gets pulled into the bundle. Happy to add a `resolutions` pin instead if the team would rather avoid the duplicate entry. ## **Changelog** CHANGELOG entry: Fixed address-poisoning detection so lookalikes of the actual recipient of a token transfer are flagged, and lookalikes of the token contract address are no longer flagged. ## **Related issues** Fixes: - Consumes [MetaMask/core#9699](MetaMask/core#9699) - Released in [MetaMask/core#9746](MetaMask/core#9746) (`@metamask/phishing-controller@17.3.1`) ## **Manual testing steps** 1. Pull this branch and run `yarn install`, then start a development build. 2. Add a test ERC-20 token, then send some of it to a recipient address you control. Let the transaction confirm. 3. Start a new send of that same token, and enter a recipient that is a lookalike of the address from step 2 — same first four and last four hex characters, different in the middle. 4. Confirm the address-poisoning warning now appears. On `main` it does not. 5. Repeat step 3, but use a lookalike of the *token contract* address instead. Confirm no address-poisoning warning appears, since the contract is no longer treated as a known recipient. 6. Send and confirm a plain ETH transfer and a generic contract interaction, and confirm a lookalike of each recipient still triggers the warning — those paths should be unchanged. ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** A lookalike of the real ERC-20 recipient produced no warning, while a lookalike of the token contract produced one. ### **After** A lookalike of the real ERC-20 recipient produces a warning; a lookalike of the token contract does not. ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I've included tests if applicable - [x] I've documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I've applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.
n3ps
pushed a commit
to MetaMask/metamask-extension
that referenced
this pull request
Aug 6, 2026
## **Description** Bumps `@metamask/phishing-controller` from `^17.3.0` to `^17.3.1` to pick up the address-poisoning fix from [MetaMask/core#9699](MetaMask/core#9699). Before that fix, the known-recipients list used to detect address poisoning took `txParams.to` from confirmed transactions. For ERC-20/721/1155 transfers `txParams.to` is the *token contract*, not the address receiving the tokens, so two things were wrong: - a lookalike of the real token recipient did **not** trigger address-poisoning detection - a lookalike of the token contract address **did** trigger it, which is meaningless noise `17.3.1` decodes the actual recipient from calldata via the new `getEffectiveRecipient` utility, so token transfers now contribute the real recipient. Plain sends and other contract interactions are unchanged. ### Why `@metamask/transaction-controller` moves in the lockfile `phishing-controller@17.3.1` declares `@metamask/transaction-controller: ^69.4.0` (that's where `getEffectiveRecipient` is exported from), so the 69.x range resolves to `69.4.0` instead of `69.3.0`. I ran a scoped `yarn dedupe` so this stays a **single** 69.x entry rather than installing `69.4.0` alongside `69.3.0`. The same dedupe collapses the `core-backend`, `gas-fee-controller`, and `polling-controller` ranges that `69.4.0` nudges forward. Two notes for reviewers on the dependency graph: - **`network-controller` is unaffected.** `transaction-controller@69.4.0` declares `network-controller: ^35.0.0`, but this repo's `resolutions` pin holds it at `34.0.0` and the lockfile still has exactly one `34.0.0` entry. That pin is safe here: the published `69.3.0` and `69.4.0` bundles are byte-identical apart from the `getEffectiveRecipient` export, an optional `strategy?: string` field on `MetamaskPayMetadata`, and the moved helper file. No compiled module in `69.4.0` references `network-controller`, `NetworkClient`, `BuiltInNetworkClientId`, or `analyticsOptions`, so its `^35.0.0` range is just the mechanical release-wide bump from MetaMask/core#9735 rather than an adoption of v35 APIs. - **`remote-feature-flag-controller@5.0.0` is added next to the existing `4.2.2`.** These can't be deduped, since `^4.2.x` cannot accept `5.0.0`. `transaction-controller@69.4.0` only references it from type declarations (`dist/TransactionController.d.cts`) and never from runtime JS, so nothing new gets pulled into the bundle. Happy to add a `resolutions` pin instead if the team would rather avoid the duplicate entry. ## **Changelog** CHANGELOG entry: Fixed address-poisoning detection so lookalikes of the actual recipient of a token transfer are flagged, and lookalikes of the token contract address are no longer flagged. ## **Related issues** Fixes: - Consumes [MetaMask/core#9699](MetaMask/core#9699) - Released in [MetaMask/core#9746](MetaMask/core#9746) (`@metamask/phishing-controller@17.3.1`) ## **Manual testing steps** 1. Pull this branch and run `yarn install`, then start a development build. 2. Add a test ERC-20 token, then send some of it to a recipient address you control. Let the transaction confirm. 3. Start a new send of that same token, and enter a recipient that is a lookalike of the address from step 2 — same first four and last four hex characters, different in the middle. 4. Confirm the address-poisoning warning now appears. On `main` it does not. 5. Repeat step 3, but use a lookalike of the *token contract* address instead. Confirm no address-poisoning warning appears, since the contract is no longer treated as a known recipient. 6. Send and confirm a plain ETH transfer and a generic contract interaction, and confirm a lookalike of each recipient still triggers the warning — those paths should be unchanged. ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** A lookalike of the real ERC-20 recipient produced no warning, while a lookalike of the token contract produced one. ### **After** A lookalike of the real ERC-20 recipient produces a warning; a lookalike of the token contract does not. ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I've included tests if applicable - [x] I've documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I've applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.
Draft
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation
The
PhishingControlleraddress poisoning check (#8171) builds its known-recipients set from confirmed transactions inTransactionControllerstate, usingtxParams.toas the recipient. For ERC-20/ERC-721/ERC-1155 token transfer methods (transfer,transferFrom,safeTransferFrom),txParams.tois the token contract, not the address receiving the tokens — the actual recipient is encoded in the calldata. As a result: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
TransactionControlleralready solves this exact problem for first-time-interaction checks with a privategetEffectiveRecipienthelper that decodes the recipient from calldata for token transfer types (falling back totxParams.tootherwise). This PR:getEffectiveRecipientfromutils/first-time-interaction.tsinto a new sharedutils/recipient.tsand exports it from@metamask/transaction-controller(behavior unchanged for the first-time-interaction flow).PhishingController.#getRecipientAddressesFromTransactionso 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.tois still added, since there is no decodable recipient to substitute.Manual testing
In
metamask-extension/package.json, add both Core preview packages toresolutions:Run
yarn installand start a development build of MetaMask Extension.Submit and confirm a standard ERC-20
transferto a test recipient address.Inspect the address-poisoning known recipients after the transaction is confirmed, or check lookalike candidates through
PhishingController.checkAddressPoisoning.Confirm that a lookalike of the recipient encoded in the transfer calldata matches the actual recipient and triggers address-poisoning detection.
Confirm that a lookalike of the ERC-20 contract address does not match the contract as a known recipient from this transfer.
Submit and confirm a generic contract interaction, then confirm its
txParams.tocontract address is still added as a known recipient.References
Checklist
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.getEffectiveRecipientis moved out of first-time-interaction into sharedutils/recipient.ts, exported from@metamask/transaction-controller, and wired intoPhishingController.#getRecipientAddressesFromTransaction. First-time-interaction behavior is unchanged; generic contract interactions still usetxParams.to. Tests cover decoding and the phishing integration case.Reviewed by Cursor Bugbot for commit 5da60b6. Bugbot is set up for automated code reviews on this repo. Configure here.