From fcf658098bec081ab410032a18a9c1cf6d40b2f3 Mon Sep 17 00:00:00 2001 From: Alex Donesky Date: Mon, 24 Aug 2026 12:08:15 -0500 Subject: [PATCH 01/10] fix: restrict address poisoning recipients to send payees 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. --- packages/phishing-controller/CHANGELOG.md | 4 + .../src/PhishingController.test.ts | 125 ++++++++++++++++ .../src/PhishingController.ts | 15 +- packages/transaction-controller/CHANGELOG.md | 4 + packages/transaction-controller/src/index.ts | 2 +- .../src/utils/recipient.test.ts | 137 +++++++++++++++++- .../src/utils/recipient.ts | 113 ++++++++++++++- 7 files changed, 386 insertions(+), 14 deletions(-) diff --git a/packages/phishing-controller/CHANGELOG.md b/packages/phishing-controller/CHANGELOG.md index 0967d9afc04..825613898a4 100644 --- a/packages/phishing-controller/CHANGELOG.md +++ b/packages/phishing-controller/CHANGELOG.md @@ -22,6 +22,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Bump `@metamask/transaction-controller` from `^69.4.0` to `^69.5.2` ([#9780](https://github.com/MetaMask/core/pull/9780), [#9798](https://github.com/MetaMask/core/pull/9798), [#9823](https://github.com/MetaMask/core/pull/9823)) +### Fixed + +- Restrict address poisoning known recipients to user-chosen send payees (`simpleSend`, decoded token transfer recipients, `swapAndSendRecipient`, and nested batch sends) so confirmed approves, swaps, and contract interactions no longer add token or protocol addresses to the comparison set + ## [17.3.1] ### Changed diff --git a/packages/phishing-controller/src/PhishingController.test.ts b/packages/phishing-controller/src/PhishingController.test.ts index d7437aa8a7c..de8c4d67952 100644 --- a/packages/phishing-controller/src/PhishingController.test.ts +++ b/packages/phishing-controller/src/PhishingController.test.ts @@ -4583,6 +4583,7 @@ describe('Address poisoning detection', () => { it('hydrates known recipients from confirmed transactions and address book state', () => { const confirmedTransaction = createMockTransaction('confirmed-tx', [], { status: TransactionStatus.confirmed, + type: TransactionType.simpleSend, txParams: { from: TEST_ADDRESSES.FROM_ADDRESS, to: CONFIRMED_TX_RECIPIENT, @@ -4688,6 +4689,118 @@ describe('Address poisoning detection', () => { ).toStrictEqual([]); }); + it('does not add token contracts from confirmed approve transactions', () => { + const TOKEN_CONTRACT = + '0xdddd111111111111111111111111111111119999' as `0x${string}`; + const CONTRACT_CANDIDATE_ADDRESS = + '0xddddaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa9999' as `0x${string}`; + + const approveTransaction = createMockTransaction('approve-tx', [], { + status: TransactionStatus.confirmed, + type: TransactionType.tokenMethodApprove, + txParams: { + from: TEST_ADDRESSES.FROM_ADDRESS, + to: TOKEN_CONTRACT, + value: '0x0' as `0x${string}`, + data: '0x095ea7b3000000000000000000000000cccccccccccccccccccccccccccccccccccccccc0000000000000000000000000000000000000000000000000000000000000001' as `0x${string}`, + }, + }); + + const { messenger } = setupMessenger({ + transactionControllerState: { + ...getDefaultTransactionControllerState(), + transactions: [approveTransaction], + }, + }); + + const controller = new PhishingController({ + messenger, + }); + + expect( + controller.checkAddressPoisoning(CONTRACT_CANDIDATE_ADDRESS), + ).toStrictEqual([]); + }); + + it('hydrates swap-and-send payees rather than the swap contract', () => { + const SWAP_CONTRACT = + '0xdddd111111111111111111111111111111119999' as `0x${string}`; + const CONTRACT_CANDIDATE_ADDRESS = + '0xddddaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa9999' as `0x${string}`; + + const swapAndSendTransaction = createMockTransaction( + 'swap-and-send-tx', + [], + { + status: TransactionStatus.confirmed, + type: TransactionType.swapAndSend, + swapAndSendRecipient: CONFIRMED_TX_RECIPIENT, + txParams: { + from: TEST_ADDRESSES.FROM_ADDRESS, + to: SWAP_CONTRACT, + value: '0x0' as `0x${string}`, + }, + }, + ); + + const { messenger } = setupMessenger({ + transactionControllerState: { + ...getDefaultTransactionControllerState(), + transactions: [swapAndSendTransaction], + }, + }); + + const controller = new PhishingController({ + messenger, + }); + + expect( + controller.checkAddressPoisoning(TX_CANDIDATE_ADDRESS), + ).toMatchObject([ + { + knownAddress: CONFIRMED_TX_RECIPIENT, + prefixMatchLength: 4, + suffixMatchLength: 32, + poisoningScore: 36, + }, + ]); + + expect( + controller.checkAddressPoisoning(CONTRACT_CANDIDATE_ADDRESS), + ).toStrictEqual([]); + }); + + it('ignores confirmed send recipients that are not valid hex addresses', () => { + const confirmedTransaction = createMockTransaction( + 'invalid-recipient-tx', + [], + { + status: TransactionStatus.confirmed, + type: TransactionType.simpleSend, + txParams: { + from: TEST_ADDRESSES.FROM_ADDRESS, + to: '0x1', + value: '0x0' as `0x${string}`, + }, + }, + ); + + const { messenger } = setupMessenger({ + transactionControllerState: { + ...getDefaultTransactionControllerState(), + transactions: [confirmedTransaction], + }, + }); + + const controller = new PhishingController({ + messenger, + }); + + expect(controller.checkAddressPoisoning(CANDIDATE_ADDRESS)).toStrictEqual( + [], + ); + }); + it('ignores non-confirmed transactions when hydrating known recipients', () => { const { messenger } = setupMessenger({ transactionControllerState: { @@ -4760,6 +4873,7 @@ describe('Address poisoning detection', () => { const confirmedTransaction = createMockTransaction('confirmed-tx', [], { status: TransactionStatus.confirmed, + type: TransactionType.simpleSend, txParams: { from: TEST_ADDRESSES.FROM_ADDRESS, to: ADDRESS_BOOK_RECIPIENT, @@ -4794,6 +4908,7 @@ describe('Address poisoning detection', () => { it('updates transaction recipients when a confirmed transaction recipient changes', async () => { const originalTransaction = createMockTransaction('confirmed-tx', [], { status: TransactionStatus.confirmed, + type: TransactionType.simpleSend, txParams: { from: TEST_ADDRESSES.FROM_ADDRESS, to: ADDRESS_BOOK_RECIPIENT, @@ -4802,6 +4917,7 @@ describe('Address poisoning detection', () => { }); const updatedTransaction = createMockTransaction('confirmed-tx', [], { status: TransactionStatus.confirmed, + type: TransactionType.simpleSend, txParams: { from: TEST_ADDRESSES.FROM_ADDRESS, to: CONFIRMED_TX_RECIPIENT, @@ -4853,6 +4969,7 @@ describe('Address poisoning detection', () => { it('keeps duplicate transaction recipients when one matching transaction recipient changes', async () => { const firstTransaction = createMockTransaction('confirmed-tx-1', [], { status: TransactionStatus.confirmed, + type: TransactionType.simpleSend, txParams: { from: TEST_ADDRESSES.FROM_ADDRESS, to: ADDRESS_BOOK_RECIPIENT, @@ -4861,6 +4978,7 @@ describe('Address poisoning detection', () => { }); const secondTransaction = createMockTransaction('confirmed-tx-2', [], { status: TransactionStatus.confirmed, + type: TransactionType.simpleSend, txParams: { from: TEST_ADDRESSES.FROM_ADDRESS, to: ADDRESS_BOOK_RECIPIENT, @@ -4872,6 +4990,7 @@ describe('Address poisoning detection', () => { [], { status: TransactionStatus.confirmed, + type: TransactionType.simpleSend, txParams: { from: TEST_ADDRESSES.FROM_ADDRESS, to: CONFIRMED_TX_RECIPIENT, @@ -4964,6 +5083,7 @@ describe('Address poisoning detection', () => { const confirmedTransaction = createMockTransaction('confirmed-tx', [], { status: TransactionStatus.confirmed, + type: TransactionType.simpleSend, txParams: { from: TEST_ADDRESSES.FROM_ADDRESS, to: ADDRESS_BOOK_RECIPIENT, @@ -4997,6 +5117,7 @@ describe('Address poisoning detection', () => { const confirmedTransaction = createMockTransaction('confirmed-tx', [], { status: TransactionStatus.confirmed, + type: TransactionType.simpleSend, txParams: { from: TEST_ADDRESSES.FROM_ADDRESS, to: ADDRESS_BOOK_RECIPIENT, @@ -5024,6 +5145,7 @@ describe('Address poisoning detection', () => { it('rebuilds known recipients when a remove patch does not include the removed transaction', async () => { const confirmedTransaction = createMockTransaction('confirmed-tx', [], { status: TransactionStatus.confirmed, + type: TransactionType.simpleSend, txParams: { from: TEST_ADDRESSES.FROM_ADDRESS, to: ADDRESS_BOOK_RECIPIENT, @@ -5064,6 +5186,7 @@ describe('Address poisoning detection', () => { it('rebuilds known recipients when the transaction array length changes', async () => { const confirmedTransaction = createMockTransaction('confirmed-tx', [], { status: TransactionStatus.confirmed, + type: TransactionType.simpleSend, txParams: { from: TEST_ADDRESSES.FROM_ADDRESS, to: ADDRESS_BOOK_RECIPIENT, @@ -5105,6 +5228,7 @@ describe('Address poisoning detection', () => { it('rebuilds duplicate transaction recipients when transactions are removed', async () => { const firstTransaction = createMockTransaction('confirmed-tx-1', [], { status: TransactionStatus.confirmed, + type: TransactionType.simpleSend, txParams: { from: TEST_ADDRESSES.FROM_ADDRESS, to: ADDRESS_BOOK_RECIPIENT, @@ -5113,6 +5237,7 @@ describe('Address poisoning detection', () => { }); const secondTransaction = createMockTransaction('confirmed-tx-2', [], { status: TransactionStatus.confirmed, + type: TransactionType.simpleSend, txParams: { from: TEST_ADDRESSES.FROM_ADDRESS, to: ADDRESS_BOOK_RECIPIENT, diff --git a/packages/phishing-controller/src/PhishingController.ts b/packages/phishing-controller/src/PhishingController.ts index b4b6365cec1..ec2b5d399d6 100644 --- a/packages/phishing-controller/src/PhishingController.ts +++ b/packages/phishing-controller/src/PhishingController.ts @@ -22,7 +22,7 @@ import type { TransactionMeta, } from '@metamask/transaction-controller'; import { - getEffectiveRecipient, + getSendRecipients, TransactionStatus, } from '@metamask/transaction-controller'; import type { Patch } from 'immer'; @@ -962,18 +962,11 @@ export class PhishingController extends BaseController< return []; } - const transactionRecipient = this.#normalizeAddress( - getEffectiveRecipient(transaction), - ); - const swapAndSendRecipient = this.#normalizeAddress( - transaction.swapAndSendRecipient, - ); - return Array.from( new Set( - [transactionRecipient, swapAndSendRecipient].filter( - (address): address is string => Boolean(address), - ), + getSendRecipients(transaction) + .map((address) => this.#normalizeAddress(address)) + .filter((address): address is string => Boolean(address)), ), ); } diff --git a/packages/transaction-controller/CHANGELOG.md b/packages/transaction-controller/CHANGELOG.md index 29d1afc6755..88d53a47af6 100644 --- a/packages/transaction-controller/CHANGELOG.md +++ b/packages/transaction-controller/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Export `getSendRecipients`, which returns user-chosen send payees for a transaction (native `simpleSend` recipients, decoded ERC-20/721/1155 transfer payees, `swapAndSendRecipient`, and nested batch sends) and omits protocol addresses such as token, Permit2, and router contracts + ### Changed - Bump `@metamask/remote-feature-flag-controller` from `^6.0.0` to `^6.1.0` ([#9980](https://github.com/MetaMask/core/pull/9980)) diff --git a/packages/transaction-controller/src/index.ts b/packages/transaction-controller/src/index.ts index 43b152990d4..b7b17038a97 100644 --- a/packages/transaction-controller/src/index.ts +++ b/packages/transaction-controller/src/index.ts @@ -141,7 +141,7 @@ export { normalizeTransactionParams, } from './utils/utils.js'; export { hasTransactionType } from './utils/transaction-type.js'; -export { getEffectiveRecipient } from './utils/recipient.js'; +export { getEffectiveRecipient, getSendRecipients } from './utils/recipient.js'; export { CHAIN_IDS } from './constants.js'; export { HARDFORK } from './utils/prepare.js'; export { getAccountAddressRelationship } from './api/accounts-api.js'; diff --git a/packages/transaction-controller/src/utils/recipient.test.ts b/packages/transaction-controller/src/utils/recipient.test.ts index c76185e198d..4125059cbd0 100644 --- a/packages/transaction-controller/src/utils/recipient.test.ts +++ b/packages/transaction-controller/src/utils/recipient.test.ts @@ -1,6 +1,6 @@ import type { TransactionMeta } from '../types.js'; import { TransactionStatus, TransactionType } from '../types.js'; -import { getEffectiveRecipient } from './recipient.js'; +import { getEffectiveRecipient, getSendRecipients } from './recipient.js'; const FROM_ADDRESS = '0x0987654321098765432109876543210987654321'; const TOKEN_CONTRACT = '0x1234567890123456789012345678901234567890'; @@ -106,3 +106,138 @@ describe('getEffectiveRecipient', () => { expect(getEffectiveRecipient(transactionMeta)).toBe(TOKEN_CONTRACT); }); }); + +describe('getSendRecipients', () => { + it('returns the native recipient for simple sends', () => { + const transactionMeta = buildTransactionMeta( + TransactionType.simpleSend, + undefined, + TOKEN_RECIPIENT, + ); + + expect(getSendRecipients(transactionMeta)).toEqual([TOKEN_RECIPIENT]); + }); + + it('prefers txParamsOriginal.to when container wrapping replaced the recipient', () => { + const transactionMeta = { + ...buildTransactionMeta( + TransactionType.simpleSend, + undefined, + TOKEN_CONTRACT, + ), + txParamsOriginal: { + from: FROM_ADDRESS, + to: TOKEN_RECIPIENT, + value: '0x0', + }, + }; + + expect(getSendRecipients(transactionMeta)).toEqual([TOKEN_RECIPIENT]); + }); + + it('returns the decoded payee for token transfers and ignores the token contract', () => { + const transactionMeta = buildTransactionMeta( + TransactionType.tokenMethodTransfer, + TRANSFER_DATA, + ); + + expect( + getSendRecipients(transactionMeta).map((address) => + address.toLowerCase(), + ), + ).toEqual([TOKEN_RECIPIENT]); + }); + + it('returns the decoded payee for transferFrom transactions', () => { + const transactionMeta = buildTransactionMeta( + TransactionType.tokenMethodTransferFrom, + TRANSFER_FROM_DATA, + ); + + expect( + getSendRecipients(transactionMeta).map((address) => + address.toLowerCase(), + ), + ).toEqual([TOKEN_RECIPIENT]); + }); + + it('returns no recipients when token transfer calldata cannot be decoded', () => { + const transactionMeta = buildTransactionMeta( + TransactionType.tokenMethodTransfer, + '0x01', + ); + + expect(getSendRecipients(transactionMeta)).toEqual([]); + }); + + it('returns no recipients for approve transactions', () => { + const transactionMeta = buildTransactionMeta( + TransactionType.tokenMethodApprove, + '0x095ea7b3000000000000000000000000cccccccccccccccccccccccccccccccccccccccc0000000000000000000000000000000000000000000000000000000000000001', + ); + + expect(getSendRecipients(transactionMeta)).toEqual([]); + }); + + it('returns no recipients for contract interactions', () => { + const transactionMeta = buildTransactionMeta( + TransactionType.contractInteraction, + TRANSFER_DATA, + ); + + expect(getSendRecipients(transactionMeta)).toEqual([]); + }); + + it('returns swapAndSendRecipient for swap-and-send transactions', () => { + const transactionMeta = { + ...buildTransactionMeta(TransactionType.swapAndSend, TRANSFER_DATA), + swapAndSendRecipient: TOKEN_RECIPIENT, + }; + + expect(getSendRecipients(transactionMeta)).toEqual([TOKEN_RECIPIENT]); + }); + + it('includes nested send and transfer payees from a batch', () => { + const nestedSendRecipient = '0x1234dddddddddddddddddddddddddddddddd9abc'; + const transactionMeta = { + ...buildTransactionMeta( + TransactionType.batch, + '0xdeadbeef', + TOKEN_CONTRACT, + ), + nestedTransactions: [ + { + to: nestedSendRecipient, + type: TransactionType.simpleSend, + }, + { + data: TRANSFER_DATA, + to: TOKEN_CONTRACT, + type: TransactionType.tokenMethodTransfer, + }, + { + to: TOKEN_CONTRACT, + type: TransactionType.tokenMethodApprove, + }, + ], + }; + + expect( + getSendRecipients(transactionMeta).map((address) => + address.toLowerCase(), + ), + ).toEqual([nestedSendRecipient, TOKEN_RECIPIENT]); + }); + + it('treats untyped transactions with no calldata as native sends', () => { + const transactionMeta = buildTransactionMeta( + TransactionType.simpleSend, + undefined, + TOKEN_RECIPIENT, + ); + + expect(getSendRecipients({ ...transactionMeta, type: undefined })).toEqual([ + TOKEN_RECIPIENT, + ]); + }); +}); diff --git a/packages/transaction-controller/src/utils/recipient.ts b/packages/transaction-controller/src/utils/recipient.ts index 5b89613cd77..456cdcf2878 100644 --- a/packages/transaction-controller/src/utils/recipient.ts +++ b/packages/transaction-controller/src/utils/recipient.ts @@ -1,7 +1,7 @@ import type { TransactionDescription } from '@ethersproject/abi'; import { TransactionType } from '../types.js'; -import type { TransactionMeta } from '../types.js'; +import type { NestedTransactionMetadata, TransactionMeta } from '../types.js'; import { decodeTransactionData } from './transaction-type.js'; const TOKEN_TRANSFER_TYPES = [ @@ -10,6 +10,13 @@ const TOKEN_TRANSFER_TYPES = [ TransactionType.tokenMethodSafeTransferFrom, ]; +type SendRecipientSource = { + data?: string; + swapAndSendRecipient?: string; + to?: string; + type?: TransactionType; +}; + /** * Returns the effective recipient of a transaction. * For ERC-20/ERC-721/ERC-1155 token transfer methods, the recipient is decoded @@ -17,6 +24,10 @@ const TOKEN_TRANSFER_TYPES = [ * address receiving the tokens. For all other transaction types, `txParams.to` * is returned as-is. * + * Address poisoning should use {@link getSendRecipients} instead. This helper + * is for first-time interaction and similar "who are we calling" checks, where + * the contract `to` is the right address for approves and contract calls. + * * @param transactionMeta - Transaction meta with txParams and type. * @returns Effective recipient address, or undefined. */ @@ -33,3 +44,103 @@ export function getEffectiveRecipient( } return to; } + +/** + * Returns user-chosen send payees for a transaction. + * + * Address poisoning compares a candidate against prior recipients the user + * actually sent to. Protocol addresses (token, Permit2, router, spender) are + * dapp-supplied and must not be treated as payees. + * + * Included: + * - `simpleSend` `to`, preferring `txParamsOriginal` when present + * - Decoded `to` / `_to` for ERC-20/721/1155 transfer methods + * - `swapAndSendRecipient` when set + * - Nested batch calls that themselves are sends or token transfers + * - Untyped transactions with no calldata, treated as legacy native sends + * + * @param transactionMeta - Transaction meta with txParams and type. + * @returns Deduplicated send recipient addresses, possibly empty. + */ +export function getSendRecipients(transactionMeta: TransactionMeta): string[] { + const params = transactionMeta.txParamsOriginal ?? transactionMeta.txParams; + const recipients: string[] = []; + const seen = new Set(); + + const addRecipient = (address?: string) => { + const normalized = address?.toLowerCase(); + if (!address || !normalized || seen.has(normalized)) { + return; + } + seen.add(normalized); + recipients.push(address); + }; + + addRecipient( + getSendRecipientFromSource({ + data: params?.data, + swapAndSendRecipient: transactionMeta.swapAndSendRecipient, + to: params?.to, + type: transactionMeta.type, + }), + ); + addRecipient(transactionMeta.swapAndSendRecipient); + + for (const nestedTransaction of transactionMeta.nestedTransactions ?? []) { + addRecipient(getSendRecipientFromNestedTransaction(nestedTransaction)); + } + + return recipients; +} + +function getSendRecipientFromNestedTransaction( + nestedTransaction: NestedTransactionMetadata, +): string | undefined { + return getSendRecipientFromSource({ + data: nestedTransaction.data, + to: nestedTransaction.to, + type: nestedTransaction.type, + }); +} + +function getSendRecipientFromSource({ + data, + swapAndSendRecipient, + to, + type, +}: SendRecipientSource): string | undefined { + if (type === TransactionType.swapAndSend) { + return swapAndSendRecipient; + } + + if (isNativeSendType(type, data)) { + return to; + } + + if (type && TOKEN_TRANSFER_TYPES.includes(type) && hasCalldata(data)) { + return decodeTokenTransferRecipient(data); + } + + return undefined; +} + +function isNativeSendType( + type: TransactionType | undefined, + data?: string, +): boolean { + return ( + type === TransactionType.simpleSend || + (type === undefined && !hasCalldata(data)) + ); +} + +function hasCalldata(data?: string): data is string { + return Boolean(data && data !== '0x'); +} + +function decodeTokenTransferRecipient(data: string): string | undefined { + const parsed = decodeTransactionData(data) as + | TransactionDescription + | undefined; + return (parsed?.args?._to ?? parsed?.args?.to) as string | undefined; +} From 953b9b8e81859c32e7689b71b054bc34b92c2af7 Mon Sep 17 00:00:00 2001 From: Alex Donesky Date: Mon, 24 Aug 2026 12:11:53 -0500 Subject: [PATCH 02/10] chore: add changelog PR links for send-payee poisoning fix --- packages/phishing-controller/CHANGELOG.md | 2 +- packages/transaction-controller/CHANGELOG.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/phishing-controller/CHANGELOG.md b/packages/phishing-controller/CHANGELOG.md index 825613898a4..c82df184a71 100644 --- a/packages/phishing-controller/CHANGELOG.md +++ b/packages/phishing-controller/CHANGELOG.md @@ -24,7 +24,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- Restrict address poisoning known recipients to user-chosen send payees (`simpleSend`, decoded token transfer recipients, `swapAndSendRecipient`, and nested batch sends) so confirmed approves, swaps, and contract interactions no longer add token or protocol addresses to the comparison set +- Restrict address poisoning known recipients to user-chosen send payees (`simpleSend`, decoded token transfer recipients, `swapAndSendRecipient`, and nested batch sends) so confirmed approves, swaps, and contract interactions no longer add token or protocol addresses to the comparison set ([#9943](https://github.com/MetaMask/core/pull/9943)) ## [17.3.1] diff --git a/packages/transaction-controller/CHANGELOG.md b/packages/transaction-controller/CHANGELOG.md index 88d53a47af6..2cda9910a13 100644 --- a/packages/transaction-controller/CHANGELOG.md +++ b/packages/transaction-controller/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- Export `getSendRecipients`, which returns user-chosen send payees for a transaction (native `simpleSend` recipients, decoded ERC-20/721/1155 transfer payees, `swapAndSendRecipient`, and nested batch sends) and omits protocol addresses such as token, Permit2, and router contracts +- Export `getSendRecipients`, which returns user-chosen send payees for a transaction (native `simpleSend` recipients, decoded ERC-20/721/1155 transfer payees, `swapAndSendRecipient`, and nested batch sends) and omits protocol addresses such as token, Permit2, and router contracts ([#9943](https://github.com/MetaMask/core/pull/9943)) ### Changed From 8763e161832cf6998a341afaef711a99da03042a Mon Sep 17 00:00:00 2001 From: Alex Donesky Date: Mon, 31 Aug 2026 12:06:13 -0500 Subject: [PATCH 03/10] fix: address review feedback on send-payee poisoning helper - 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 --- packages/phishing-controller/CHANGELOG.md | 8 ++--- .../src/utils/recipient.test.ts | 20 ++++++------- .../src/utils/recipient.ts | 30 ++++++------------- 3 files changed, 23 insertions(+), 35 deletions(-) diff --git a/packages/phishing-controller/CHANGELOG.md b/packages/phishing-controller/CHANGELOG.md index c82df184a71..a4378ad0334 100644 --- a/packages/phishing-controller/CHANGELOG.md +++ b/packages/phishing-controller/CHANGELOG.md @@ -12,6 +12,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Optimize C2 domain blocklist lookups by switching internal storage from `Array` to `Set`, reducing per-lookup complexity from O(n) to O(1) ([#6388](https://github.com/MetaMask/core/pull/6388)) - Bump `@metamask/transaction-controller` from `^69.5.2` to `^69.6.1` ([#9960](https://github.com/MetaMask/core/pull/9960), [#9969](https://github.com/MetaMask/core/pull/9969)) +### Fixed + +- Restrict address poisoning known recipients to user-chosen send payees (`simpleSend`, decoded token transfer recipients, `swapAndSendRecipient`, and nested batch sends) so confirmed approves, swaps, and contract interactions no longer add token or protocol addresses to the comparison set ([#9943](https://github.com/MetaMask/core/pull/9943)) + ## [17.4.0] ### Added @@ -22,10 +26,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Bump `@metamask/transaction-controller` from `^69.4.0` to `^69.5.2` ([#9780](https://github.com/MetaMask/core/pull/9780), [#9798](https://github.com/MetaMask/core/pull/9798), [#9823](https://github.com/MetaMask/core/pull/9823)) -### Fixed - -- Restrict address poisoning known recipients to user-chosen send payees (`simpleSend`, decoded token transfer recipients, `swapAndSendRecipient`, and nested batch sends) so confirmed approves, swaps, and contract interactions no longer add token or protocol addresses to the comparison set ([#9943](https://github.com/MetaMask/core/pull/9943)) - ## [17.3.1] ### Changed diff --git a/packages/transaction-controller/src/utils/recipient.test.ts b/packages/transaction-controller/src/utils/recipient.test.ts index 4125059cbd0..fa1f9f18404 100644 --- a/packages/transaction-controller/src/utils/recipient.test.ts +++ b/packages/transaction-controller/src/utils/recipient.test.ts @@ -115,7 +115,7 @@ describe('getSendRecipients', () => { TOKEN_RECIPIENT, ); - expect(getSendRecipients(transactionMeta)).toEqual([TOKEN_RECIPIENT]); + expect(getSendRecipients(transactionMeta)).toStrictEqual([TOKEN_RECIPIENT]); }); it('prefers txParamsOriginal.to when container wrapping replaced the recipient', () => { @@ -132,7 +132,7 @@ describe('getSendRecipients', () => { }, }; - expect(getSendRecipients(transactionMeta)).toEqual([TOKEN_RECIPIENT]); + expect(getSendRecipients(transactionMeta)).toStrictEqual([TOKEN_RECIPIENT]); }); it('returns the decoded payee for token transfers and ignores the token contract', () => { @@ -145,7 +145,7 @@ describe('getSendRecipients', () => { getSendRecipients(transactionMeta).map((address) => address.toLowerCase(), ), - ).toEqual([TOKEN_RECIPIENT]); + ).toStrictEqual([TOKEN_RECIPIENT]); }); it('returns the decoded payee for transferFrom transactions', () => { @@ -158,7 +158,7 @@ describe('getSendRecipients', () => { getSendRecipients(transactionMeta).map((address) => address.toLowerCase(), ), - ).toEqual([TOKEN_RECIPIENT]); + ).toStrictEqual([TOKEN_RECIPIENT]); }); it('returns no recipients when token transfer calldata cannot be decoded', () => { @@ -167,7 +167,7 @@ describe('getSendRecipients', () => { '0x01', ); - expect(getSendRecipients(transactionMeta)).toEqual([]); + expect(getSendRecipients(transactionMeta)).toStrictEqual([]); }); it('returns no recipients for approve transactions', () => { @@ -176,7 +176,7 @@ describe('getSendRecipients', () => { '0x095ea7b3000000000000000000000000cccccccccccccccccccccccccccccccccccccccc0000000000000000000000000000000000000000000000000000000000000001', ); - expect(getSendRecipients(transactionMeta)).toEqual([]); + expect(getSendRecipients(transactionMeta)).toStrictEqual([]); }); it('returns no recipients for contract interactions', () => { @@ -185,7 +185,7 @@ describe('getSendRecipients', () => { TRANSFER_DATA, ); - expect(getSendRecipients(transactionMeta)).toEqual([]); + expect(getSendRecipients(transactionMeta)).toStrictEqual([]); }); it('returns swapAndSendRecipient for swap-and-send transactions', () => { @@ -194,7 +194,7 @@ describe('getSendRecipients', () => { swapAndSendRecipient: TOKEN_RECIPIENT, }; - expect(getSendRecipients(transactionMeta)).toEqual([TOKEN_RECIPIENT]); + expect(getSendRecipients(transactionMeta)).toStrictEqual([TOKEN_RECIPIENT]); }); it('includes nested send and transfer payees from a batch', () => { @@ -226,7 +226,7 @@ describe('getSendRecipients', () => { getSendRecipients(transactionMeta).map((address) => address.toLowerCase(), ), - ).toEqual([nestedSendRecipient, TOKEN_RECIPIENT]); + ).toStrictEqual([nestedSendRecipient, TOKEN_RECIPIENT]); }); it('treats untyped transactions with no calldata as native sends', () => { @@ -236,7 +236,7 @@ describe('getSendRecipients', () => { TOKEN_RECIPIENT, ); - expect(getSendRecipients({ ...transactionMeta, type: undefined })).toEqual([ + expect(getSendRecipients({ ...transactionMeta, type: undefined })).toStrictEqual([ TOKEN_RECIPIENT, ]); }); diff --git a/packages/transaction-controller/src/utils/recipient.ts b/packages/transaction-controller/src/utils/recipient.ts index 456cdcf2878..07a3f1abdc2 100644 --- a/packages/transaction-controller/src/utils/recipient.ts +++ b/packages/transaction-controller/src/utils/recipient.ts @@ -1,7 +1,7 @@ import type { TransactionDescription } from '@ethersproject/abi'; import { TransactionType } from '../types.js'; -import type { NestedTransactionMetadata, TransactionMeta } from '../types.js'; +import type { TransactionMeta } from '../types.js'; import { decodeTransactionData } from './transaction-type.js'; const TOKEN_TRANSFER_TYPES = [ @@ -12,7 +12,6 @@ const TOKEN_TRANSFER_TYPES = [ type SendRecipientSource = { data?: string; - swapAndSendRecipient?: string; to?: string; type?: TransactionType; }; @@ -55,7 +54,7 @@ export function getEffectiveRecipient( * Included: * - `simpleSend` `to`, preferring `txParamsOriginal` when present * - Decoded `to` / `_to` for ERC-20/721/1155 transfer methods - * - `swapAndSendRecipient` when set + * - `swapAndSendRecipient` whenever set (only ever a user-entered payee) * - Nested batch calls that themselves are sends or token transfers * - Untyped transactions with no calldata, treated as legacy native sends * @@ -63,11 +62,16 @@ export function getEffectiveRecipient( * @returns Deduplicated send recipient addresses, possibly empty. */ export function getSendRecipients(transactionMeta: TransactionMeta): string[] { + // Swap the whole params object rather than falling back field by field. + // `txParamsOriginal` is a `cloneDeep` snapshot of the pre-wrapping params, so + // its `to` and `data` always describe the same call. Mixing an original `to` + // with a wrapped `data` would misread an untyped transaction as a contract + // call and drop a real payee. const params = transactionMeta.txParamsOriginal ?? transactionMeta.txParams; const recipients: string[] = []; const seen = new Set(); - const addRecipient = (address?: string) => { + const addRecipient = (address?: string): void => { const normalized = address?.toLowerCase(); if (!address || !normalized || seen.has(normalized)) { return; @@ -79,7 +83,6 @@ export function getSendRecipients(transactionMeta: TransactionMeta): string[] { addRecipient( getSendRecipientFromSource({ data: params?.data, - swapAndSendRecipient: transactionMeta.swapAndSendRecipient, to: params?.to, type: transactionMeta.type, }), @@ -87,32 +90,17 @@ export function getSendRecipients(transactionMeta: TransactionMeta): string[] { addRecipient(transactionMeta.swapAndSendRecipient); for (const nestedTransaction of transactionMeta.nestedTransactions ?? []) { - addRecipient(getSendRecipientFromNestedTransaction(nestedTransaction)); + addRecipient(getSendRecipientFromSource(nestedTransaction)); } return recipients; } -function getSendRecipientFromNestedTransaction( - nestedTransaction: NestedTransactionMetadata, -): string | undefined { - return getSendRecipientFromSource({ - data: nestedTransaction.data, - to: nestedTransaction.to, - type: nestedTransaction.type, - }); -} - function getSendRecipientFromSource({ data, - swapAndSendRecipient, to, type, }: SendRecipientSource): string | undefined { - if (type === TransactionType.swapAndSend) { - return swapAndSendRecipient; - } - if (isNativeSendType(type, data)) { return to; } From 878689a268ad175c2af7499edf005676c7bbd2b4 Mon Sep 17 00:00:00 2001 From: Alex Donesky Date: Mon, 31 Aug 2026 12:32:21 -0500 Subject: [PATCH 04/10] chore: run oxfmt on recipient tests Co-Authored-By: Claude Opus 5 --- packages/transaction-controller/src/utils/recipient.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/transaction-controller/src/utils/recipient.test.ts b/packages/transaction-controller/src/utils/recipient.test.ts index fa1f9f18404..a0d138497b1 100644 --- a/packages/transaction-controller/src/utils/recipient.test.ts +++ b/packages/transaction-controller/src/utils/recipient.test.ts @@ -236,8 +236,8 @@ describe('getSendRecipients', () => { TOKEN_RECIPIENT, ); - expect(getSendRecipients({ ...transactionMeta, type: undefined })).toStrictEqual([ - TOKEN_RECIPIENT, - ]); + expect( + getSendRecipients({ ...transactionMeta, type: undefined }), + ).toStrictEqual([TOKEN_RECIPIENT]); }); }); From 846e6a684c0e2b686c62b900e3c7b066b1d0fe22 Mon Sep 17 00:00:00 2001 From: Alex Donesky Date: Mon, 31 Aug 2026 17:00:57 -0500 Subject: [PATCH 05/10] fix: classify retries and no-calldata contract sends as native sends 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 --- .../src/utils/recipient.test.ts | 73 +++++++++++++++++++ .../src/utils/recipient.ts | 44 ++++++++++- 2 files changed, 114 insertions(+), 3 deletions(-) diff --git a/packages/transaction-controller/src/utils/recipient.test.ts b/packages/transaction-controller/src/utils/recipient.test.ts index a0d138497b1..8f7b6c3f13e 100644 --- a/packages/transaction-controller/src/utils/recipient.test.ts +++ b/packages/transaction-controller/src/utils/recipient.test.ts @@ -240,4 +240,77 @@ describe('getSendRecipients', () => { getSendRecipients({ ...transactionMeta, type: undefined }), ).toStrictEqual([TOKEN_RECIPIENT]); }); + + it('classifies a sped-up simple send using originalType', () => { + const transactionMeta = { + ...buildTransactionMeta( + TransactionType.retry, + undefined, + TOKEN_RECIPIENT, + ), + originalType: TransactionType.simpleSend, + }; + + expect(getSendRecipients(transactionMeta)).toStrictEqual([TOKEN_RECIPIENT]); + }); + + it('decodes the payee for a sped-up token transfer using originalType', () => { + const transactionMeta = { + ...buildTransactionMeta(TransactionType.retry, TRANSFER_DATA), + originalType: TransactionType.tokenMethodTransfer, + }; + + expect( + getSendRecipients(transactionMeta).map((address) => + address.toLowerCase(), + ), + ).toStrictEqual([TOKEN_RECIPIENT]); + }); + + it('returns no recipients for a cancellation even with a simpleSend originalType', () => { + const transactionMeta = { + ...buildTransactionMeta(TransactionType.cancel, undefined, FROM_ADDRESS), + originalType: TransactionType.simpleSend, + }; + + expect(getSendRecipients(transactionMeta)).toStrictEqual([]); + }); + + it('treats a native transfer to a contract address as a native send', () => { + const transactionMeta = buildTransactionMeta( + TransactionType.contractInteraction, + undefined, + TOKEN_RECIPIENT, + ); + + expect(getSendRecipients(transactionMeta)).toStrictEqual([TOKEN_RECIPIENT]); + }); + + it('returns no recipients for a contract interaction that has calldata', () => { + const transactionMeta = buildTransactionMeta( + TransactionType.contractInteraction, + '0xdeadbeef', + TOKEN_RECIPIENT, + ); + + expect(getSendRecipients(transactionMeta)).toStrictEqual([]); + }); + + it('treats a nested native transfer to a contract address as a native send', () => { + const transactionMeta = { + ...buildTransactionMeta( + TransactionType.batch, + '0xdeadbeef', + TOKEN_CONTRACT, + ), + nestedTransactions: [ + { + to: TOKEN_RECIPIENT, + type: TransactionType.contractInteraction, + }, + ], + }; + + expect(getSendRecipients(transactionMeta)).toStrictEqual([TOKEN_RECIPIENT]); + }); }); diff --git a/packages/transaction-controller/src/utils/recipient.ts b/packages/transaction-controller/src/utils/recipient.ts index 07a3f1abdc2..1bb15649c70 100644 --- a/packages/transaction-controller/src/utils/recipient.ts +++ b/packages/transaction-controller/src/utils/recipient.ts @@ -57,6 +57,14 @@ export function getEffectiveRecipient( * - `swapAndSendRecipient` whenever set (only ever a user-entered payee) * - Nested batch calls that themselves are sends or token transfers * - Untyped transactions with no calldata, treated as legacy native sends + * - Speed-up transactions (`type: retry`), classified using `originalType`, + * since `txParams` are otherwise unchanged from the transaction being sped + * up. Cancellations (`type: cancel`) are not resolved this way: `to` and + * `data` are overwritten into a self-send with no real payee to track. + * - A native transfer with no calldata to an address that happens to be a + * contract. `determineTransactionType` only returns `simpleSend` when `to` + * is not a contract, so these are typed `contractInteraction` even though + * the user chose that address as a plain payee. * * @param transactionMeta - Transaction meta with txParams and type. * @returns Deduplicated send recipient addresses, possibly empty. @@ -84,7 +92,10 @@ export function getSendRecipients(transactionMeta: TransactionMeta): string[] { getSendRecipientFromSource({ data: params?.data, to: params?.to, - type: transactionMeta.type, + type: getEffectiveType( + transactionMeta.type, + transactionMeta.originalType, + ), }), ); addRecipient(transactionMeta.swapAndSendRecipient); @@ -116,12 +127,39 @@ function isNativeSendType( type: TransactionType | undefined, data?: string, ): boolean { + if (type === TransactionType.simpleSend) { + return true; + } + return ( - type === TransactionType.simpleSend || - (type === undefined && !hasCalldata(data)) + !hasCalldata(data) && + (type === undefined || type === TransactionType.contractInteraction) ); } +/** + * Resolves the type used to classify a transaction as a send. + * + * Speed-up transactions keep the original `txParams`, so they should be + * classified by what they originally were, not by `retry`. Cancellations + * overwrite `to`/`data` into a self-send, so `originalType` would misclassify + * them; `type` is returned unchanged instead. + * + * @param type - The transaction's own type. + * @param originalType - The type before a speed-up replaced it, if any. + * @returns The type to use when classifying the transaction as a send. + */ +function getEffectiveType( + type: TransactionType | undefined, + originalType: TransactionType | undefined, +): TransactionType | undefined { + if (type === TransactionType.retry && originalType) { + return originalType; + } + + return type; +} + function hasCalldata(data?: string): data is string { return Boolean(data && data !== '0x'); } From a85ea0bce6df5ed36e520acc8b08873c6aa811ff Mon Sep 17 00:00:00 2001 From: Alex Donesky Date: Tue, 1 Sep 2026 09:22:04 -0500 Subject: [PATCH 06/10] fix: ignore stale nested transactions on a cancelled batch 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 --- .../src/utils/recipient.test.ts | 39 +++++++++++++++++++ .../src/utils/recipient.ts | 13 +++++-- 2 files changed, 49 insertions(+), 3 deletions(-) diff --git a/packages/transaction-controller/src/utils/recipient.test.ts b/packages/transaction-controller/src/utils/recipient.test.ts index 8f7b6c3f13e..443c2e2137c 100644 --- a/packages/transaction-controller/src/utils/recipient.test.ts +++ b/packages/transaction-controller/src/utils/recipient.test.ts @@ -313,4 +313,43 @@ describe('getSendRecipients', () => { expect(getSendRecipients(transactionMeta)).toStrictEqual([TOKEN_RECIPIENT]); }); + + it('ignores stale nested transactions on a cancelled batch', () => { + const nestedSendRecipient = '0x1234dddddddddddddddddddddddddddddddd9abc'; + const transactionMeta = { + ...buildTransactionMeta(TransactionType.cancel, undefined, FROM_ADDRESS), + originalType: TransactionType.batch, + nestedTransactions: [ + { + to: nestedSendRecipient, + type: TransactionType.simpleSend, + }, + { + data: TRANSFER_DATA, + to: TOKEN_CONTRACT, + type: TransactionType.tokenMethodTransfer, + }, + ], + }; + + expect(getSendRecipients(transactionMeta)).toStrictEqual([]); + }); + + it('still processes nested transactions on a sped-up batch', () => { + const nestedSendRecipient = '0x1234dddddddddddddddddddddddddddddddd9abc'; + const transactionMeta = { + ...buildTransactionMeta(TransactionType.retry, undefined, TOKEN_CONTRACT), + originalType: TransactionType.batch, + nestedTransactions: [ + { + to: nestedSendRecipient, + type: TransactionType.simpleSend, + }, + ], + }; + + expect(getSendRecipients(transactionMeta)).toStrictEqual([ + nestedSendRecipient, + ]); + }); }); diff --git a/packages/transaction-controller/src/utils/recipient.ts b/packages/transaction-controller/src/utils/recipient.ts index 1bb15649c70..ab3b9f79c09 100644 --- a/packages/transaction-controller/src/utils/recipient.ts +++ b/packages/transaction-controller/src/utils/recipient.ts @@ -60,7 +60,9 @@ export function getEffectiveRecipient( * - Speed-up transactions (`type: retry`), classified using `originalType`, * since `txParams` are otherwise unchanged from the transaction being sped * up. Cancellations (`type: cancel`) are not resolved this way: `to` and - * `data` are overwritten into a self-send with no real payee to track. + * `data` are overwritten into a self-send with no real payee to track, and + * a cancelled batch's nested transactions are skipped entirely for the + * same reason, even though they are still present on the transaction. * - A native transfer with no calldata to an address that happens to be a * contract. `determineTransactionType` only returns `simpleSend` when `to` * is not a contract, so these are typed `contractInteraction` even though @@ -100,8 +102,13 @@ export function getSendRecipients(transactionMeta: TransactionMeta): string[] { ); addRecipient(transactionMeta.swapAndSendRecipient); - for (const nestedTransaction of transactionMeta.nestedTransactions ?? []) { - addRecipient(getSendRecipientFromSource(nestedTransaction)); + // A cancellation keeps the batch's original nestedTransactions (only + // txParams/type are overwritten into a self-send), but none of those nested + // calls actually executed, so they must not be read as real payees. + if (transactionMeta.type !== TransactionType.cancel) { + for (const nestedTransaction of transactionMeta.nestedTransactions ?? []) { + addRecipient(getSendRecipientFromSource(nestedTransaction)); + } } return recipients; From 9fdd24dfc7adf8f5ccc0690eca4dd9686b4fe238 Mon Sep 17 00:00:00 2001 From: Alex Donesky Date: Tue, 1 Sep 2026 09:41:57 -0500 Subject: [PATCH 07/10] test: cover NFT send recipient decoding Verify ERC-721 and ERC-1155 safe transfers contribute their decoded payees to address poisoning recipient checks. --- .../src/utils/recipient.test.ts | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/packages/transaction-controller/src/utils/recipient.test.ts b/packages/transaction-controller/src/utils/recipient.test.ts index 443c2e2137c..d7b8dfb2f16 100644 --- a/packages/transaction-controller/src/utils/recipient.test.ts +++ b/packages/transaction-controller/src/utils/recipient.test.ts @@ -1,3 +1,6 @@ +import { Interface } from '@ethersproject/abi'; +import { abiERC721, abiERC1155 } from '@metamask/metamask-eth-abis'; + import type { TransactionMeta } from '../types.js'; import { TransactionStatus, TransactionType } from '../types.js'; import { getEffectiveRecipient, getSendRecipients } from './recipient.js'; @@ -18,6 +21,24 @@ const TRANSFER_FROM_DATA = `0x23b872dd000000000000000000000000${FROM_ADDRESS.sli 2, )}0000000000000000000000000000000000000000000000000000000000000064`; +const ERC721_SAFE_TRANSFER_FROM_DATA = new Interface( + abiERC721, +).encodeFunctionData('safeTransferFrom(address,address,uint256)', [ + FROM_ADDRESS, + TOKEN_RECIPIENT, + '1', +]); + +const ERC1155_SAFE_TRANSFER_FROM_DATA = new Interface( + abiERC1155, +).encodeFunctionData('safeTransferFrom', [ + FROM_ADDRESS, + TOKEN_RECIPIENT, + '1', + '1', + '0x', +]); + /** * Builds a minimal transaction meta object for testing. * @@ -161,6 +182,32 @@ describe('getSendRecipients', () => { ).toStrictEqual([TOKEN_RECIPIENT]); }); + it('returns the decoded payee for ERC-721 safeTransferFrom transactions', () => { + const transactionMeta = buildTransactionMeta( + TransactionType.tokenMethodSafeTransferFrom, + ERC721_SAFE_TRANSFER_FROM_DATA, + ); + + expect( + getSendRecipients(transactionMeta).map((address) => + address.toLowerCase(), + ), + ).toStrictEqual([TOKEN_RECIPIENT]); + }); + + it('returns the decoded payee for ERC-1155 safeTransferFrom transactions', () => { + const transactionMeta = buildTransactionMeta( + TransactionType.tokenMethodSafeTransferFrom, + ERC1155_SAFE_TRANSFER_FROM_DATA, + ); + + expect( + getSendRecipients(transactionMeta).map((address) => + address.toLowerCase(), + ), + ).toStrictEqual([TOKEN_RECIPIENT]); + }); + it('returns no recipients when token transfer calldata cannot be decoded', () => { const transactionMeta = buildTransactionMeta( TransactionType.tokenMethodTransfer, From 3f7b513ad2eeef66f78f93d9189e0b73ac8a326d Mon Sep 17 00:00:00 2001 From: Alex Donesky Date: Tue, 1 Sep 2026 09:51:57 -0500 Subject: [PATCH 08/10] fix: include ERC-1155 batch transfer recipients Decode safeBatchTransferFrom payees even though those calls classify as contract interactions, so address poisoning checks cover both top-level and nested transfers. --- .../src/utils/recipient.test.ts | 46 +++++++++++++++++++ .../src/utils/recipient.ts | 11 +++++ 2 files changed, 57 insertions(+) diff --git a/packages/transaction-controller/src/utils/recipient.test.ts b/packages/transaction-controller/src/utils/recipient.test.ts index d7b8dfb2f16..0a47114b7db 100644 --- a/packages/transaction-controller/src/utils/recipient.test.ts +++ b/packages/transaction-controller/src/utils/recipient.test.ts @@ -39,6 +39,16 @@ const ERC1155_SAFE_TRANSFER_FROM_DATA = new Interface( '0x', ]); +const ERC1155_SAFE_BATCH_TRANSFER_FROM_DATA = new Interface( + abiERC1155, +).encodeFunctionData('safeBatchTransferFrom', [ + FROM_ADDRESS, + TOKEN_RECIPIENT, + ['1'], + ['1'], + '0x', +]); + /** * Builds a minimal transaction meta object for testing. * @@ -235,6 +245,19 @@ describe('getSendRecipients', () => { expect(getSendRecipients(transactionMeta)).toStrictEqual([]); }); + it('returns the decoded payee for ERC-1155 safeBatchTransferFrom contract interactions', () => { + const transactionMeta = buildTransactionMeta( + TransactionType.contractInteraction, + ERC1155_SAFE_BATCH_TRANSFER_FROM_DATA, + ); + + expect( + getSendRecipients(transactionMeta).map((address) => + address.toLowerCase(), + ), + ).toStrictEqual([TOKEN_RECIPIENT]); + }); + it('returns swapAndSendRecipient for swap-and-send transactions', () => { const transactionMeta = { ...buildTransactionMeta(TransactionType.swapAndSend, TRANSFER_DATA), @@ -276,6 +299,29 @@ describe('getSendRecipients', () => { ).toStrictEqual([nestedSendRecipient, TOKEN_RECIPIENT]); }); + it('includes a nested ERC-1155 safeBatchTransferFrom payee from a batch', () => { + const transactionMeta = { + ...buildTransactionMeta( + TransactionType.batch, + '0xdeadbeef', + TOKEN_CONTRACT, + ), + nestedTransactions: [ + { + data: ERC1155_SAFE_BATCH_TRANSFER_FROM_DATA, + to: TOKEN_CONTRACT, + type: TransactionType.contractInteraction, + }, + ], + }; + + expect( + getSendRecipients(transactionMeta).map((address) => + address.toLowerCase(), + ), + ).toStrictEqual([TOKEN_RECIPIENT]); + }); + it('treats untyped transactions with no calldata as native sends', () => { const transactionMeta = buildTransactionMeta( TransactionType.simpleSend, diff --git a/packages/transaction-controller/src/utils/recipient.ts b/packages/transaction-controller/src/utils/recipient.ts index ab3b9f79c09..a8e29d2943b 100644 --- a/packages/transaction-controller/src/utils/recipient.ts +++ b/packages/transaction-controller/src/utils/recipient.ts @@ -10,6 +10,8 @@ const TOKEN_TRANSFER_TYPES = [ TransactionType.tokenMethodSafeTransferFrom, ]; +const ERC1155_SAFE_BATCH_TRANSFER_FROM_SELECTOR = '0x2eb2c2d6'; + type SendRecipientSource = { data?: string; to?: string; @@ -54,6 +56,7 @@ export function getEffectiveRecipient( * Included: * - `simpleSend` `to`, preferring `txParamsOriginal` when present * - Decoded `to` / `_to` for ERC-20/721/1155 transfer methods + * - Decoded `to` for ERC-1155 `safeBatchTransferFrom` * - `swapAndSendRecipient` whenever set (only ever a user-entered payee) * - Nested batch calls that themselves are sends or token transfers * - Untyped transactions with no calldata, treated as legacy native sends @@ -127,6 +130,14 @@ function getSendRecipientFromSource({ return decodeTokenTransferRecipient(data); } + if ( + hasCalldata(data) && + data.slice(0, 10).toLowerCase() === + ERC1155_SAFE_BATCH_TRANSFER_FROM_SELECTOR + ) { + return decodeTokenTransferRecipient(data); + } + return undefined; } From ac85c318f494674ca8983cd31dc1cb0a674de02d Mon Sep 17 00:00:00 2001 From: Alex Donesky Date: Tue, 1 Sep 2026 10:30:36 -0500 Subject: [PATCH 09/10] fix: ignore recipients from cancellation retries 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. --- .../src/utils/recipient.test.ts | 31 +++++++++++++++++++ .../src/utils/recipient.ts | 31 ++++++++++++------- 2 files changed, 51 insertions(+), 11 deletions(-) diff --git a/packages/transaction-controller/src/utils/recipient.test.ts b/packages/transaction-controller/src/utils/recipient.test.ts index 0a47114b7db..2160d0bd9bc 100644 --- a/packages/transaction-controller/src/utils/recipient.test.ts +++ b/packages/transaction-controller/src/utils/recipient.test.ts @@ -369,6 +369,37 @@ describe('getSendRecipients', () => { expect(getSendRecipients(transactionMeta)).toStrictEqual([]); }); + it('returns no recipients for a cancellation with stale original ERC-1155 batch calldata', () => { + const transactionMeta = { + ...buildTransactionMeta(TransactionType.cancel, undefined, FROM_ADDRESS), + txParamsOriginal: { + data: ERC1155_SAFE_BATCH_TRANSFER_FROM_DATA, + from: FROM_ADDRESS, + to: TOKEN_CONTRACT, + value: '0x0', + }, + }; + + expect(getSendRecipients(transactionMeta)).toStrictEqual([]); + }); + + it('returns no recipients for a sped-up cancellation with stale send metadata', () => { + const nestedSendRecipient = '0x1234dddddddddddddddddddddddddddddddd9abc'; + const transactionMeta = { + ...buildTransactionMeta(TransactionType.retry, undefined, FROM_ADDRESS), + originalType: TransactionType.cancel, + swapAndSendRecipient: TOKEN_RECIPIENT, + nestedTransactions: [ + { + to: nestedSendRecipient, + type: TransactionType.simpleSend, + }, + ], + }; + + expect(getSendRecipients(transactionMeta)).toStrictEqual([]); + }); + it('treats a native transfer to a contract address as a native send', () => { const transactionMeta = buildTransactionMeta( TransactionType.contractInteraction, diff --git a/packages/transaction-controller/src/utils/recipient.ts b/packages/transaction-controller/src/utils/recipient.ts index a8e29d2943b..94231238e2b 100644 --- a/packages/transaction-controller/src/utils/recipient.ts +++ b/packages/transaction-controller/src/utils/recipient.ts @@ -62,10 +62,10 @@ export function getEffectiveRecipient( * - Untyped transactions with no calldata, treated as legacy native sends * - Speed-up transactions (`type: retry`), classified using `originalType`, * since `txParams` are otherwise unchanged from the transaction being sped - * up. Cancellations (`type: cancel`) are not resolved this way: `to` and - * `data` are overwritten into a self-send with no real payee to track, and - * a cancelled batch's nested transactions are skipped entirely for the - * same reason, even though they are still present on the transaction. + * up. + * - Cancellations and retries of cancellations return no payees. Their + * original params, nested transactions, and swap-and-send recipient can + * remain on the metadata even though none of those sends executed. * - A native transfer with no calldata to an address that happens to be a * contract. `determineTransactionType` only returns `simpleSend` when `to` * is not a contract, so these are typed `contractInteraction` even though @@ -75,6 +75,10 @@ export function getEffectiveRecipient( * @returns Deduplicated send recipient addresses, possibly empty. */ export function getSendRecipients(transactionMeta: TransactionMeta): string[] { + if (isCancellation(transactionMeta)) { + return []; + } + // Swap the whole params object rather than falling back field by field. // `txParamsOriginal` is a `cloneDeep` snapshot of the pre-wrapping params, so // its `to` and `data` always describe the same call. Mixing an original `to` @@ -105,18 +109,23 @@ export function getSendRecipients(transactionMeta: TransactionMeta): string[] { ); addRecipient(transactionMeta.swapAndSendRecipient); - // A cancellation keeps the batch's original nestedTransactions (only - // txParams/type are overwritten into a self-send), but none of those nested - // calls actually executed, so they must not be read as real payees. - if (transactionMeta.type !== TransactionType.cancel) { - for (const nestedTransaction of transactionMeta.nestedTransactions ?? []) { - addRecipient(getSendRecipientFromSource(nestedTransaction)); - } + for (const nestedTransaction of transactionMeta.nestedTransactions ?? []) { + addRecipient(getSendRecipientFromSource(nestedTransaction)); } return recipients; } +function isCancellation({ + type, + originalType, +}: Pick): boolean { + return ( + type === TransactionType.cancel || + (type === TransactionType.retry && originalType === TransactionType.cancel) + ); +} + function getSendRecipientFromSource({ data, to, From 994d9777120628fbd7a0bc3a46bd8cd9c3d716fc Mon Sep 17 00:00:00 2001 From: Alex Donesky Date: Tue, 1 Sep 2026 11:27:16 -0500 Subject: [PATCH 10/10] fix: decode wrapped token transfer recipients Infer transfer methods from original calldata when wrapping replaces transaction classification, preserving address-poisoning payees without broadening ordinary contract interactions. --- .../src/utils/recipient.test.ts | 45 +++++++++++++++++++ .../src/utils/recipient.ts | 33 ++++++++++++-- 2 files changed, 74 insertions(+), 4 deletions(-) diff --git a/packages/transaction-controller/src/utils/recipient.test.ts b/packages/transaction-controller/src/utils/recipient.test.ts index 2160d0bd9bc..29d23c9639d 100644 --- a/packages/transaction-controller/src/utils/recipient.test.ts +++ b/packages/transaction-controller/src/utils/recipient.test.ts @@ -166,6 +166,34 @@ describe('getSendRecipients', () => { expect(getSendRecipients(transactionMeta)).toStrictEqual([TOKEN_RECIPIENT]); }); + it.each([ + { data: TRANSFER_DATA, standard: 'ERC-20' }, + { data: ERC721_SAFE_TRANSFER_FROM_DATA, standard: 'ERC-721' }, + { data: ERC1155_SAFE_TRANSFER_FROM_DATA, standard: 'ERC-1155' }, + ])( + 'decodes an $standard payee from original params after wrapping changed the type', + ({ data }) => { + const transactionMeta = { + ...buildTransactionMeta( + TransactionType.contractInteraction, + '0xdeadbeef', + ), + txParamsOriginal: { + data, + from: FROM_ADDRESS, + to: TOKEN_CONTRACT, + value: '0x0', + }, + }; + + expect( + getSendRecipients(transactionMeta).map((address) => + address.toLowerCase(), + ), + ).toStrictEqual([TOKEN_RECIPIENT]); + }, + ); + it('returns the decoded payee for token transfers and ignores the token contract', () => { const transactionMeta = buildTransactionMeta( TransactionType.tokenMethodTransfer, @@ -236,6 +264,23 @@ describe('getSendRecipients', () => { expect(getSendRecipients(transactionMeta)).toStrictEqual([]); }); + it('does not infer an approval recipient from original params', () => { + const transactionMeta = { + ...buildTransactionMeta( + TransactionType.contractInteraction, + '0xdeadbeef', + ), + txParamsOriginal: { + data: '0x095ea7b3000000000000000000000000cccccccccccccccccccccccccccccccccccccccc0000000000000000000000000000000000000000000000000000000000000001', + from: FROM_ADDRESS, + to: TOKEN_CONTRACT, + value: '0x0', + }, + }; + + expect(getSendRecipients(transactionMeta)).toStrictEqual([]); + }); + it('returns no recipients for contract interactions', () => { const transactionMeta = buildTransactionMeta( TransactionType.contractInteraction, diff --git a/packages/transaction-controller/src/utils/recipient.ts b/packages/transaction-controller/src/utils/recipient.ts index 94231238e2b..55f9d1862f4 100644 --- a/packages/transaction-controller/src/utils/recipient.ts +++ b/packages/transaction-controller/src/utils/recipient.ts @@ -14,6 +14,7 @@ const ERC1155_SAFE_BATCH_TRANSFER_FROM_SELECTOR = '0x2eb2c2d6'; type SendRecipientSource = { data?: string; + inferTransferType?: boolean; to?: string; type?: TransactionType; }; @@ -84,7 +85,8 @@ export function getSendRecipients(transactionMeta: TransactionMeta): string[] { // its `to` and `data` always describe the same call. Mixing an original `to` // with a wrapped `data` would misread an untyped transaction as a contract // call and drop a real payee. - const params = transactionMeta.txParamsOriginal ?? transactionMeta.txParams; + const originalParams = transactionMeta.txParamsOriginal; + const params = originalParams ?? transactionMeta.txParams; const recipients: string[] = []; const seen = new Set(); @@ -100,6 +102,7 @@ export function getSendRecipients(transactionMeta: TransactionMeta): string[] { addRecipient( getSendRecipientFromSource({ data: params?.data, + inferTransferType: Boolean(originalParams), to: params?.to, type: getEffectiveType( transactionMeta.type, @@ -128,6 +131,7 @@ function isCancellation({ function getSendRecipientFromSource({ data, + inferTransferType, to, type, }: SendRecipientSource): string | undefined { @@ -135,14 +139,26 @@ function getSendRecipientFromSource({ return to; } - if (type && TOKEN_TRANSFER_TYPES.includes(type) && hasCalldata(data)) { + if (!hasCalldata(data)) { + return undefined; + } + + // Wrapping can replace the current type while preserving the original + // calldata without its original type. Infer only for original params so + // ordinary contract interactions remain excluded. + const inferredType = inferTransferType + ? (decodeTransactionData(data, { + getMethodName: true, + }) as string | undefined) + : undefined; + + if (isTokenTransferType(type) || isTokenTransferType(inferredType)) { return decodeTokenTransferRecipient(data); } if ( - hasCalldata(data) && data.slice(0, 10).toLowerCase() === - ERC1155_SAFE_BATCH_TRANSFER_FROM_SELECTOR + ERC1155_SAFE_BATCH_TRANSFER_FROM_SELECTOR ) { return decodeTokenTransferRecipient(data); } @@ -150,6 +166,15 @@ function getSendRecipientFromSource({ return undefined; } +function isTokenTransferType(type?: string): boolean { + return Boolean( + type && + TOKEN_TRANSFER_TYPES.some( + (transferType) => transferType.toLowerCase() === type.toLowerCase(), + ), + ); +} + function isNativeSendType( type: TransactionType | undefined, data?: string,