diff --git a/packages/keyring-eth-ledger-bridge/CHANGELOG.md b/packages/keyring-eth-ledger-bridge/CHANGELOG.md index 3eb7dc32b..a2451114b 100644 --- a/packages/keyring-eth-ledger-bridge/CHANGELOG.md +++ b/packages/keyring-eth-ledger-bridge/CHANGELOG.md @@ -11,6 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Bump `@metamask/keyring-sdk` from `^2.0.2` to `^2.1.1` ([#544](https://github.com/MetaMask/accounts/pull/544), [#546](https://github.com/MetaMask/accounts/pull/546)) +### Fixed + +- Fall back to blind signing on mobile when `clearSignTransaction` fails, except when the user rejects on the device ([#522](https://github.com/MetaMask/accounts/pull/522)) + ## [12.0.2] ### Changed diff --git a/packages/keyring-eth-ledger-bridge/package.json b/packages/keyring-eth-ledger-bridge/package.json index cee9ea818..4fef234bc 100644 --- a/packages/keyring-eth-ledger-bridge/package.json +++ b/packages/keyring-eth-ledger-bridge/package.json @@ -72,6 +72,7 @@ "@ethereumjs/tx": "^5.4.0", "@ethereumjs/util": "^9.1.0", "@ledgerhq/hw-app-eth": "^6.42.0", + "@ledgerhq/hw-transport": "^6.31.3", "@metamask/eth-sig-util": "^8.2.0", "@metamask/hw-wallet-sdk": "^0.8.0", "@metamask/keyring-api": "^23.1.0", @@ -82,7 +83,6 @@ "@ethereumjs/common": "^4.4.0", "@lavamoat/allow-scripts": "^3.2.1", "@lavamoat/preinstall-always-fail": "^2.1.0", - "@ledgerhq/hw-transport": "^6.31.3", "@ledgerhq/types-cryptoassets": "^7.15.1", "@ledgerhq/types-devices": "^6.25.3", "@ledgerhq/types-live": "^6.52.0", 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 dea82502c..79e1b98af 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,7 +1,7 @@ import { Common, Chain, Hardfork } from '@ethereumjs/common'; import { TransactionFactory } from '@ethereumjs/tx'; import { bytesToHex } from '@ethereumjs/util'; -import Transport from '@ledgerhq/hw-transport'; +import Transport, { TransportStatusError } from '@ledgerhq/hw-transport'; import { EIP712Message } from '@ledgerhq/types-live'; import { remove0x } from '@metamask/utils'; @@ -25,6 +25,7 @@ describe('LedgerMobileBridge', function () { const mockEthApp = { signEIP712Message: jest.fn(), clearSignTransaction: jest.fn(), + signTransaction: jest.fn(), getAddress: jest.fn(), signPersonalMessage: jest.fn(), openEthApp: jest.fn(), @@ -248,6 +249,56 @@ describe('LedgerMobileBridge', function () { nft: false, }); }); + + it('falls back to blind signing when clear-sign fails for unsupported resolution', async function () { + const hdPath = "m/44'/60'/0'/0/0"; + const tx = + 'f86d8202b38477359400825208944592d8f8d7b001e72cb26a73e4fa1806a51ac79d880de0b6b3a7640000802ba0699ff162205967ccbabae13e07cdd4284258d46ec1051a70a51be51ec2bc69f3a04e6944d508244ea54a62ebf9a72683eeadacb73ad7c373ee542f1998147b220e'; + const blindSignResult = { + v: '2b', + r: '0699ff162205967ccbabae13e07cdd4284258d46ec1051a70a51be51ec2bc69f3', + s: '04e6944d508244ea54a62ebf9a72683eeadacb73ad7c373ee542f1998147b220e', + }; + mockEthApp.clearSignTransaction.mockRejectedValueOnce( + new Error('resolution failed'), + ); + mockEthApp.signTransaction.mockResolvedValueOnce(blindSignResult); + + const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(); + + const result = await bridge.deviceSignTransaction({ + hdPath, + tx, + }); + + expect(result).toStrictEqual(blindSignResult); + expect(mockEthApp.clearSignTransaction).toHaveBeenCalledTimes(1); + expect(mockEthApp.signTransaction).toHaveBeenCalledTimes(1); + expect(mockEthApp.signTransaction).toHaveBeenCalledWith(hdPath, tx, null); + expect(consoleSpy).toHaveBeenCalledTimes(1); + expect(consoleSpy.mock.calls[0]?.[0]).toBe( + 'Ledger clear-sign failed; falling back to blind signing.', + ); + + consoleSpy.mockRestore(); + }); + + it('does not fall back when the user rejects on the device', async function () { + const hdPath = "m/44'/60'/0'/0/0"; + const tx = + 'f86d8202b38477359400825208944592d8f8d7b001e72cb26a73e4fa1806a51ac79d880de0b6b3a7640000802ba0699ff162205967ccbabae13e07cdd4284258d46ec1051a70a51be51ec2bc69f3a04e6944d508244ea54a62ebf9a72683eeadacb73ad7c373ee542f1998147b220e'; + const rejection = new TransportStatusError(0x6985); + mockEthApp.clearSignTransaction.mockRejectedValueOnce(rejection); + + await expect( + bridge.deviceSignTransaction({ + hdPath, + tx, + }), + ).rejects.toThrow(rejection); + + expect(mockEthApp.signTransaction).not.toHaveBeenCalled(); + }); }); describe('deviceSignTypedData', function () { 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 152854cfe..105c79c81 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-mobile-bridge.ts @@ -1,3 +1,4 @@ +import { TransportStatusError } from '@ledgerhq/hw-transport'; import type Transport from '@ledgerhq/hw-transport'; import { ERC20_WRITE_SELECTORS, NFT_ONLY_SELECTORS } from './constants'; @@ -19,6 +20,9 @@ import { TransportMiddleware } from './ledger-transport-middleware'; import { LedgerMobileBridgeOptions } from './type'; import { getTransactionSelector } from './utils'; +/** Ledger APDU: CONDITIONS_OF_USE_NOT_SATISFIED (user rejected on device). */ +const LEDGER_USER_REJECTION_STATUS = 0x6985; + // MobileBridge Type will always use LedgerBridge with LedgerMobileBridgeOptions export type MobileBridge = LedgerBridge & { openEthApp(): Promise; @@ -102,6 +106,9 @@ export class LedgerMobileBridge implements MobileBridge { * Method to sign a transaction * Sending the hexadecimal transaction message to the device and returning the signed transaction. * + * Tries clear-signing first. If resolution fails (e.g. no Ledger plugin for the chain or contract), + * falls back to blind signing via `signTransaction(..., null)` unless the user rejected on the device. + * * @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. @@ -115,11 +122,25 @@ export class LedgerMobileBridge implements MobileBridge { 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, - nft, - }); + const ethApp = this.#getEthApp(); + + try { + return await ethApp.clearSignTransaction(hdPath, tx, { + externalPlugins: true, + erc20, + nft, + }); + } catch (error: unknown) { + if (LedgerMobileBridge.#isLedgerUserRejection(error)) { + throw error; + } + + console.warn( + 'Ledger clear-sign failed; falling back to blind signing.', + error, + ); + return ethApp.signTransaction(hdPath, tx, null); + } } /** @@ -235,4 +256,18 @@ export class LedgerMobileBridge implements MobileBridge { #getEthApp(): MetaMaskLedgerHwAppEth { return this.#getTransportMiddleWare().getEthApp(); } + + /** + * Detects an explicit on-device rejection so we do not fall back to blind signing + * and re-prompt the user. + * + * @param error - Error from Ledger transport or hw-app-eth. + * @returns True when the user rejected the action on the device. + */ + static #isLedgerUserRejection(error: unknown): boolean { + return ( + error instanceof TransportStatusError && + error.statusCode === LEDGER_USER_REJECTION_STATUS + ); + } }