diff --git a/packages/keyring-eth-hd/package.json b/packages/keyring-eth-hd/package.json index 8a5885f0..56a13cda 100644 --- a/packages/keyring-eth-hd/package.json +++ b/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", diff --git a/packages/keyring-eth-ledger-bridge/CHANGELOG.md b/packages/keyring-eth-ledger-bridge/CHANGELOG.md index cde54004..21c2801f 100644 --- a/packages/keyring-eth-ledger-bridge/CHANGELOG.md +++ b/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 diff --git a/packages/keyring-eth-ledger-bridge/jest.config.js b/packages/keyring-eth-ledger-bridge/jest.config.js index 48b8bcb0..c60d853b 100644 --- a/packages/keyring-eth-ledger-bridge/jest.config.js +++ b/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, }, }, }); diff --git a/packages/keyring-eth-ledger-bridge/package.json b/packages/keyring-eth-ledger-bridge/package.json index 55699819..b20188be 100644 --- a/packages/keyring-eth-ledger-bridge/package.json +++ b/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", diff --git a/packages/keyring-eth-ledger-bridge/src/constants.test.ts b/packages/keyring-eth-ledger-bridge/src/constants.test.ts new file mode 100644 index 00000000..9dc51eaa --- /dev/null +++ b/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, + ); + } + }); +}); diff --git a/packages/keyring-eth-ledger-bridge/src/constants.ts b/packages/keyring-eth-ledger-bridge/src/constants.ts new file mode 100644 index 00000000..fad1e6ec --- /dev/null +++ b/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) +]); diff --git a/packages/keyring-eth-ledger-bridge/src/index.ts b/packages/keyring-eth-ledger-bridge/src/index.ts index 46951e09..90b53a3a 100644 --- a/packages/keyring-eth-ledger-bridge/src/index.ts +++ b/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'; 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..dea82502 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 @@ -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, }); }); }); 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..7a90a7ed 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.ts +++ b/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 & { @@ -110,10 +112,14 @@ export class LedgerMobileBridge implements MobileBridge { tx, hdPath, }: LedgerSignTransactionParams): Promise { + 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, }); } diff --git a/packages/keyring-eth-ledger-bridge/src/utils.test.ts b/packages/keyring-eth-ledger-bridge/src/utils.test.ts new file mode 100644 index 00000000..3dfe5847 --- /dev/null +++ b/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(); + }); +}); diff --git a/packages/keyring-eth-ledger-bridge/src/utils.ts b/packages/keyring-eth-ledger-bridge/src/utils.ts new file mode 100644 index 00000000..6b1fc743 --- /dev/null +++ b/packages/keyring-eth-ledger-bridge/src/utils.ts @@ -0,0 +1,25 @@ +import { TransactionFactory } from '@ethereumjs/tx'; +import { bytesToHex, hexToBytes } from '@ethereumjs/util'; +import { add0x } from '@metamask/utils'; + +/** + * Returns the 4-byte selector from raw serialized transaction hex or undefined if not present. + * Supports legacy RLP and EIP-2718 typed transactions (via `@ethereumjs/tx`). + * + * @param rawTxHex - Raw serialized transaction hex (with or without `0x` prefix). + * @returns The selector (`0x` + 8 hex digits, lowercased) or undefined if parsing fails or no calldata. + */ +export function getTransactionSelector(rawTxHex: string): string | undefined { + try { + const prefixedHex = add0x(rawTxHex); + const tx = TransactionFactory.fromSerializedData(hexToBytes(prefixedHex)); + const dataHex = bytesToHex(tx.data); + const selectorSize = 2 /* 0x */ + 4 * 2; /* 4 bytes (hex) */ + if (dataHex.length >= selectorSize) { + return dataHex.slice(0, selectorSize).toLowerCase(); + } + } catch { + // ignore parse errors + } + return undefined; +} diff --git a/packages/keyring-eth-simple/package.json b/packages/keyring-eth-simple/package.json index 523d4da1..eb8acaea 100644 --- a/packages/keyring-eth-simple/package.json +++ b/packages/keyring-eth-simple/package.json @@ -49,7 +49,7 @@ "@metamask/keyring-api": "^22.0.0", "@metamask/keyring-sdk": "^1.2.0", "@metamask/utils": "^11.10.0", - "ethereum-cryptography": "^2.1.2", + "ethereum-cryptography": "^2.2.1", "randombytes": "^2.1.0" }, "devDependencies": { diff --git a/packages/keyring-sdk/package.json b/packages/keyring-sdk/package.json index 320d87bd..7ff6ced7 100644 --- a/packages/keyring-sdk/package.json +++ b/packages/keyring-sdk/package.json @@ -53,7 +53,7 @@ "@metamask/superstruct": "^3.1.0", "@metamask/utils": "^11.10.0", "async-mutex": "^0.5.0", - "ethereum-cryptography": "^2.1.2", + "ethereum-cryptography": "^2.2.1", "uuid": "^9.0.1" }, "devDependencies": { diff --git a/yarn.lock b/yarn.lock index c279559f..024c282b 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1700,7 +1700,7 @@ __metadata: "@ts-bridge/cli": "npm:^0.6.3" "@types/jest": "npm:^29.5.12" deepmerge: "npm:^4.2.2" - ethereum-cryptography: "npm:^2.1.2" + ethereum-cryptography: "npm:^2.2.1" jest: "npm:^29.5.0" old-hd-keyring: "npm:@metamask/eth-hd-keyring@^4.0.1" languageName: unknown @@ -1737,6 +1737,7 @@ __metadata: "@types/web": "npm:^0.0.69" deepmerge: "npm:^4.2.2" depcheck: "npm:^1.4.7" + ethereum-cryptography: "npm:^2.2.1" ethereumjs-tx: "npm:^1.3.7" hdkey: "npm:^2.1.0" jest: "npm:^29.5.0" @@ -1863,7 +1864,7 @@ __metadata: "@types/randombytes": "npm:^2.0.0" deepmerge: "npm:^4.2.2" depcheck: "npm:^1.4.7" - ethereum-cryptography: "npm:^2.1.2" + ethereum-cryptography: "npm:^2.2.1" ethereumjs-tx: "npm:^1.3.7" jest: "npm:^29.5.0" randombytes: "npm:^2.1.0" @@ -2136,7 +2137,7 @@ __metadata: async-mutex: "npm:^0.5.0" deepmerge: "npm:^4.2.2" depcheck: "npm:^1.4.7" - ethereum-cryptography: "npm:^2.1.2" + ethereum-cryptography: "npm:^2.2.1" jest: "npm:^29.5.0" jest-it-up: "npm:^3.1.0" rimraf: "npm:^5.0.7"