diff --git a/packages/keyring-eth-ledger-bridge/CHANGELOG.md b/packages/keyring-eth-ledger-bridge/CHANGELOG.md index 03d11cbc..a4ca3ff6 100644 --- a/packages/keyring-eth-ledger-bridge/CHANGELOG.md +++ b/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 diff --git a/packages/keyring-eth-ledger-bridge/src/index.ts b/packages/keyring-eth-ledger-bridge/src/index.ts index 46951e09..0d1ecb8e 100644 --- a/packages/keyring-eth-ledger-bridge/src/index.ts +++ b/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'; diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-bridge.ts b/packages/keyring-eth-ledger-bridge/src/ledger-bridge.ts index 5f4ba801..2dbb164a 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-bridge.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-bridge.ts @@ -7,7 +7,16 @@ export type GetPublicKeyResponse = Awaited< ReturnType >; -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 >; diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.test.ts b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.test.ts index a4be4895..07a8c25a 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.test.ts +++ b/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; }); diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts index ca5cdc39..e3ca1e48 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts +++ b/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( diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.test.ts b/packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.test.ts index bd1b45f3..0e02d6f7 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.test.ts +++ b/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, }); }); }); diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.ts b/packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.ts index 3eb8ff13..096b3870 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.ts +++ b/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 { + const nftResolution = nft ?? shouldUseNftLedgerClearSign(tx); return this.#getEthApp().clearSignTransaction(hdPath, tx, { externalPlugins: true, erc20: true, - nft: true, + nft: nftResolution, }); } diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-nft-clear-sign.test.ts b/packages/keyring-eth-ledger-bridge/src/ledger-nft-clear-sign.test.ts new file mode 100644 index 00000000..60330084 --- /dev/null +++ b/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); + }); +}); diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-nft-clear-sign.ts b/packages/keyring-eth-ledger-bridge/src/ledger-nft-clear-sign.ts new file mode 100644 index 00000000..0f1d61d5 --- /dev/null +++ b/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; + } +}