diff --git a/packages/phishing-controller/CHANGELOG.md b/packages/phishing-controller/CHANGELOG.md index 472306f647a..16068e96af4 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.0.0` to `^69.3.0` ([#9568](https://github.com/MetaMask/core/pull/9568), [#9589](https://github.com/MetaMask/core/pull/9589), [#9593](https://github.com/MetaMask/core/pull/9593), [#9693](https://github.com/MetaMask/core/pull/9693)) +### Fixed + +- Address poisoning known recipients now use the actual token recipient decoded from calldata for confirmed ERC-20/ERC-721/ERC-1155 token transfers, instead of the token contract address from `txParams.to` ([#9699](https://github.com/MetaMask/core/pull/9699)) + ## [17.3.0] ### Added diff --git a/packages/phishing-controller/src/PhishingController.test.ts b/packages/phishing-controller/src/PhishingController.test.ts index eeacb996d9a..d7437aa8a7c 100644 --- a/packages/phishing-controller/src/PhishingController.test.ts +++ b/packages/phishing-controller/src/PhishingController.test.ts @@ -6,7 +6,10 @@ import type { MessengerEvents, MockAnyNamespace, } from '@metamask/messenger'; -import { TransactionStatus } from '@metamask/transaction-controller'; +import { + TransactionStatus, + TransactionType, +} from '@metamask/transaction-controller'; import type { TransactionControllerState } from '@metamask/transaction-controller'; import { strict as assert } from 'assert'; import nock, { cleanAll, isDone, pendingMocks } from 'nock'; @@ -4632,6 +4635,59 @@ describe('Address poisoning detection', () => { ]); }); + it('uses the decoded token recipient instead of the token contract for confirmed token transfers', () => { + const TOKEN_CONTRACT = + '0xdddd111111111111111111111111111111119999' as `0x${string}`; + const CONTRACT_CANDIDATE_ADDRESS = + '0xddddaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa9999' as `0x${string}`; + // transfer(address _to, uint256 _value) sending tokens to CONFIRMED_TX_RECIPIENT + const transferData = + `0xa9059cbb000000000000000000000000${CONFIRMED_TX_RECIPIENT.slice( + 2, + )}0000000000000000000000000000000000000000000000000000000000000064` as `0x${string}`; + + const tokenTransferTransaction = createMockTransaction( + 'token-transfer-tx', + [], + { + status: TransactionStatus.confirmed, + type: TransactionType.tokenMethodTransfer, + txParams: { + from: TEST_ADDRESSES.FROM_ADDRESS, + to: TOKEN_CONTRACT, + value: '0x0' as `0x${string}`, + data: transferData, + }, + }, + ); + + const { messenger } = setupMessenger({ + transactionControllerState: { + ...getDefaultTransactionControllerState(), + transactions: [tokenTransferTransaction], + }, + }); + + 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 non-confirmed transactions when hydrating known recipients', () => { const { messenger } = setupMessenger({ transactionControllerState: { diff --git a/packages/phishing-controller/src/PhishingController.ts b/packages/phishing-controller/src/PhishingController.ts index 02da6b89e2f..3ec4df29c1c 100644 --- a/packages/phishing-controller/src/PhishingController.ts +++ b/packages/phishing-controller/src/PhishingController.ts @@ -21,7 +21,10 @@ import type { TransactionControllerStateChangeEvent, TransactionMeta, } from '@metamask/transaction-controller'; -import { TransactionStatus } from '@metamask/transaction-controller'; +import { + getEffectiveRecipient, + TransactionStatus, +} from '@metamask/transaction-controller'; import type { Patch } from 'immer'; import { toASCII } from 'punycode/punycode.js'; @@ -960,7 +963,7 @@ export class PhishingController extends BaseController< } const transactionRecipient = this.#normalizeAddress( - transaction.txParams.to, + getEffectiveRecipient(transaction), ); const swapAndSendRecipient = this.#normalizeAddress( transaction.swapAndSendRecipient, diff --git a/packages/transaction-controller/CHANGELOG.md b/packages/transaction-controller/CHANGELOG.md index 2d462fae3ca..40c49b64977 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 `getEffectiveRecipient` utility that returns the actual recipient of a transaction, decoding it from calldata for ERC-20/ERC-721/ERC-1155 token transfer methods where `txParams.to` is the token contract ([#9699](https://github.com/MetaMask/core/pull/9699)) + ## [69.3.0] ### Added diff --git a/packages/transaction-controller/src/index.ts b/packages/transaction-controller/src/index.ts index 7ab9bb60ea3..43b152990d4 100644 --- a/packages/transaction-controller/src/index.ts +++ b/packages/transaction-controller/src/index.ts @@ -141,6 +141,7 @@ export { normalizeTransactionParams, } from './utils/utils.js'; export { hasTransactionType } from './utils/transaction-type.js'; +export { getEffectiveRecipient } 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/first-time-interaction.ts b/packages/transaction-controller/src/utils/first-time-interaction.ts index f6d20c95393..9f854ac14f3 100644 --- a/packages/transaction-controller/src/utils/first-time-interaction.ts +++ b/packages/transaction-controller/src/utils/first-time-interaction.ts @@ -1,38 +1,13 @@ -import type { TransactionDescription } from '@ethersproject/abi'; import type { TraceContext, TraceCallback } from '@metamask/controller-utils'; import { hexToNumber } from '@metamask/utils'; import { getAccountAddressRelationship } from '../api/accounts-api.js'; import type { GetAccountAddressRelationshipRequest } from '../api/accounts-api.js'; import { projectLogger as log } from '../logger.js'; -import { TransactionType } from '../types.js'; import type { TransactionMeta } from '../types.js'; -import { decodeTransactionData } from './transaction-type.js'; +import { getEffectiveRecipient } from './recipient.js'; import { validateParamTo } from './validation.js'; -const TOKEN_TRANSFER_TYPES = [ - TransactionType.tokenMethodTransfer, - TransactionType.tokenMethodTransferFrom, - TransactionType.tokenMethodSafeTransferFrom, -]; - -/** - * Returns the effective recipient for first-time-interaction checks (decoded from data for token transfers). - * Used when comparing existing transactions so we match by actual recipient, not txParams.to (the token - * contract for ERC20/ERC721/ERC1155 transfer methods). - * - * @param tx - Transaction meta with txParams and type - * @returns Effective recipient address, or undefined - */ -function getEffectiveRecipient(tx: TransactionMeta): string | undefined { - const { data, to } = tx?.txParams ?? {}; - if (data && TOKEN_TRANSFER_TYPES.includes(tx?.type as TransactionType)) { - const parsed = decodeTransactionData(data) as TransactionDescription; - return (parsed?.args?._to ?? parsed?.args?.to ?? to) as string | undefined; - } - return to; -} - type UpdateFirstTimeInteractionRequest = { existingTransactions: TransactionMeta[]; getTransaction: (transactionId: string) => TransactionMeta | undefined; diff --git a/packages/transaction-controller/src/utils/recipient.test.ts b/packages/transaction-controller/src/utils/recipient.test.ts new file mode 100644 index 00000000000..c76185e198d --- /dev/null +++ b/packages/transaction-controller/src/utils/recipient.test.ts @@ -0,0 +1,108 @@ +import type { TransactionMeta } from '../types.js'; +import { TransactionStatus, TransactionType } from '../types.js'; +import { getEffectiveRecipient } from './recipient.js'; + +const FROM_ADDRESS = '0x0987654321098765432109876543210987654321'; +const TOKEN_CONTRACT = '0x1234567890123456789012345678901234567890'; +const TOKEN_RECIPIENT = '0x1234cccccccccccccccccccccccccccccccc9abc'; + +// transfer(address _to, uint256 _value) +const TRANSFER_DATA = `0xa9059cbb000000000000000000000000${TOKEN_RECIPIENT.slice( + 2, +)}0000000000000000000000000000000000000000000000000000000000000064`; + +// transferFrom(address _from, address _to, uint256 _value) +const TRANSFER_FROM_DATA = `0x23b872dd000000000000000000000000${FROM_ADDRESS.slice( + 2, +)}000000000000000000000000${TOKEN_RECIPIENT.slice( + 2, +)}0000000000000000000000000000000000000000000000000000000000000064`; + +/** + * Builds a minimal transaction meta object for testing. + * + * @param type - The transaction type. + * @param data - Optional transaction calldata. + * @param to - The `txParams.to` address. + * @returns The transaction meta object. + */ +function buildTransactionMeta( + type: TransactionType, + data?: string, + to: string = TOKEN_CONTRACT, +): TransactionMeta { + return { + chainId: '0x1', + id: 'test-tx', + networkClientId: 'mainnet', + status: TransactionStatus.confirmed, + time: 123456789, + txParams: { + from: FROM_ADDRESS, + to, + value: '0x0', + ...(data ? { data } : {}), + }, + type, + } as TransactionMeta; +} + +describe('getEffectiveRecipient', () => { + it('returns txParams.to for simple sends', () => { + const transactionMeta = buildTransactionMeta( + TransactionType.simpleSend, + undefined, + TOKEN_RECIPIENT, + ); + + expect(getEffectiveRecipient(transactionMeta)).toBe(TOKEN_RECIPIENT); + }); + + it('returns txParams.to for contract interactions even when calldata is present', () => { + const transactionMeta = buildTransactionMeta( + TransactionType.contractInteraction, + TRANSFER_DATA, + ); + + expect(getEffectiveRecipient(transactionMeta)).toBe(TOKEN_CONTRACT); + }); + + it('returns the decoded recipient for ERC-20 transfer transactions', () => { + const transactionMeta = buildTransactionMeta( + TransactionType.tokenMethodTransfer, + TRANSFER_DATA, + ); + + expect(getEffectiveRecipient(transactionMeta)?.toLowerCase()).toBe( + TOKEN_RECIPIENT, + ); + }); + + it('returns the decoded recipient for transferFrom transactions', () => { + const transactionMeta = buildTransactionMeta( + TransactionType.tokenMethodTransferFrom, + TRANSFER_FROM_DATA, + ); + + expect(getEffectiveRecipient(transactionMeta)?.toLowerCase()).toBe( + TOKEN_RECIPIENT, + ); + }); + + it('falls back to txParams.to when token transfer calldata cannot be decoded', () => { + const transactionMeta = buildTransactionMeta( + TransactionType.tokenMethodTransfer, + '0x01', + ); + + expect(getEffectiveRecipient(transactionMeta)).toBe(TOKEN_CONTRACT); + }); + + it('returns txParams.to when a token transfer transaction has no calldata', () => { + const transactionMeta = buildTransactionMeta( + TransactionType.tokenMethodTransfer, + ); + + expect(getEffectiveRecipient(transactionMeta)).toBe(TOKEN_CONTRACT); + }); +}); diff --git a/packages/transaction-controller/src/utils/recipient.ts b/packages/transaction-controller/src/utils/recipient.ts new file mode 100644 index 00000000000..5b89613cd77 --- /dev/null +++ b/packages/transaction-controller/src/utils/recipient.ts @@ -0,0 +1,35 @@ +import type { TransactionDescription } from '@ethersproject/abi'; + +import { TransactionType } from '../types.js'; +import type { TransactionMeta } from '../types.js'; +import { decodeTransactionData } from './transaction-type.js'; + +const TOKEN_TRANSFER_TYPES = [ + TransactionType.tokenMethodTransfer, + TransactionType.tokenMethodTransferFrom, + TransactionType.tokenMethodSafeTransferFrom, +]; + +/** + * Returns the effective recipient of a transaction. + * For ERC-20/ERC-721/ERC-1155 token transfer methods, the recipient is decoded + * from the calldata since `txParams.to` is the token contract rather than the + * address receiving the tokens. For all other transaction types, `txParams.to` + * is returned as-is. + * + * @param transactionMeta - Transaction meta with txParams and type. + * @returns Effective recipient address, or undefined. + */ +export function getEffectiveRecipient( + transactionMeta: TransactionMeta, +): string | undefined { + const { data, to } = transactionMeta?.txParams ?? {}; + if ( + data && + TOKEN_TRANSFER_TYPES.includes(transactionMeta?.type as TransactionType) + ) { + const parsed = decodeTransactionData(data) as TransactionDescription; + return (parsed?.args?._to ?? parsed?.args?.to ?? to) as string | undefined; + } + return to; +}