fix: only scan send payees for address poisoning - #9943
Merged
Merged
Conversation
4 of 7 tasks
Confirmed approves, swaps, and contract calls were adding token and protocol addresses to the known-recipient set, which made vanity token contracts look like poisoning matches. Only hydrate and compare against user-chosen send payees.
- Move the phishing-controller changelog entry back under [Unreleased]; the rebase onto main landed it inside the released 17.4.0 section. - Drop the unreachable `swapAndSend` branch in `getSendRecipientFromSource`. `getSendRecipients` already adds `swapAndSendRecipient` unconditionally, and without the branch a `swapAndSend` type falls through to `undefined` anyway. Removing it also lets nested transactions pass straight through, so the wrapper and the `NestedTransactionMetadata` import are gone. - Document why `txParamsOriginal` is swapped in as a whole object rather than field by field, which is where this deliberately differs from `useTransferRecipient` in the clients. - Fix lint: return type on `addRecipient`, `toStrictEqual` in the new tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
adonesky1
force-pushed
the
fix/address-poisoning-send-recipients-only
branch
from
August 31, 2026 17:27
17ae26b to
8763e16
Compare
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
commented
Aug 31, 2026
Contributor
Author
|
@metamaskbot publish-previews |
commented
Aug 31, 2026
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
adonesky1
marked this pull request as ready for review
August 31, 2026 21:28
adonesky1
temporarily deployed
to
default-branch
August 31, 2026 21:28 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Two gaps flagged by review, both false negatives that would let getSendRecipients silently miss a real payee rather than misclassify a protocol address: - speedUpTransaction sets type: retry and keeps the original txParams unchanged, storing the prior type in originalType. getSendRecipients only ever looked at type, so a confirmed speed-up of any send yielded no recipient. Resolve type through originalType for retries; leave cancel alone, since stopTransaction overwrites to/data into a self-send that has no real payee to track. - determineTransactionType only returns simpleSend when to is not a contract, so a plain native transfer with no calldata to a contract address (a Safe, a smart-contract wallet, many exchange deposit addresses) is typed contractInteraction. isNativeSendType required an exact simpleSend match, so these payees were dropped on both the known-set and candidate side. Treat contractInteraction with no calldata as a native send; a contractInteraction that does carry calldata is left alone, since that calldata could be an arbitrary payable call rather than a plain transfer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
imblue-dabadee
previously approved these changes
Sep 1, 2026
| transaction.swapAndSendRecipient, | ||
| ); | ||
|
|
||
| return Array.from( |
Contributor
There was a problem hiding this comment.
Nit: getSendRecipients dedupes so don't need this anymore and can just return the new function.
stopTransaction's #retryTransaction only overwrites txParams (into a self-send), type, and originalType when building the cancellation's TransactionMeta; it never clears nestedTransactions. A cancelled batch keeps its original nested legs even though none of them executed, so getSendRecipients' unconditional nested loop was returning those stale payees as if the user had actually sent to them. Skip the nested loop when type is cancel. A sped-up (retry) batch still processes its nested transactions, since those calls do execute on-chain unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
added 2 commits
September 1, 2026 09:41
Verify ERC-721 and ERC-1155 safe transfers contribute their decoded payees to address poisoning recipient checks.
Decode safeBatchTransferFrom payees even though those calls classify as contract interactions, so address poisoning checks cover both top-level and nested transfers.
Resolve the transaction-controller changelog conflict while preserving the unreleased getSendRecipients entry.
imblue-dabadee
previously approved these changes
Sep 1, 2026
Return before reading any recipient metadata for cancellations and their retries so stale original params, nested calls, and swap recipients cannot enter the poisoning set.
adonesky1
enabled auto-merge
September 1, 2026 15:35
left a comment
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 ac85c31. Configure here.
Infer transfer methods from original calldata when wrapping replaces transaction classification, preserving address-poisoning payees without broadening ordinary contract interactions.
imblue-dabadee
previously approved these changes
Sep 2, 2026
jpuri
approved these changes
Sep 2, 2026
mcmire
approved these changes
Sep 2, 2026
mindofmar
approved these changes
Sep 2, 2026
This was referenced Sep 2, 2026
Merged
chore: bump phishing and transaction controllers for poisoning fix
MetaMask/metamask-extension#45982
Merged
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
5 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
Restrict address poisoning known recipients to user-chosen send payees so confirmed approves, swaps, and contract calls stop putting token and protocol addresses into the comparison set.
Summary
Export
getSendRecipientsand hydrate the phishing known-recipient set from it instead ofgetEffectiveRecipient, which falls back totxParams.tofor every non-transfer type.Problem
Address poisoning is a payee mixup: the user previously sent to Alice, an attacker grinds a lookalike EOA, and the user pastes from history. We were also storing token, Permit2, router, and other contract
tovalues from confirmed approves and contract interactions. Vanity token factories (shared prefix plus suffix) then 4+4-matched each other, which is what fired on the o1.exchange Permit2 approve: on Base, dogue (0xb2000000000000000000003779298b7eB3D8d501) matched Base Uncle (0xb200000000000000000000c02ce1aA07c9E3d501) at 22 prefix characters plus the shared suffixd501.getEffectiveRecipientis the right helper for first-time interaction (the contracttois who we are calling). It is the wrong helper for poisoning.Solution
Add
getSendRecipientsnext togetEffectiveRecipientand use it only for poisoning hydration.Included:
simpleSendto, preferringtxParamsOriginalwhen presentto/_tofor the three transfer methods:transfer,transferFrom,safeTransferFrom(no fallback to the token contract)swapAndSendRecipientNot included: approves and Permit2,
swapandswapApproval, bridges, staking, and generic contract calls. NoteswapAndSendis deliberately not in that list, since its recipient is entered by the user.Also not included, and worth being explicit about: ERC-1155
safeBatchTransferFrom. There is noTransactionTypefor it, so it classifies ascontractInteractionand its payees are never scanned. That is pre-existing behaviour inherited fromgetEffectiveRecipientrather than something this PR changes, but it does mean "ERC-1155 transfers are covered" is only true of the single-transfer method.getEffectiveRecipientis unchanged so first-time interaction keeps using the contractto.Two deliberate details a reviewer might question:
txParamsOriginalis swapped in as a whole object rather than field by field, unlikeuseTransferRecipientin the clients. It is acloneDeepsnapshot, so itstoanddataalways describe the same call. Mixing an originaltowith a wrappeddatawould misread an untyped transaction as a contract call and drop a real payee.swapAndSendRecipientis added unconditionally rather than gated onTransactionType.swapAndSend. It is only ever populated from the payee the user entered in the swap-and-send flow, and gating on the type as well would be dead code.Follow-up client PR: MetaMask/metamask-extension#45724
Risk
#knownRecipientsis in-memory and rebuilt in the constructor, so this self-heals on the next controller construction with no migration.^17.4.0and mobile pins^17.3.1, so both need a bump PR after this releases. Until then the extension fix is candidate-side only.Verification
Unit coverage in this PR:
getSendRecipientsreturns[]fortokenMethodApproveandcontractInteraction, and returns the decoded payee (not the token contract) for the three transfer typesPhishingControllerdoes not add token contracts from confirmed approve transactions, and hydratesswapAndSendRecipientrather than the swap contractManual verification of the known-set change
The false positive in the report is fixed on the extension side alone, because an approve confirmation stops producing a candidate at all. That means the extension PR's manual steps cannot observe this PR's change: with no candidate, the known set is never consulted. To see the known-set cleanup you have to probe it from the send side.
Setup
Needs a preview build of this package, since the extension pins
@metamask/phishing-controller@^17.4.0.In
metamask-extension, on #45724, add a top-levelpreviewBuildsblock topackage.jsonand runyarn install:Both are needed: the preview
phishing-controllerdepends ontransaction-controller@^69.6.1, which the extension's^69.5.1pin does not satisfy, so without the second entry you get a second copy on disk. Confirm the install withyarn why @metamask-previews/transaction-controller, notyarn why @metamask/transaction-controller; once remapped, the copies live under the preview scope. Do not commit thepackage.jsonoryarn.lockchanges, and do not delete the vendoredgetSendRecipientsin that PR, which still has to build against the published^69.5.1.Then
yarn distand load the build. Everything below runs in the page console at https://metamask.github.io/test-dapp/ with the account connected.Helpers, paste once
Assigns to
windowso it survives separate pastes. Pasting a bareconst from = ...in one block and using it in the next is what producesfrom is not defined.1. Seed the known set
Confirms an
approve(Permit2, 0)on Base Uncle. On the publishedphishing-controllerthis puts the token contract into the known set; with this PR it does not. Approving0needs no token balance.Confirm it and wait for confirmed status in Activity.
#getRecipientAddressesFromTransactionreturns[]for anything not yet confirmed, so running step 2 while this is pending gives a false pass.Expect a trust-signal alert on this confirmation regardless of this PR: the address scan returns
Warningfor Base Uncle. Separate detector, unrelated.2. Probe the known set from the send side
0xb20011…d501scores prefix 4 / suffix 4 against the Base Uncle contract, exactly the detector's threshold. A dapp-initiated native send with no calldata types assimpleSend, so it does produce a candidate and the known set is consulted.Expected: no address poisoning warning. Cancel rather than confirming, so the probe address stays out of the known set.
BEFORE:
AFTER: using the same address from the original report
Video depicting using the same contract address with no address poisoning detection when compared against a similar looking contract:
https://github.com/user-attachments/assets/b1688fa2-2d2d-4868-9f70-eef3d44c89b7
To see the failing side, remove the
previewBuildsblock,yarn install, rebuild, and repeat steps 1 and 2. The published^17.4.0warns here, comparing the send address against the token contract the approve stored.3. Control: the known set still holds real payees
Confirm this one, wait for confirmed status:
Then submit the lookalike and cancel it:
Expected: the address poisoning warning appears. This must hold with the preview installed. If step 2 and step 3 both come back silent, the detector is broken rather than the fix working.
Real poisoning for entered native send poisoning still working as expected
Screen.Recording.2026-08-31.at.4.08.06.PM.mov
Step 2 is the only check that distinguishes this PR from the extension PR. Note the known set is rebuilt in the
PhishingControllerconstructor from confirmed transaction state, so there is nothing to clear after swapping builds: restarting the extension re-derives it.References
Checklist
Note
Medium Risk
Changes how address-poisoning known recipients are derived from transaction history, which can reduce false positives but also narrows the comparison set; real send-based poisoning detection should remain covered by tests and address book entries.
Overview
Adds and exports
getSendRecipientsin@metamask/transaction-controllerso callers can resolve user-chosen payees (native sends, decoded ERC-20/721/1155 transfers including batch,swapAndSendRecipient, nested batch sends) while excluding approves, swaps, and generic contracttoaddresses.getEffectiveRecipientis unchanged for “who we’re calling” flows; docs now steer poisoning checks toward the new helper.PhishingControllerbuilds its in-memory known-recipient set fromgetSendRecipientsinstead ofgetEffectiveRecipient, so confirmed token approves and similar txs no longer seed lookalike comparisons against token/router contracts. Invalid hex recipients are still dropped via existing normalization.Tests cover
getSendRecipientsedge cases (cancellations, speed-ups,txParamsOriginal, batches) and phishing hydration (approve txs, swap-and-send payee vs swap contract).Reviewed by Cursor Bugbot for commit b5a186b. Bugbot is set up for automated code reviews on this repo. Configure here.