diff --git a/packages/phishing-controller/CHANGELOG.md b/packages/phishing-controller/CHANGELOG.md index 66ab0fe4ea1..e5cc1625a30 100644 --- a/packages/phishing-controller/CHANGELOG.md +++ b/packages/phishing-controller/CHANGELOG.md @@ -11,6 +11,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 ([#9943](https://github.com/MetaMask/core/pull/9943)) + ## [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 3ec4df29c1c..87209b22bdd 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 49c3583c997..3b32a501a38 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 ([#9943](https://github.com/MetaMask/core/pull/9943)) + ### Changed - Bump `@metamask/core-backend` from `^8.1.1` to `^8.1.2` ([#9886](https://github.com/MetaMask/core/pull/9886)) 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; +}