From 7aff0444bf9f18f193444a175e469109f0b89d45 Mon Sep 17 00:00:00 2001 From: Derek Brans Date: Tue, 15 Oct 2024 07:26:06 -0400 Subject: [PATCH] fix(TXL-399): Use contract abi to decode the amounts of token balance changes (#4775) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Explanation * Use contract abi's to decode the amounts of token balance changes – instead of the raw output from a balanceOf call. ## References * Fixes: https://github.com/MetaMask/metamask-extension/issues/26521 ## Changelog ### `@metamask/transaction-controller` - **FIXED**: Use contract abi's to decode the amounts of token balance changes – instead of the raw output from a balanceOf call. ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've highlighted breaking changes using the "BREAKING" category above as appropriate - [ ] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes --- .../transaction-controller/jest.config.js | 6 +- .../src/utils/simulation.test.ts | 227 +++++++++++------- .../src/utils/simulation.ts | 73 +++--- 3 files changed, 192 insertions(+), 114 deletions(-) diff --git a/packages/transaction-controller/jest.config.js b/packages/transaction-controller/jest.config.js index a7cd29f672..c618be1554 100644 --- a/packages/transaction-controller/jest.config.js +++ b/packages/transaction-controller/jest.config.js @@ -18,9 +18,9 @@ module.exports = merge(baseConfig, { coverageThreshold: { global: { branches: 94.42, - functions: 97.46, - lines: 98.44, - statements: 98.46, + functions: 97.45, + lines: 98.37, + statements: 98.38, }, }, diff --git a/packages/transaction-controller/src/utils/simulation.test.ts b/packages/transaction-controller/src/utils/simulation.test.ts index b07c3a75ea..1df0a186e8 100644 --- a/packages/transaction-controller/src/utils/simulation.test.ts +++ b/packages/transaction-controller/src/utils/simulation.test.ts @@ -1,5 +1,6 @@ import type { LogDescription } from '@ethersproject/abi'; import { Interface } from '@ethersproject/abi'; +import { type Hex } from '@metamask/utils'; import { SimulationInvalidResponseError, @@ -11,7 +12,10 @@ import { SupportedToken, type GetSimulationDataRequest, } from './simulation'; -import type { SimulationResponseLog } from './simulation-api'; +import type { + SimulationResponseLog, + SimulationResponseTransaction, +} from './simulation-api'; import { simulateTransactions, type SimulationResponse, @@ -19,19 +23,37 @@ import { jest.mock('./simulation-api'); -const USER_ADDRESS_MOCK = '0x123'; -const OTHER_ADDRESS_MOCK = '0x456'; -const CONTRACT_ADDRESS_1_MOCK = '0x789'; -const CONTRACT_ADDRESS_2_MOCK = '0xDEF'; -const BALANCE_1_MOCK = '0x0'; -const BALANCE_2_MOCK = '0x1'; -const DIFFERENCE_MOCK = '0x1'; -const VALUE_MOCK = '0x4'; -const TOKEN_ID_MOCK = '0x5'; -const OTHER_TOKEN_ID_MOCK = '0x6'; +// Utility function to encode addresses and values to 32-byte ABI format +const encodeTo32ByteHex = (value: string | number): Hex => { + // Pad to 32 bytes (64 characters) and add '0x' prefix + return `0x${BigInt(value).toString(16).padStart(64, '0')}`; +}; + +// getSimulationData returns values in hex format with leading zeros trimmed. +const trimLeadingZeros = (hexString: Hex): Hex => { + const trimmed = hexString.replace(/^0x0+/u, '0x') as Hex; + return trimmed === '0x' ? '0x0' : trimmed; +}; + +const USER_ADDRESS_MOCK = '0x1233333333333333333333333333333333333333' as Hex; +const OTHER_ADDRESS_MOCK = '0x4566666666666666666666666666666666666666' as Hex; +const CONTRACT_ADDRESS_1_MOCK = + '0x7899999999999999999999999999999999999999' as Hex; +const CONTRACT_ADDRESS_2_MOCK = + '0xDEFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF' as Hex; +const BALANCE_1_MOCK = '0x0' as Hex; +const BALANCE_2_MOCK = '0x1' as Hex; +const DIFFERENCE_MOCK = '0x1' as Hex; +const VALUE_MOCK = '0x4' as Hex; +const TOKEN_ID_MOCK = '0x5' as Hex; +const OTHER_TOKEN_ID_MOCK = '0x6' as Hex; const ERROR_CODE_MOCK = 123; const ERROR_MESSAGE_MOCK = 'Test Error'; +// Regression test – leading zero in user address +const USER_ADDRESS_WITH_LEADING_ZERO = + '0x0012333333333333333333333333333333333333' as Hex; + const REQUEST_MOCK: GetSimulationDataRequest = { chainId: '0x1', from: USER_ADDRESS_MOCK, @@ -87,10 +109,16 @@ const PARSED_WRAPPED_ERC20_DEPOSIT_EVENT_MOCK = { args: [USER_ADDRESS_MOCK, { toHexString: () => VALUE_MOCK }], } as unknown as LogDescription; +const defaultResponseTx: SimulationResponseTransaction = { + return: encodeTo32ByteHex('0x0'), + callTrace: { calls: [], logs: [] }, + stateDiff: { pre: {}, post: {} }, +}; + const RESPONSE_NESTED_LOGS_MOCK: SimulationResponse = { transactions: [ { - return: '0x1', + ...defaultResponseTx, callTrace: { calls: [ { @@ -105,10 +133,6 @@ const RESPONSE_NESTED_LOGS_MOCK: SimulationResponse = { ], logs: [], }, - stateDiff: { - pre: {}, - post: {}, - }, }, ], }; @@ -129,22 +153,12 @@ function createLogMock(contractAddress: string) { * @param logs - The logs. * @returns Mock API response. */ -function createEventResponseMock(logs: SimulationResponseLog[]) { +function createEventResponseMock( + logs: SimulationResponseLog[], +): SimulationResponse { return { - transactions: [ - { - return: '0x', - callTrace: { - calls: [], - logs, - }, - stateDiff: { - pre: {}, - post: {}, - }, - }, - ], - } as unknown as SimulationResponse; + transactions: [{ ...defaultResponseTx, callTrace: { calls: [], logs } }], + }; } /** @@ -160,20 +174,14 @@ function createNativeBalanceResponse( return { transactions: [ { - callTrace: { - calls: [], - logs: [], - }, + ...defaultResponseTx, + return: encodeTo32ByteHex(previousBalance), stateDiff: { pre: { - [USER_ADDRESS_MOCK]: { - balance: previousBalance, - }, + [USER_ADDRESS_MOCK]: { balance: previousBalance }, }, post: { - [USER_ADDRESS_MOCK]: { - balance: newBalance, - }, + [USER_ADDRESS_MOCK]: { balance: newBalance }, }, }, }, @@ -194,37 +202,13 @@ function createBalanceOfResponse( return { transactions: [ ...previousBalances.map((previousBalance) => ({ - return: previousBalance, - callTrace: { - calls: [], - logs: [], - }, - stateDiff: { - pre: {}, - post: {}, - }, + ...defaultResponseTx, + return: encodeTo32ByteHex(previousBalance), })), - { - return: '0xabc', - callTrace: { - calls: [], - logs: [], - }, - stateDiff: { - pre: {}, - post: {}, - }, - }, + defaultResponseTx, ...newBalances.map((newBalance) => ({ - return: newBalance, - callTrace: { - calls: [], - logs: [], - }, - stateDiff: { - pre: {}, - post: {}, - }, + ...defaultResponseTx, + return: encodeTo32ByteHex(newBalance), })), ], } as unknown as SimulationResponse; @@ -317,6 +301,7 @@ describe('Simulation Utils', () => { it.each([ { title: 'ERC-20 token', + from: USER_ADDRESS_MOCK, parsedEvent: PARSED_ERC20_TRANSFER_EVENT_MOCK, tokenType: SupportedToken.ERC20, tokenStandard: SimulationTokenStandard.erc20, @@ -326,6 +311,7 @@ describe('Simulation Utils', () => { }, { title: 'ERC-721 token', + from: USER_ADDRESS_MOCK, parsedEvent: PARSED_ERC721_TRANSFER_EVENT_MOCK, tokenType: SupportedToken.ERC721, tokenStandard: SimulationTokenStandard.erc721, @@ -333,8 +319,27 @@ describe('Simulation Utils', () => { previousBalances: [OTHER_ADDRESS_MOCK], newBalances: [USER_ADDRESS_MOCK], }, + { + // Regression test – leading zero in user address + title: 'ERC-721 token – where user address has leadding zero', + from: USER_ADDRESS_WITH_LEADING_ZERO, + parsedEvent: { + ...PARSED_ERC721_TRANSFER_EVENT_MOCK, + args: [ + OTHER_ADDRESS_MOCK, + USER_ADDRESS_WITH_LEADING_ZERO, + { toHexString: () => TOKEN_ID_MOCK }, + ], + }, + tokenType: SupportedToken.ERC721, + tokenStandard: SimulationTokenStandard.erc721, + tokenId: TOKEN_ID_MOCK, + previousBalances: [OTHER_ADDRESS_MOCK], + newBalances: [USER_ADDRESS_WITH_LEADING_ZERO], + }, { title: 'ERC-1155 token via single event', + from: USER_ADDRESS_MOCK, parsedEvent: PARSED_ERC1155_TRANSFER_SINGLE_EVENT_MOCK, tokenType: SupportedToken.ERC1155, tokenStandard: SimulationTokenStandard.erc1155, @@ -344,6 +349,7 @@ describe('Simulation Utils', () => { }, { title: 'ERC-1155 token via batch event', + from: USER_ADDRESS_MOCK, parsedEvent: PARSED_ERC1155_TRANSFER_BATCH_EVENT_MOCK, tokenType: SupportedToken.ERC1155, tokenStandard: SimulationTokenStandard.erc1155, @@ -353,6 +359,7 @@ describe('Simulation Utils', () => { }, { title: 'wrapped ERC-20 token', + from: USER_ADDRESS_MOCK, parsedEvent: PARSED_WRAPPED_ERC20_DEPOSIT_EVENT_MOCK, tokenType: SupportedToken.ERC20_WRAPPED, tokenStandard: SimulationTokenStandard.erc20, @@ -362,6 +369,7 @@ describe('Simulation Utils', () => { }, { title: 'legacy ERC-721 token', + from: USER_ADDRESS_MOCK, parsedEvent: PARSED_ERC721_TRANSFER_EVENT_MOCK, tokenType: SupportedToken.ERC721_LEGACY, tokenStandard: SimulationTokenStandard.erc721, @@ -372,6 +380,7 @@ describe('Simulation Utils', () => { ])( 'on $title', async ({ + from, parsedEvent, tokenStandard, tokenType, @@ -389,7 +398,10 @@ describe('Simulation Utils', () => { createBalanceOfResponse(previousBalances, newBalances), ); - const simulationData = await getSimulationData(REQUEST_MOCK); + const simulationData = await getSimulationData({ + chainId: '0x1', + from, + }); expect(simulationData).toStrictEqual({ nativeBalanceChange: undefined, @@ -398,8 +410,8 @@ describe('Simulation Utils', () => { standard: tokenStandard, address: CONTRACT_ADDRESS_1_MOCK, id: tokenId, - previousBalance: BALANCE_1_MOCK, - newBalance: BALANCE_2_MOCK, + previousBalance: trimLeadingZeros(BALANCE_1_MOCK), + newBalance: trimLeadingZeros(BALANCE_2_MOCK), difference: DIFFERENCE_MOCK, isDecrease: false, }, @@ -501,8 +513,8 @@ describe('Simulation Utils', () => { standard: SimulationTokenStandard.erc20, address: CONTRACT_ADDRESS_1_MOCK, id: undefined, - previousBalance: BALANCE_2_MOCK, - newBalance: BALANCE_1_MOCK, + previousBalance: trimLeadingZeros(BALANCE_2_MOCK), + newBalance: trimLeadingZeros(BALANCE_1_MOCK), difference: DIFFERENCE_MOCK, isDecrease: true, }, @@ -545,8 +557,8 @@ describe('Simulation Utils', () => { standard: SimulationTokenStandard.erc721, address: CONTRACT_ADDRESS_1_MOCK, id: TOKEN_ID_MOCK, - previousBalance: BALANCE_1_MOCK, - newBalance: BALANCE_2_MOCK, + previousBalance: trimLeadingZeros(BALANCE_1_MOCK), + newBalance: trimLeadingZeros(BALANCE_2_MOCK), difference: DIFFERENCE_MOCK, isDecrease: false, }, @@ -554,8 +566,8 @@ describe('Simulation Utils', () => { standard: SimulationTokenStandard.erc721, address: CONTRACT_ADDRESS_1_MOCK, id: OTHER_TOKEN_ID_MOCK, - previousBalance: BALANCE_1_MOCK, - newBalance: BALANCE_2_MOCK, + previousBalance: trimLeadingZeros(BALANCE_1_MOCK), + newBalance: trimLeadingZeros(BALANCE_2_MOCK), difference: DIFFERENCE_MOCK, isDecrease: false, }, @@ -738,9 +750,60 @@ describe('Simulation Utils', () => { standard: SimulationTokenStandard.erc20, address: CONTRACT_ADDRESS_1_MOCK, id: undefined, - previousBalance: BALANCE_1_MOCK, - newBalance: BALANCE_2_MOCK, - difference: DIFFERENCE_MOCK, + previousBalance: trimLeadingZeros(BALANCE_1_MOCK), + newBalance: trimLeadingZeros(BALANCE_2_MOCK), + difference: '0x1', + isDecrease: false, + }, + ], + }); + }); + + // Ensures no regression of bug https://github.com/MetaMask/metamask-extension/issues/26521 + it('decodes raw balanceOf output correctly for ERC20 token with extra zeros', async () => { + const DECODED_BALANCE_BEFORE = '0x134c31d252'; + const DECODED_BALANCE_AFTER = '0x134c31d257'; + const EXPECTED_BALANCE_CHANGE = '0x5'; + + // Contract returns 64 extra zeros in raw output of balanceOf. + // Abi decoding should ignore them. + const encodeOutputWith64ExtraZeros = (value: string) => + (encodeTo32ByteHex(value) + ''.padStart(64, '0')) as Hex; + const RAW_BALANCE_BEFORE = encodeOutputWith64ExtraZeros( + DECODED_BALANCE_BEFORE, + ); + const RAW_BALANCE_AFTER = encodeOutputWith64ExtraZeros( + DECODED_BALANCE_AFTER, + ); + + mockParseLog({ + erc20: PARSED_ERC20_TRANSFER_EVENT_MOCK, + }); + + simulateTransactionsMock + .mockResolvedValueOnce( + createEventResponseMock([createLogMock(CONTRACT_ADDRESS_2_MOCK)]), + ) + .mockResolvedValueOnce({ + transactions: [ + { ...defaultResponseTx, return: RAW_BALANCE_BEFORE }, + defaultResponseTx, + { ...defaultResponseTx, return: RAW_BALANCE_AFTER }, + ], + }); + + const simulationData = await getSimulationData(REQUEST_MOCK); + + expect(simulationData).toStrictEqual({ + nativeBalanceChange: undefined, + tokenBalanceChanges: [ + { + standard: SimulationTokenStandard.erc20, + address: CONTRACT_ADDRESS_2_MOCK, + id: undefined, + previousBalance: DECODED_BALANCE_BEFORE, + newBalance: DECODED_BALANCE_AFTER, + difference: EXPECTED_BALANCE_CHANGE, isDecrease: false, }, ], diff --git a/packages/transaction-controller/src/utils/simulation.ts b/packages/transaction-controller/src/utils/simulation.ts index 04ea5f66f4..cb6ff73f06 100644 --- a/packages/transaction-controller/src/utils/simulation.ts +++ b/packages/transaction-controller/src/utils/simulation.ts @@ -332,14 +332,14 @@ async function getTokenBalanceChanges( const previousBalanceCheckSkipped = !balanceTxs.before.get(token); const previousBalance = previousBalanceCheckSkipped ? '0x0' - : getValueFromBalanceTransaction( + : getAmountFromBalanceTransactionResult( request.from, token, // eslint-disable-next-line no-plusplus response.transactions[prevBalanceTxIndex++], ); - const newBalance = getValueFromBalanceTransaction( + const newBalance = getAmountFromBalanceTransactionResult( request.from, token, response.transactions[index + balanceTxs.before.size + 1], @@ -477,24 +477,54 @@ function getEventTokenIds(event: ParsedEvent): (Hex | undefined)[] { } /** - * Extract the value from a balance transaction response. + * Get the interface for a token standard. + * @param tokenStandard - The token standard. + * @returns The interface for the token standard. + */ +function getContractInterface( + tokenStandard: SimulationTokenStandard, +): Interface { + switch (tokenStandard) { + case SimulationTokenStandard.erc721: + return new Interface(abiERC721); + case SimulationTokenStandard.erc1155: + return new Interface(abiERC1155); + default: + return new Interface(abiERC20); + } +} + +/** + * Extract the value from a balance transaction response using the correct ABI. * @param from - The address to check the balance of. * @param token - The token to check the balance of. * @param response - The balance transaction response. - * @returns The value of the balance transaction. + * @returns The value of the balance transaction as Hex. */ -function getValueFromBalanceTransaction( +function getAmountFromBalanceTransactionResult( from: Hex, token: SimulationToken, response: SimulationResponseTransaction, ): Hex { - const normalizedReturn = normalizeReturnValue(response.return); + const contract = getContractInterface(token.standard); - if (token.standard === SimulationTokenStandard.erc721) { - return normalizedReturn === from ? '0x1' : '0x0'; - } + try { + if (token.standard === SimulationTokenStandard.erc721) { + const result = contract.decodeFunctionResult('ownerOf', response.return); + const owner = result[0]; + return owner.toLowerCase() === from.toLowerCase() ? '0x1' : '0x0'; + } - return normalizedReturn; + const result = contract.decodeFunctionResult('balanceOf', response.return); + return toHex(result[0]); + } catch (error) { + log('Failed to decode balance transaction', error, { token, response }); + throw new SimulationError( + `Failed to decode balance transaction for token ${ + token.address + }: ${String(error)}`, + ); + } } /** @@ -509,22 +539,16 @@ function getBalanceTransactionData( from: Hex, tokenId?: Hex, ): Hex { + const contract = getContractInterface(tokenStandard); switch (tokenStandard) { case SimulationTokenStandard.erc721: - return new Interface(abiERC721).encodeFunctionData('ownerOf', [ - tokenId, - ]) as Hex; + return contract.encodeFunctionData('ownerOf', [tokenId]) as Hex; case SimulationTokenStandard.erc1155: - return new Interface(abiERC1155).encodeFunctionData('balanceOf', [ - from, - tokenId, - ]) as Hex; + return contract.encodeFunctionData('balanceOf', [from, tokenId]) as Hex; default: - return new Interface(abiERC20).encodeFunctionData('balanceOf', [ - from, - ]) as Hex; + return contract.encodeFunctionData('balanceOf', [from]) as Hex; } } @@ -607,15 +631,6 @@ function getSimulationBalanceChange( }; } -/** - * Normalize a return value. - * @param value - The return value to normalize. - * @returns The normalized return value. - */ -function normalizeReturnValue(value: Hex): Hex { - return toHex(hexToBN(value)); -} - /** * Get the contract interfaces for all supported tokens. * @returns A map of supported tokens to their contract interfaces.