fix: incorrect method name parsed from transaction data (#27363)

Method data derived for the metrics is only persisted if a result is found. Queries to 4Byte are skipped if the transaction data is `0x` or less than four bytes. Transaction data is not rendered in the legacy or redesigned confirmations if it is `0x`. The `getKnownMethodData` selector only returns a value if transaction data is at least four bytes. The unused `useKnownMethodDataInTransaction` hook was removed.
MetaMask · Sep 25, 2024 · fce2e8b · fce2e8b
1 parent 470420f
commit fce2e8b
Show file tree

Hide file tree

Showing 17 changed files with 139 additions and 145 deletions.
diff --git a/app/scripts/lib/util.test.js b/app/scripts/lib/util.test.js
@@ -369,21 +369,25 @@ describe('app utils', () => {
         name: 'Approve Tokens',
       },
     };
+
     it('return null if use4ByteResolution is not true', async () => {
       expect(
         await getMethodDataName(knownMethodData, false, '0x60806040'),
       ).toStrictEqual(null);
     });
+
     it('return null if prefixedData is not defined', async () => {
       expect(
         await getMethodDataName(knownMethodData, true, undefined),
       ).toStrictEqual(null);
     });
+
     it('return details from knownMethodData if defined', async () => {
       expect(
         await getMethodDataName(knownMethodData, true, '0x60806040'),
       ).toStrictEqual(knownMethodData['0x60806040']);
     });
+
     it('invoke getMethodDataAsync if details not available in knownMethodData', async () => {
       const DUMMY_METHOD_NAME = {
         name: 'Dummy Method Name',
@@ -392,9 +396,10 @@ describe('app utils', () => {
         .spyOn(FourBiteUtils, 'getMethodDataAsync')
         .mockResolvedValue(DUMMY_METHOD_NAME);
       expect(
-        await getMethodDataName(knownMethodData, true, '0x123'),
+        await getMethodDataName(knownMethodData, true, '0x123', jest.fn()),
       ).toStrictEqual(DUMMY_METHOD_NAME);
     });
+
     it('invoke addKnownMethodData if details not available in knownMethodData', async () => {
       const DUMMY_METHOD_NAME = {
         name: 'Dummy Method Name',
@@ -413,5 +418,22 @@ describe('app utils', () => {
       ).toStrictEqual(DUMMY_METHOD_NAME);
       expect(addKnownMethodData).toHaveBeenCalledTimes(1);
     });
+
+    it('does not invoke addKnownMethodData if no method data available', async () => {
+      const addKnownMethodData = jest.fn();
+
+      jest.spyOn(FourBiteUtils, 'getMethodDataAsync').mockResolvedValue({});
+
+      expect(
+        await getMethodDataName(
+          knownMethodData,
+          true,
+          '0x123',
+          addKnownMethodData,
+        ),
+      ).toStrictEqual({});
+
+      expect(addKnownMethodData).toHaveBeenCalledTimes(0);
+    });
   });
 });
diff --git a/app/scripts/lib/util.ts b/app/scripts/lib/util.ts
@@ -411,7 +411,7 @@ export const getMethodDataName = async (
     provider,
   );
 
-  if (addKnownMethodData) {
+  if (methodData?.name) {
     addKnownMethodData(fourBytePrefix, methodData as MethodData);
   }
 

diff --git a/shared/lib/four-byte.test.ts b/shared/lib/four-byte.test.ts
@@ -25,6 +25,25 @@ describe('Four Byte', () => {
 
       expect(result).toStrictEqual('someOtherFunction(address,uint256)');
     });
+
+    // @ts-expect-error This is missing from the Mocha type definitions
+    it.each([undefined, null, '', '0x', '0X'])(
+      'returns undefined if four byte prefix is %s',
+      async (prefix: string) => {
+        expect(await getMethodFrom4Byte(prefix)).toBeUndefined();
+      },
+    );
+
+    // @ts-expect-error This is missing from the Mocha type definitions
+    it.each([
+      ['with hex prefix', '0x1234567'],
+      ['without hex prefix', '1234567'],
+    ])(
+      'returns undefined if length of four byte prefix %s is less than 8',
+      async (_: string, prefix: string) => {
+        expect(await getMethodFrom4Byte(prefix)).toBeUndefined();
+      },
+    );
   });
 
   describe('getMethodDataAsync', () => {

diff --git a/shared/lib/four-byte.ts b/shared/lib/four-byte.ts
@@ -1,4 +1,7 @@
 import { MethodRegistry } from 'eth-method-registry';
+import { Hex } from '@metamask/utils';
+import { hasTransactionData } from '../modules/transaction.utils';
+import { stripHexPrefix } from '../modules/hexstring-utils';
 import fetchWithCache from './fetch-with-cache';
 
 type FourByteResult = {
@@ -12,7 +15,14 @@ type FourByteResponse = {
 
 export async function getMethodFrom4Byte(
   fourBytePrefix: string,
-): Promise<string> {
+): Promise<string | undefined> {
+  if (
+    !hasTransactionData(fourBytePrefix as Hex) ||
+    stripHexPrefix(fourBytePrefix)?.length < 8
+  ) {
+    return undefined;
+  }
+
   const fourByteResponse = (await fetchWithCache({
     url: `https://www.4byte.directory/api/v1/signatures/?hex_signature=${fourBytePrefix}`,
     fetchOptions: {

diff --git a/shared/modules/transaction.utils.test.js b/shared/modules/transaction.utils.test.js
@@ -4,6 +4,7 @@ import { TransactionType } from '@metamask/transaction-controller';
 import { createTestProviderTools } from '../../test/stub/provider';
 import {
   determineTransactionType,
+  hasTransactionData,
   isEIP1559Transaction,
   isLegacyTransaction,
   parseStandardTokenTransactionData,
@@ -417,4 +418,20 @@ describe('Transaction.utils', function () {
       });
     });
   });
+
+  describe('hasTransactionData', () => {
+    it.each([
+      ['has prefix', '0x1234'],
+      ['has no prefix', '1234'],
+    ])('returns true if data %s', (_, data) => {
+      expect(hasTransactionData(data)).toBe(true);
+    });
+
+    it.each([undefined, null, '', '0x', '0X'])(
+      'returns false if data is %s',
+      (data) => {
+        expect(hasTransactionData(data)).toBe(false);
+      },
+    );
+  });
 });
diff --git a/shared/modules/transaction.utils.ts b/shared/modules/transaction.utils.ts
@@ -14,6 +14,7 @@ import {
 } from '@metamask/transaction-controller';
 import type { TransactionParams } from '@metamask/transaction-controller';
 
+import { Hex } from '@metamask/utils';
 import { AssetType, TokenStandard } from '../constants/transaction';
 import { readAddressAsContract } from './contract-utils';
 import { isEqualCaseInsensitive } from './string-utils';
@@ -319,3 +320,9 @@ export const parseTypedDataMessage = (dataToParse: string) => {
 
   return result;
 };
+
+export function hasTransactionData(transactionData?: Hex): boolean {
+  return Boolean(
+    transactionData?.length && transactionData?.toLowerCase?.() !== '0x',
+  );
+}
diff --git a/ui/pages/confirmations/components/confirm-hexdata/confirm-hexdata.js b/ui/pages/confirmations/components/confirm-hexdata/confirm-hexdata.js
@@ -15,6 +15,7 @@ import {
 import Box from '../../../../components/ui/box';
 import { Text } from '../../../../components/component-library';
 import CopyRawData from '../transaction-decoding/components/ui/copy-raw-data';
+import { hasTransactionData } from '../../../../../shared/modules/transaction.utils';
 
 const ConfirmHexData = ({ txData, dataHexComponent }) => {
   const t = useI18nContext();
@@ -28,7 +29,7 @@ const ConfirmHexData = ({ txData, dataHexComponent }) => {
     return dataHexComponent;
   }
 
-  if (!txParams.data || !txParams.to) {
+  if (!hasTransactionData(txParams.data) || !txParams.to) {
     return null;
   }
 

diff --git a/ui/pages/confirmations/components/confirm-hexdata/confirm-hexdata.test.js b/ui/pages/confirmations/components/confirm-hexdata/confirm-hexdata.test.js
@@ -28,21 +28,24 @@ describe('ConfirmHexData', () => {
     expect(await findByText('Transfer')).toBeInTheDocument();
   });
 
-  it('should return null if transaction has no data', () => {
-    const { container } = renderWithProvider(
-      <ConfirmHexData
-        txData={{
-          txParams: {
-            data: '0x608060405234801',
-          },
-          origin: 'https://metamask.github.io',
-          type: 'transfer',
-        }}
-      />,
-      store,
-    );
-    expect(container.firstChild).toStrictEqual(null);
-  });
+  it.each([undefined, null, '', '0x', '0X'])(
+    'should return null if transaction data is %s',
+    (data) => {
+      const { container } = renderWithProvider(
+        <ConfirmHexData
+          txData={{
+            txParams: {
+              data,
+            },
+            origin: 'https://metamask.github.io',
+            type: 'transfer',
+          }}
+        />,
+        store,
+      );
+      expect(container.firstChild).toStrictEqual(null);
+    },
+  );
 
   it('should return null if transaction has no to address', () => {
     const { container } = renderWithProvider(

diff --git a/ui/pages/confirmations/components/confirm/info/hooks/useDecodedTransactionData.test.ts b/ui/pages/confirmations/components/confirm/info/hooks/useDecodedTransactionData.test.ts
@@ -38,22 +38,26 @@ async function runHook(state: Record<string, unknown>) {
 describe('useDecodedTransactionData', () => {
   const decodeTransactionDataMock = jest.mocked(decodeTransactionData);
 
-  it('returns undefined if no transaction data', async () => {
-    const result = await runHook(
-      getMockConfirmStateForTransaction({
-        id: '123',
-        chainId: CHAIN_ID_MOCK,
-        type: TransactionType.contractInteraction,
-        status: TransactionStatus.unapproved,
-        txParams: {
-          data: '',
-          to: CONTRACT_ADDRESS_MOCK,
-        } as TransactionParams,
-      }),
-    );
+  // @ts-expect-error This is missing from the Mocha type definitions
+  it.each([undefined, null, '', '0x', '0X'])(
+    'returns undefined if transaction data is %s',
+    async (data: string) => {
+      const result = await runHook(
+        getMockConfirmStateForTransaction({
+          id: '123',
+          chainId: CHAIN_ID_MOCK,
+          type: TransactionType.contractInteraction,
+          status: TransactionStatus.unapproved,
+          txParams: {
+            data,
+            to: CONTRACT_ADDRESS_MOCK,
+          } as TransactionParams,
+        }),
+      );
 
-    expect(result).toStrictEqual({ pending: false, value: undefined });
-  });
+      expect(result).toStrictEqual({ pending: false, value: undefined });
+    },
+  );
 
   it('returns the decoded data', async () => {
     decodeTransactionDataMock.mockResolvedValue(TRANSACTION_DECODE_SOURCIFY);

diff --git a/ui/pages/confirmations/components/confirm/info/hooks/useDecodedTransactionData.ts b/ui/pages/confirmations/components/confirm/info/hooks/useDecodedTransactionData.ts
@@ -8,6 +8,7 @@ import {
 import { decodeTransactionData } from '../../../../../../store/actions';
 import { DecodedTransactionDataResponse } from '../../../../../../../shared/types/transaction-decode';
 import { useConfirmContext } from '../../../../context/confirm';
+import { hasTransactionData } from '../../../../../../../shared/modules/transaction.utils';
 
 export function useDecodedTransactionData(): AsyncResult<
   DecodedTransactionDataResponse | undefined
@@ -19,7 +20,7 @@ export function useDecodedTransactionData(): AsyncResult<
   const transactionData = currentConfirmation?.txParams?.data as Hex;
 
   return useAsyncResult(async () => {
-    if (!transactionData?.length) {
+    if (!hasTransactionData(transactionData)) {
       return undefined;
     }
 

diff --git a/ui/pages/confirmations/components/confirm/info/hooks/useFourByte.test.ts b/ui/pages/confirmations/components/confirm/info/hooks/useFourByte.test.ts
@@ -54,7 +54,7 @@ describe('useFourByte', () => {
       },
     );
 
-    expect(result.current).toEqual({});
+    expect(result.current).toBeNull();
   });
 
   it("returns undefined if it's not known even if resolution is enabled", () => {
@@ -75,6 +75,6 @@ describe('useFourByte', () => {
       },
     );
 
-    expect(result.current).toEqual({});
+    expect(result.current).toBeNull();
   });
 });
diff --git a/ui/pages/confirmations/components/confirm/info/hooks/useKnownMethodDataInTransaction.test.ts b/ui/pages/confirmations/components/confirm/info/hooks/useKnownMethodDataInTransaction.test.ts