fix: use decoded token recipient for address poisoning known recipients

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

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: {

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,

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

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';

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;

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);
+  });
+});

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;
+}