fix: add dynamic nft and erc20 check

packages/keyring-eth-hd/package.json

@@ -53,7 +53,7 @@
     "@metamask/scure-bip39": "^2.1.1",
     "@metamask/superstruct": "^3.1.0",
     "@metamask/utils": "^11.10.0",
-    "ethereum-cryptography": "^2.1.2"
+    "ethereum-cryptography": "^2.2.1"
   },
   "devDependencies": {
     "@lavamoat/allow-scripts": "^3.2.1",

packages/keyring-eth-ledger-bridge/CHANGELOG.md

@@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Added
+
+- Add `getTransactionSelector` to read the 4-byte calldata selector from serialized transaction hex (legacy and typed txs) ([#506](https://github.com/MetaMask/accounts/pull/506))
+  - Ledger mobile bridge passes `nft: true` to `clearSignTransaction` when that selector is NFT-only (ERC-721 / ERC-1155).
+- Add `ERC20_WRITE_SELECTORS` for the three EIP-20 write functions (`transfer`, `transferFrom`, `approve`) ([#506](https://github.com/MetaMask/accounts/pull/506))
+
 ## [11.3.1]
 
 ### Changed

packages/keyring-eth-ledger-bridge/jest.config.js

@@ -26,10 +26,10 @@ module.exports = merge(baseConfig, {
   // An object that configures minimum threshold enforcement for coverage results
   coverageThreshold: {
     global: {
-      branches: 93.36,
-      functions: 98.29,
-      lines: 97.86,
-      statements: 97.88,
+      branches: 93.53,
+      functions: 98.3,
+      lines: 97.94,
+      statements: 97.95,
     },
   },
 });

packages/keyring-eth-ledger-bridge/package.json

@@ -77,6 +77,7 @@
     "@types/web": "^0.0.69",
     "deepmerge": "^4.2.2",
     "depcheck": "^1.4.7",
+    "ethereum-cryptography": "^2.2.1",
     "ethereumjs-tx": "^1.3.7",
     "jest": "^29.5.0",
     "jest-it-up": "^3.1.0",

packages/keyring-eth-ledger-bridge/src/constants.test.ts

@@ -0,0 +1,50 @@
+import { keccak256 } from 'ethereum-cryptography/keccak';
+
+import { ERC20_WRITE_SELECTORS, NFT_ONLY_SELECTORS } from './constants';
+
+/**
+ * Computes the four-byte function selector for a canonical Solidity signature.
+ *
+ * @param signature - Canonical Solidity ABI function signature.
+ * @returns Lowercase `0x` + 8-hex-digit selector.
+ */
+function selectorFromSignature(signature: string): string {
+  const hash = keccak256(Buffer.from(signature, 'utf8'));
+  return `0x${Buffer.from(hash).subarray(0, 4).toString('hex')}`;
+}
+
+describe('NFT_ONLY_SELECTORS', () => {
+  const signatures: readonly string[] = [
+    'setApprovalForAll(address,bool)',
+    'safeTransferFrom(address,address,uint256)',
+    'safeTransferFrom(address,address,uint256,bytes)',
+    'safeTransferFrom(address,address,uint256,uint256,bytes)',
+    'safeBatchTransferFrom(address,address,uint256[],uint256[],bytes)',
+  ];
+
+  it('contains exactly one entry per canonical NFT-related signature', () => {
+    expect(NFT_ONLY_SELECTORS.size).toBe(signatures.length);
+    for (const signature of signatures) {
+      expect(NFT_ONLY_SELECTORS.has(selectorFromSignature(signature))).toBe(
+        true,
+      );
+    }
+  });
+});
+
+describe('ERC20_WRITE_SELECTORS', () => {
+  const signatures: readonly string[] = [
+    'transfer(address,uint256)',
+    'transferFrom(address,address,uint256)',
+    'approve(address,uint256)',
+  ];
+
+  it('contains exactly the three EIP-20 write function selectors', () => {
+    expect(ERC20_WRITE_SELECTORS.size).toBe(signatures.length);
+    for (const signature of signatures) {
+      expect(ERC20_WRITE_SELECTORS.has(selectorFromSignature(signature))).toBe(
+        true,
+      );
+    }
+  });
+});

packages/keyring-eth-ledger-bridge/src/constants.ts

@@ -0,0 +1,23 @@
+/**
+ * Selectors that are used only by NFT standards (ERC721/ERC1155), not by ERC20.
+ * When the tx uses one of these, we enable Ledger NFT clear signing.
+ * approve(0x095ea7b3) is shared by ERC20 and ERC721 so we do NOT include it here.
+ */
+export const NFT_ONLY_SELECTORS = new Set([
+  '0xa22cb465', // setApprovalForAll (ERC721 + ERC1155)
+  '0x42842e0e', // safeTransferFrom (ERC721)
+  '0xb88d4fde', // safeTransferFrom(address,address,uint256,bytes) (ERC721)
+  '0xf242432a', // safeTransferFrom (ERC1155)
+  '0x2eb2c2d6', // safeBatchTransferFrom (ERC1155)
+]);
+
+/**
+ * Four-byte selectors for the three state-changing functions defined in EIP-20.
+ *
+ * @see https://eips.ethereum.org/EIPS/eip-20
+ */
+export const ERC20_WRITE_SELECTORS = new Set([
+  '0xa9059cbb', // transfer(address,uint256)
+  '0x23b872dd', // transferFrom(address,address,uint256)
+  '0x095ea7b3', // approve(address,uint256)
+]);

packages/keyring-eth-ledger-bridge/src/index.ts

@@ -8,3 +8,5 @@ export type * from './type';
 export * from './ledger-hw-app';
 export * from './errors';
 export * from './ledger-error-handler';
+export * from './constants';
+export * from './utils';

packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.test.ts

@@ -1,5 +1,9 @@
+import { Common, Chain, Hardfork } from '@ethereumjs/common';
+import { TransactionFactory } from '@ethereumjs/tx';
+import { bytesToHex } from '@ethereumjs/util';
 import Transport from '@ledgerhq/hw-transport';
 import { EIP712Message } from '@ledgerhq/types-live';
+import { remove0x } from '@metamask/utils';
 
 import { MetaMaskLedgerHwAppEth } from './ledger-hw-app';
 import { LedgerMobileBridge } from './ledger-mobile-bridge';
@@ -162,9 +166,67 @@ describe('LedgerMobileBridge', function () {
       });
       expect(transportMiddlewareGetEthAppSpy).toHaveBeenCalledTimes(1);
       expect(mockEthApp.clearSignTransaction).toHaveBeenCalledTimes(1);
+      expect(mockEthApp.clearSignTransaction).toHaveBeenCalledWith(hdPath, tx, {
+        externalPlugins: true,
+        erc20: false,
+        nft: false,
+      });
+    });
+
+    it('sets erc20 when calldata uses an EIP-20 write selector', async function () {
+      const hdPath = "m/44'/60'/0'/0/0";
+      const common = new Common({
+        chain: Chain.Mainnet,
+        hardfork: Hardfork.Berlin,
+      });
+      const erc20Tx = TransactionFactory.fromTxData(
+        {
+          nonce: '0x00',
+          gasPrice: '0x01',
+          gasLimit: '0x5208',
+          to: '0x0000000000000000000000000000000000000000',
+          value: '0x00',
+          data: '0xa9059cbb0000000000000000000000000000000000000000000000000000000000000000',
+        },
+        { common },
+      );
+      const tx = remove0x(bytesToHex(erc20Tx.serialize()));
+      await bridge.deviceSignTransaction({
+        hdPath,
+        tx,
+      });
       expect(mockEthApp.clearSignTransaction).toHaveBeenCalledWith(hdPath, tx, {
         externalPlugins: true,
         erc20: true,
+        nft: false,
+      });
+    });
+
+    it('sets nft when calldata uses an NFT-only selector', async function () {
+      const hdPath = "m/44'/60'/0'/0/0";
+      const common = new Common({
+        chain: Chain.Mainnet,
+        hardfork: Hardfork.Berlin,
+      });
+      const nftTx = TransactionFactory.fromTxData(
+        {
+          nonce: '0x00',
+          gasPrice: '0x01',
+          gasLimit: '0x5208',
+          to: '0x0000000000000000000000000000000000000000',
+          value: '0x00',
+          data: '0xa22cb46500000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001',
+        },
+        { common },
+      );
+      const tx = remove0x(bytesToHex(nftTx.serialize()));
+      await bridge.deviceSignTransaction({
+        hdPath,
+        tx,
+      });
+      expect(mockEthApp.clearSignTransaction).toHaveBeenCalledWith(hdPath, tx, {
+        externalPlugins: true,
+        erc20: false,
         nft: true,
       });
     });
@@ -182,8 +244,8 @@ describe('LedgerMobileBridge', function () {
       expect(mockEthApp.clearSignTransaction).toHaveBeenCalledTimes(1);
       expect(mockEthApp.clearSignTransaction).toHaveBeenCalledWith(hdPath, tx, {
         externalPlugins: true,
-        erc20: true,
-        nft: true,
+        erc20: false,
+        nft: false,
       });
     });
   });

packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.ts

@@ -1,5 +1,6 @@
 import type Transport from '@ledgerhq/hw-transport';
 
+import { ERC20_WRITE_SELECTORS, NFT_ONLY_SELECTORS } from './constants';
 import {
   AppConfigurationResponse,
   GetAppNameAndVersionResponse,
@@ -16,6 +17,7 @@ import {
 import { MetaMaskLedgerHwAppEth } from './ledger-hw-app';
 import { TransportMiddleware } from './ledger-transport-middleware';
 import { LedgerMobileBridgeOptions } from './type';
+import { getTransactionSelector } from './utils';
 
 // MobileBridge Type will always use LedgerBridge with LedgerMobileBridgeOptions
 export type MobileBridge = LedgerBridge<LedgerMobileBridgeOptions> & {
@@ -110,10 +112,14 @@ export class LedgerMobileBridge implements MobileBridge {
     tx,
     hdPath,
   }: LedgerSignTransactionParams): Promise<LedgerSignTransactionResponse> {
+    const selector = getTransactionSelector(tx);
+    const nft = Boolean(selector && NFT_ONLY_SELECTORS.has(selector));
+    const erc20 = Boolean(selector && ERC20_WRITE_SELECTORS.has(selector));
+
     return this.#getEthApp().clearSignTransaction(hdPath, tx, {
       externalPlugins: true,
-      erc20: true,
-      nft: true,
+      erc20,
+      nft,
     });
   }
 

packages/keyring-eth-ledger-bridge/src/utils.test.ts

@@ -0,0 +1,139 @@
+import { Common, Chain, Hardfork } from '@ethereumjs/common';
+import { TransactionFactory } from '@ethereumjs/tx';
+import { bytesToHex } from '@ethereumjs/util';
+import { remove0x } from '@metamask/utils';
+
+import { getTransactionSelector } from './utils';
+
+const TRANSFER_SELECTOR = '0xa9059cbb';
+const TRANSFER_FROM_SELECTOR = '0x23b872dd';
+const APPROVE_SELECTOR = '0x095ea7b3';
+
+describe('getTransactionSelector', () => {
+  const commonLegacy = new Common({
+    chain: Chain.Mainnet,
+    hardfork: Hardfork.Berlin,
+  });
+  const common1559 = new Common({
+    chain: Chain.Mainnet,
+    hardfork: Hardfork.London,
+  });
+
+  it('returns the first four bytes of calldata for a legacy serialized tx', () => {
+    const tx = TransactionFactory.fromTxData(
+      {
+        nonce: '0x00',
+        gasPrice: '0x01',
+        gasLimit: '0x5208',
+        to: '0x0000000000000000000000000000000000000000',
+        value: '0x00',
+        data: `${TRANSFER_SELECTOR}00`,
+      },
+      { common: commonLegacy },
+    );
+    const serializedHex = bytesToHex(tx.serialize());
+    expect(getTransactionSelector(serializedHex)).toBe(TRANSFER_SELECTOR);
+  });
+
+  it('returns the selector for an EIP-1559 serialized tx', () => {
+    const tx = TransactionFactory.fromTxData(
+      {
+        type: 2,
+        nonce: '0x00',
+        maxFeePerGas: '0x01',
+        maxPriorityFeePerGas: '0x01',
+        gasLimit: '0x5208',
+        to: '0x0000000000000000000000000000000000000000',
+        value: '0x00',
+        data: `${TRANSFER_SELECTOR}deadbeef`,
+      },
+      { common: common1559 },
+    );
+    const serializedHex = bytesToHex(tx.serialize());
+    expect(getTransactionSelector(serializedHex)).toBe(TRANSFER_SELECTOR);
+  });
+
+  it('accepts hex without a 0x prefix', () => {
+    const tx = TransactionFactory.fromTxData(
+      {
+        nonce: '0x00',
+        gasPrice: '0x01',
+        gasLimit: '0x5208',
+        to: '0x0000000000000000000000000000000000000000',
+        value: '0x00',
+        data: `${TRANSFER_SELECTOR}00`,
+      },
+      { common: commonLegacy },
+    );
+    const serializedHex = remove0x(bytesToHex(tx.serialize()));
+    expect(getTransactionSelector(serializedHex)).toBe(TRANSFER_SELECTOR);
+  });
+
+  it('returns undefined when calldata is empty', () => {
+    const tx = TransactionFactory.fromTxData(
+      {
+        nonce: '0x00',
+        gasPrice: '0x01',
+        gasLimit: '0x5208',
+        to: '0x0000000000000000000000000000000000000000',
+        value: '0x00',
+        data: '0x',
+      },
+      { common: commonLegacy },
+    );
+    expect(getTransactionSelector(bytesToHex(tx.serialize()))).toBeUndefined();
+  });
+
+  it('returns undefined for invalid serialized hex', () => {
+    expect(getTransactionSelector('0xnothex')).toBeUndefined();
+  });
+
+  it('returns transferFrom selector when calldata uses transferFrom', () => {
+    const tx = TransactionFactory.fromTxData(
+      {
+        nonce: '0x00',
+        gasPrice: '0x01',
+        gasLimit: '0x5208',
+        to: '0x0000000000000000000000000000000000000000',
+        value: '0x00',
+        data: `${TRANSFER_FROM_SELECTOR}00`,
+      },
+      { common: commonLegacy },
+    );
+    expect(getTransactionSelector(bytesToHex(tx.serialize()))).toBe(
+      TRANSFER_FROM_SELECTOR,
+    );
+  });
+
+  it('returns approve selector when calldata uses approve', () => {
+    const tx = TransactionFactory.fromTxData(
+      {
+        nonce: '0x00',
+        gasPrice: '0x01',
+        gasLimit: '0x5208',
+        to: '0x0000000000000000000000000000000000000000',
+        value: '0x00',
+        data: `${APPROVE_SELECTOR}00`,
+      },
+      { common: commonLegacy },
+    );
+    expect(getTransactionSelector(bytesToHex(tx.serialize()))).toBe(
+      APPROVE_SELECTOR,
+    );
+  });
+
+  it('returns undefined when calldata is shorter than four bytes', () => {
+    const tx = TransactionFactory.fromTxData(
+      {
+        nonce: '0x00',
+        gasPrice: '0x01',
+        gasLimit: '0x5208',
+        to: '0x0000000000000000000000000000000000000000',
+        value: '0x00',
+        data: '0xabcd',
+      },
+      { common: commonLegacy },
+    );
+    expect(getTransactionSelector(bytesToHex(tx.serialize()))).toBeUndefined();
+  });
+});