fix (cherry-pick): incorrect method name parsed from transaction data (…

…#27363) (#27379)

## **Description**

Cherry pick of #27363 for version 12.3.0.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27379?quickstart=1)

## **Related issues**

## **Manual testing steps**

## **Screenshots/Recordings**

### **Before**

### **After**

## **Pre-merge author checklist**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

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

app/scripts/lib/util.ts

@@ -411,7 +411,7 @@ export const getMethodDataName = async (
     provider,
   );
 
-  if (addKnownMethodData) {
+  if (methodData?.name) {
     addKnownMethodData(fourBytePrefix, methodData as MethodData);
   }
 

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', () => {

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

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

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

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

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(

ui/pages/confirmations/components/confirm/info/hooks/useDecodedTransactionData.test.ts

@@ -47,19 +47,23 @@ async function runHook(stateOptions?: Parameters<typeof buildState>[0]) {
 describe('useDecodedTransactionData', () => {
   const decodeTransactionDataMock = jest.mocked(decodeTransactionData);
 
-  it('returns undefined if no transaction data', async () => {
-    const result = await runHook({
-      currentConfirmation: {
-        chainId: CHAIN_ID_MOCK,
-        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 () => {
+      const result = await runHook({
+        currentConfirmation: {
+          chainId: CHAIN_ID_MOCK,
+          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);

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 { currentConfirmationSelector } from '../../../../selectors';
+import { hasTransactionData } from '../../../../../../../shared/modules/transaction.utils';
 
 export function useDecodedTransactionData(): AsyncResult<
   DecodedTransactionDataResponse | undefined
@@ -21,7 +22,7 @@ export function useDecodedTransactionData(): AsyncResult<
   const transactionData = currentConfirmation?.txParams?.data as Hex;
 
   return useAsyncResult(async () => {
-    if (!transactionData?.length) {
+    if (!hasTransactionData(transactionData)) {
       return undefined;
     }
 

ui/pages/confirmations/components/confirm/info/hooks/useFourByte.test.ts

@@ -54,7 +54,7 @@ describe('useFourByte', () => {
       },
     );
 
-    expect(result.current).toBeUndefined();
+    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).toBeUndefined();
+    expect(result.current).toBeNull();
   });
 });