fix: add clear signing selector

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

@@ -12,6 +12,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Add new dependency `@metamask/keyring-sdk` ([#478](https://github.com/MetaMask/accounts/pull/478))
   - This package now contains the keyring v2 wrapper helpers (`EthKeyringWrapper`).
 
+### Fixed
+
+- Set Ledger clear-sign `nft` from transaction calldata so ERC-20 `approve` and other shared selectors no longer show NFT allowance prompts ([#475](https://github.com/MetaMask/accounts/pull/475))
+  - Export `shouldUseNftLedgerClearSign` for reuse.
+  - Extend `LedgerSignTransactionParams` with optional `nft` (forwarded to iframe bridges).
+
 ## [11.3.0]
 
 ### Changed

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

@@ -8,3 +8,4 @@ export type * from './type';
 export * from './ledger-hw-app';
 export * from './errors';
 export * from './ledger-error-handler';
+export { shouldUseNftLedgerClearSign } from './ledger-nft-clear-sign';

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

@@ -7,7 +7,16 @@ export type GetPublicKeyResponse = Awaited<
   ReturnType<LedgerHwAppEth['getAddress']>
 >;
 
-export type LedgerSignTransactionParams = { hdPath: string; tx: string };
+export type LedgerSignTransactionParams = {
+  hdPath: string;
+  tx: string;
+  /**
+   * Ledger clear-sign NFT resolution. When omitted, implementations should
+   * derive a safe default from the serialized transaction (see
+   * `shouldUseNftLedgerClearSign`).
+   */
+  nft?: boolean;
+};
 export type LedgerSignTransactionResponse = Awaited<
   ReturnType<LedgerHwAppEth['signTransaction']>
 >;

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

@@ -11,6 +11,7 @@ import HDKey from 'hdkey';
 import { LedgerBridge, LedgerBridgeOptions } from './ledger-bridge';
 import { LedgerIframeBridge } from './ledger-iframe-bridge';
 import { AccountDetails, LedgerKeyring } from './ledger-keyring';
+import { shouldUseNftLedgerClearSign } from './ledger-nft-clear-sign';
 
 jest.mock('@metamask/eth-sig-util', () => {
   return {
@@ -618,9 +619,11 @@ describe('LedgerKeyring', function () {
           jest
             .spyOn(keyring.bridge, 'deviceSignTransaction')
             .mockImplementation(async (params) => {
+              const expectedTxHex = fakeTx.serialize().toString('hex');
               expect(params).toStrictEqual({
                 hdPath: "m/44'/60'/0'/0",
-                tx: fakeTx.serialize().toString('hex'),
+                tx: expectedTxHex,
+                nft: shouldUseNftLedgerClearSign(expectedTxHex),
               });
               return { v: '0x1', r: '0x0', s: '0x0' };
             });
@@ -670,11 +673,13 @@ describe('LedgerKeyring', function () {
           jest
             .spyOn(keyring.bridge, 'deviceSignTransaction')
             .mockImplementation(async (params) => {
+              const expectedTxHex = Buffer.from(
+                RLP.encode(newFakeTx.getMessageToSign()),
+              ).toString('hex');
               expect(params).toStrictEqual({
                 hdPath: "m/44'/60'/0'/0",
-                tx: Buffer.from(
-                  RLP.encode(newFakeTx.getMessageToSign()),
-                ).toString('hex'),
+                tx: expectedTxHex,
+                nft: shouldUseNftLedgerClearSign(expectedTxHex),
               });
               return expectedRSV;
             });
@@ -718,11 +723,14 @@ describe('LedgerKeyring', function () {
           jest
             .spyOn(keyring.bridge, 'deviceSignTransaction')
             .mockImplementation(async (params) => {
+              const messageHex = bytesToHex(
+                fakeTypeTwoTx.getMessageToSign() as Uint8Array,
+              );
+              const expectedTxHex = remove0x(messageHex);
               expect(params).toStrictEqual({
                 hdPath: "m/44'/60'/0'/0",
-                tx: remove0x(
-                  bytesToHex(fakeTypeTwoTx.getMessageToSign() as Uint8Array),
-                ),
+                tx: expectedTxHex,
+                nft: shouldUseNftLedgerClearSign(messageHex),
               });
               return expectedRSV;
             });

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

@@ -32,6 +32,7 @@ import {
 } from './ledger-bridge';
 import { handleLedgerTransportError } from './ledger-error-handler';
 import { LedgerIframeBridgeOptions } from './ledger-iframe-bridge';
+import { shouldUseNftLedgerClearSign } from './ledger-nft-clear-sign';
 
 const pathBase = 'm';
 const hdPathString = `${pathBase}/44'/60'/0'`;
@@ -441,6 +442,7 @@ export class LedgerKeyring implements Keyring {
       payload = await this.bridge.deviceSignTransaction({
         tx: remove0x(rawTxHex),
         hdPath,
+        nft: shouldUseNftLedgerClearSign(rawTxHex),
       });
     } catch (error: unknown) {
       handleLedgerTransportError(

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

@@ -162,6 +162,37 @@ describe('LedgerMobileBridge', function () {
       });
       expect(transportMiddlewareGetEthAppSpy).toHaveBeenCalledTimes(1);
       expect(mockEthApp.clearSignTransaction).toHaveBeenCalledTimes(1);
+      expect(mockEthApp.clearSignTransaction).toHaveBeenCalledWith(hdPath, tx, {
+        externalPlugins: true,
+        erc20: true,
+        nft: false,
+      });
+    });
+
+    it('enables NFT clear-sign resolution for setApprovalForAll calldata', async function () {
+      const hdPath = "m/44'/60'/0'/0/0";
+      const tx =
+        '02f86b0180843b9aca00843b9aca0082520894111111111111111111111111111111111111111180b840a22cb465000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000c0808080';
+      await bridge.deviceSignTransaction({
+        hdPath,
+        tx,
+      });
+      expect(mockEthApp.clearSignTransaction).toHaveBeenCalledWith(hdPath, tx, {
+        externalPlugins: true,
+        erc20: true,
+        nft: true,
+      });
+    });
+
+    it('uses explicit nft when provided', async function () {
+      const hdPath = "m/44'/60'/0'/0/0";
+      const tx =
+        '02f86b0180843b9aca00843b9aca0082520894111111111111111111111111111111111111111180b840095ea7b3000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000c0808080';
+      await bridge.deviceSignTransaction({
+        hdPath,
+        tx,
+        nft: true,
+      });
       expect(mockEthApp.clearSignTransaction).toHaveBeenCalledWith(hdPath, tx, {
         externalPlugins: true,
         erc20: true,
@@ -183,7 +214,7 @@ describe('LedgerMobileBridge', function () {
       expect(mockEthApp.clearSignTransaction).toHaveBeenCalledWith(hdPath, tx, {
         externalPlugins: true,
         erc20: true,
-        nft: true,
+        nft: false,
       });
     });
   });

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

@@ -14,6 +14,7 @@ import {
   LedgerSignTypedDataResponse,
 } from './ledger-bridge';
 import { MetaMaskLedgerHwAppEth } from './ledger-hw-app';
+import { shouldUseNftLedgerClearSign } from './ledger-nft-clear-sign';
 import { TransportMiddleware } from './ledger-transport-middleware';
 import { LedgerMobileBridgeOptions } from './type';
 
@@ -104,16 +105,19 @@ export class LedgerMobileBridge implements MobileBridge {
    * @param params - An object contains tx, hdPath.
    * @param params.tx - The raw ethereum transaction in hexadecimal to sign.
    * @param params.hdPath - The BIP 32 path of the account.
+   * @param params.nft - Whether to enable NFT clear-sign resolution for the transaction.
    * @returns Retrieve v, r, s from the signed transaction.
    */
   async deviceSignTransaction({
     tx,
     hdPath,
+    nft,
   }: LedgerSignTransactionParams): Promise<LedgerSignTransactionResponse> {
+    const nftResolution = nft ?? shouldUseNftLedgerClearSign(tx);
     return this.#getEthApp().clearSignTransaction(hdPath, tx, {
       externalPlugins: true,
       erc20: true,
-      nft: true,
+      nft: nftResolution,
     });
   }
 

packages/keyring-eth-ledger-bridge/src/ledger-nft-clear-sign.test.ts

@@ -0,0 +1,104 @@
+import { Common, Chain, Hardfork } from '@ethereumjs/common';
+import { TransactionFactory } from '@ethereumjs/tx';
+
+import { shouldUseNftLedgerClearSign } from './ledger-nft-clear-sign';
+
+const commonEIP1559 = new Common({
+  chain: Chain.Mainnet,
+  hardfork: Hardfork.London,
+});
+
+/** EIP-1559 tx with ERC-20 `approve` calldata (`0x095ea7b3`). */
+const SERIALIZED_TX_ERC20_APPROVE =
+  '02f86b0180843b9aca00843b9aca0082520894111111111111111111111111111111111111111180b840095ea7b3000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000c0808080';
+
+/** EIP-1559 tx with ERC-721 / ERC-1155 `setApprovalForAll` calldata (`0xa22cb465`). */
+const SERIALIZED_TX_SET_APPROVAL_FOR_ALL =
+  '02f86b0180843b9aca00843b9aca0082520894111111111111111111111111111111111111111180b840a22cb465000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000c0808080';
+
+function serializeType2TxWithData(data: `0x${string}`): string {
+  return TransactionFactory.fromTxData(
+    {
+      type: 2,
+      nonce: '0x00',
+      maxFeePerGas: '0x3b9aca00',
+      maxPriorityFeePerGas: '0x3b9aca00',
+      gasLimit: '0x5208',
+      to: `0x${'2'.repeat(40)}`,
+      value: '0x00',
+      data,
+      v: '0x0',
+      r: `0x${'0'.repeat(64)}`,
+      s: `0x${'0'.repeat(64)}`,
+    },
+    { common: commonEIP1559, freeze: false },
+  )
+    .serialize()
+    .toString('hex');
+}
+
+describe('shouldUseNftLedgerClearSign', function () {
+  it('returns false for empty input', function () {
+    expect(shouldUseNftLedgerClearSign('')).toBe(false);
+    expect(shouldUseNftLedgerClearSign('0x')).toBe(false);
+  });
+
+  it('returns false when calldata is shorter than 4 bytes', function () {
+    const hex = serializeType2TxWithData('0x010203');
+    expect(shouldUseNftLedgerClearSign(hex)).toBe(false);
+    expect(shouldUseNftLedgerClearSign(`0x${hex}`)).toBe(false);
+  });
+
+  it('returns false for ERC-20 approve selector (shared with ERC-721 approve)', function () {
+    expect(shouldUseNftLedgerClearSign(SERIALIZED_TX_ERC20_APPROVE)).toBe(false);
+    expect(
+      shouldUseNftLedgerClearSign(`0x${SERIALIZED_TX_ERC20_APPROVE}`),
+    ).toBe(false);
+  });
+
+  it('returns true for setApprovalForAll selector', function () {
+    expect(
+      shouldUseNftLedgerClearSign(SERIALIZED_TX_SET_APPROVAL_FOR_ALL),
+    ).toBe(true);
+  });
+
+  it('returns true for ERC-721 safeTransferFrom(address,address,uint256)', function () {
+    const hex = serializeType2TxWithData(
+      `0x42842e0e${'00'.repeat(60)}`,
+    );
+    expect(shouldUseNftLedgerClearSign(hex)).toBe(true);
+  });
+
+  it('returns true for ERC-721 safeTransferFrom with bytes', function () {
+    const hex = serializeType2TxWithData(
+      `0xb88d4fde${'00'.repeat(60)}`,
+    );
+    expect(shouldUseNftLedgerClearSign(hex)).toBe(true);
+  });
+
+  it('returns true for ERC-1155 safeTransferFrom', function () {
+    const hex = serializeType2TxWithData(
+      `0xf242432a${'00'.repeat(60)}`,
+    );
+    expect(shouldUseNftLedgerClearSign(hex)).toBe(true);
+  });
+
+  it('returns true for ERC-1155 safeBatchTransferFrom', function () {
+    const hex = serializeType2TxWithData(
+      `0x2eb2c2d6${'00'.repeat(60)}`,
+    );
+    expect(shouldUseNftLedgerClearSign(hex)).toBe(true);
+  });
+
+  it('returns false for transferFrom selector (shared ERC-20 / ERC-721)', function () {
+    const hex = serializeType2TxWithData(
+      `0x23b872dd${'00'.repeat(60)}`,
+    );
+    expect(shouldUseNftLedgerClearSign(hex)).toBe(false);
+  });
+
+  it('returns false on invalid serialized transaction', function () {
+    expect(shouldUseNftLedgerClearSign('zz')).toBe(false);
+    expect(shouldUseNftLedgerClearSign('deadbeef')).toBe(false);
+  });
+});

packages/keyring-eth-ledger-bridge/src/ledger-nft-clear-sign.ts

@@ -0,0 +1,52 @@
+import { TransactionFactory } from '@ethereumjs/tx';
+import { Buffer } from 'buffer';
+
+/**
+ * Function selectors that only appear on ERC-721 / ERC-1155 contracts for the
+ * purposes of Ledger `@ledgerhq/evm-tools` resolution. Shared selectors such as
+ * `approve` (`0x095ea7b3`) and `transferFrom` (`0x23b872dd`) are intentionally
+ * excluded because ERC-20 uses the same selectors; forcing NFT resolution for
+ * those causes incorrect Ledger prompts (e.g. "manage NFT allowance" on ERC-20
+ * approves).
+ *
+ * @see https://github.com/MetaMask/metamask-extension/pull/39921
+ */
+const NFT_ONLY_SELECTORS = new Set([
+  '0xa22cb465', // setApprovalForAll(address,bool)
+  '0x42842e0e', // safeTransferFrom(from,to,tokenId) — ERC-721
+  '0xb88d4fde', // safeTransferFrom(from,to,tokenId,data) — ERC-721
+  '0xf242432a', // safeTransferFrom — ERC-1155
+  '0x2eb2c2d6', // safeBatchTransferFrom — ERC-1155
+]);
+
+/**
+ * Whether Ledger clear-signing should enable NFT plugin resolution for this raw
+ * serialized transaction.
+ *
+ * @param rawTxHex - RLP-encoded transaction hex, with or without `0x` prefix.
+ * @returns True when calldata's 4-byte selector is NFT-only.
+ */
+export function shouldUseNftLedgerClearSign(rawTxHex: string): boolean {
+  const first4BytesHex =
+    rawTxHex.startsWith('0x') || rawTxHex.startsWith('0X')
+      ? rawTxHex.slice(2)
+      : rawTxHex;
+  if (first4BytesHex.length === 0) {
+    return false;
+  }
+  try {
+    const hexBuffer = Buffer.from(first4BytesHex, 'hex');
+    if (hexBuffer.length === 0) {
+      return false;
+    }
+    const tx = TransactionFactory.fromSerializedData(hexBuffer);
+    const { data } = tx;
+    if (data.length < 4) {
+      return false;
+    }
+    const selector = `0x${Buffer.from(data.subarray(0, 4)).toString('hex')}`;
+    return NFT_ONLY_SELECTORS.has(selector.toLowerCase());
+  } catch {
+    return false;
+  }
+}