diff --git a/app/scripts/lib/transaction/util.test.ts b/app/scripts/lib/transaction/util.test.ts index 19028999a70b..d64590efc61b 100644 --- a/app/scripts/lib/transaction/util.test.ts +++ b/app/scripts/lib/transaction/util.test.ts @@ -528,6 +528,40 @@ describe('Transaction Utils', () => { expect(validateRequestWithPPOMMock).toHaveBeenCalledTimes(0); }); + + it('does not call PPOM if is a transfer to self and value is zero', async () => { + const INTERNAL_ACCOUNT_ADDRESS_2 = + '0x68d3ad12ea94779cb37262be1c179dbd8e208afe'; + const sendRequest = { + ...request, + internalAccounts: [ + createMockInternalAccount({ + address: INTERNAL_ACCOUNT_ADDRESS_2, + }), + ], + transactionParams: { + ...request.transactionParams, + data: '0xa9059cbb00000000000000000000000068d3ad12ea94779cb37262be1c179dbd8e208afe00000000000000000000000000000000000000000000000000000000000f4240', + value: '0x0', + }, + }; + + await addTransaction({ + ...sendRequest, + transactionOptions: { + ...TRANSACTION_OPTIONS_MOCK, + type: TransactionType.tokenMethodTransfer, + }, + securityAlertsEnabled: true, + chainId: '0x1', + }); + + expect( + request.transactionController.addTransaction, + ).toHaveBeenCalledTimes(1); + + expect(validateRequestWithPPOMMock).toHaveBeenCalledTimes(0); + }); }); describe('adds trust signals', () => { diff --git a/app/scripts/lib/transaction/util.ts b/app/scripts/lib/transaction/util.ts index 6f9f8943eb5f..4a6affd77702 100644 --- a/app/scripts/lib/transaction/util.ts +++ b/app/scripts/lib/transaction/util.ts @@ -37,6 +37,7 @@ import { GetAddressSecurityAlertResponse, ScanAddressResponse, } from '../../../../shared/lib/trust-signals'; +import { getTransactionDataRecipient } from '../../../../shared/modules/transaction.utils'; export type AddTransactionOptions = NonNullable< Parameters[1] @@ -72,6 +73,12 @@ export type AddDappTransactionRequest = BaseAddTransactionRequest & { dappRequest: Record; }; +const TRANSFER_TYPES = [ + TransactionType.tokenMethodTransfer, + TransactionType.tokenMethodTransferFrom, + TransactionType.tokenMethodSafeTransferFrom, +]; + export async function addDappTransaction( request: AddDappTransactionRequest, ): Promise { @@ -299,6 +306,7 @@ async function validateSecurity(request: AddTransactionRequest) { scanAddressForTrustSignals(request); const { type } = transactionOptions; + const { data, value, to } = transactionParams; const typeIsExcludedFromPPOM = SECURITY_PROVIDER_EXCLUDED_TRANSACTION_TYPES.includes( @@ -309,17 +317,21 @@ async function validateSecurity(request: AddTransactionRequest) { return; } + const isTransfer = + value === '0x0' && TRANSFER_TYPES.includes(type as TransactionType); + + const recipient = + isTransfer && data ? getTransactionDataRecipient(data) : undefined; + if ( - internalAccounts.some( - ({ address }) => - address.toLowerCase() === transactionParams.to?.toLowerCase(), - ) + isInternalAccount(internalAccounts, to) || + isInternalAccount(internalAccounts, recipient) ) { return; } try { - const { from, to, value, data } = transactionParams; + const { from } = transactionParams; const { actionId, origin } = transactionOptions; const ppomRequest = { @@ -367,3 +379,23 @@ export function stripSingleLeadingZero(hex: string): string { } return `0x${hex.slice(3)}`; } + +function normalizeAddress(address?: string): string | undefined { + return address?.toLowerCase(); +} + +function isInternalAccount( + internalAccounts: { address: string }[], + address?: string, +): boolean { + const normalized = normalizeAddress(address); + if (!normalized) { + return false; + } + + const internalSet = new Set( + internalAccounts.map((acc) => normalizeAddress(acc.address)), + ); + + return internalSet.has(normalized); +} diff --git a/shared/modules/transaction.utils.test.js b/shared/modules/transaction.utils.test.js index 521b1d7709e7..83b3127a313e 100644 --- a/shared/modules/transaction.utils.test.js +++ b/shared/modules/transaction.utils.test.js @@ -10,6 +10,7 @@ import { import { buildSetApproveForAllTransactionData } from '../../test/data/confirmations/set-approval-for-all'; import { determineTransactionType, + getTransactionDataRecipient, hasTransactionData, isEIP1559Transaction, isLegacyTransaction, @@ -54,13 +55,9 @@ describe('Transaction.utils', function () { ); expect(result.name).toBe('approve'); - expect(result.args).toStrictEqual( - expect.objectContaining({ - token: ADDRESS_MOCK, - spender: ADDRESS_2_MOCK, - expiration: EXPIRATION_MOCK, - }), - ); + expect(result.args.token).toBe(ADDRESS_MOCK); + expect(result.args.spender).toBe(ADDRESS_2_MOCK); + expect(result.args.expiration).toBe(EXPIRATION_MOCK); expect(result.args.amount.toString()).toBe(AMOUNT_MOCK.toString()); }); }); @@ -619,4 +616,72 @@ describe('Transaction.utils', function () { }); }); }); + + describe('getTransactionDataRecipient', () => { + const RECIPIENT_ADDRESS = '0x50A9D56C2B8BA9A5c7f2C08C3d26E0499F23a706'; + it('returns recipient address from standard ERC20 transfer', () => { + // Standard transfer(address _to, uint256 _value) + const data = + '0xa9059cbb00000000000000000000000050a9d56c2b8ba9a5c7f2c08c3d26e0499f23a7060000000000000000000000000000000000000000000000000000000000004e20'; + + const result = getTransactionDataRecipient(data); + + expect(result).toBe(RECIPIENT_ADDRESS); + }); + + it('returns undefined for empty data', () => { + const result = getTransactionDataRecipient(''); + + expect(result).toBeUndefined(); + }); + + it('returns undefined for 0x', () => { + const result = getTransactionDataRecipient('0x'); + + expect(result).toBeUndefined(); + }); + + it('returns undefined for non-transfer function data', () => { + // approve(address spender, uint256 amount) + const data = + '0x095ea7b3' + + '00000000000000000000000050a9d56c2b8ba9a5c7f2c08c3d26e0499f23a706' + + '0000000000000000000000000000000000000000000000000000000000004e20'; + + const result = getTransactionDataRecipient(data); + + expect(result).toBeUndefined(); + }); + + it('returns undefined for invalid hex data', () => { + const result = getTransactionDataRecipient('0xinvalid'); + + expect(result).toBeUndefined(); + }); + + it('returns undefined for too short data', () => { + const result = getTransactionDataRecipient('0x12345678'); + + expect(result).toBeUndefined(); + }); + + it('returns undefined for unrecognized function selector', () => { + const data = + '0xffffffff' + // unknown function selector + '00000000000000000000000050a9d56c2b8ba9a5c7f2c08c3d26e0499f23a706' + + '0000000000000000000000000000000000000000000000000000000000004e20'; + + const result = getTransactionDataRecipient(data); + + expect(result).toBeUndefined(); + }); + + it('handles malformed transaction data gracefully', () => { + const data = '0xa9059cbb123'; // incomplete data + + const result = getTransactionDataRecipient(data); + + expect(result).toBeUndefined(); + }); + }); }); diff --git a/shared/modules/transaction.utils.ts b/shared/modules/transaction.utils.ts index 7de8d4a1e30e..a22587c21b85 100644 --- a/shared/modules/transaction.utils.ts +++ b/shared/modules/transaction.utils.ts @@ -391,3 +391,20 @@ export function parseApprovalTransactionData(data: Hex): spender, }; } + +/** + * Extracts the recipient address from a transaction's data field. + * This function parses standard token transaction data and attempts to retrieve + * the recipient address from the transaction arguments. It checks for both `_to` + * and `to` argument patterns commonly found in token transfer transactions. + * + * @param data - The hexadecimal string representation of the transaction data to parse + * @returns The recipient address as a string if found in the transaction data, or undefined if not present + */ +export function getTransactionDataRecipient(data: string): string | undefined { + const transactionData = parseStandardTokenTransactionData(data); + + const transferTo = transactionData?.args?._to || transactionData?.args?.to; + + return transferTo; +}