fix: only scan send payees for address poisoning

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

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,

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

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))

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