diff --git a/packages/keyring-eth-ledger-bridge/CHANGELOG.md b/packages/keyring-eth-ledger-bridge/CHANGELOG.md index b86345862..239c014c9 100644 --- a/packages/keyring-eth-ledger-bridge/CHANGELOG.md +++ b/packages/keyring-eth-ledger-bridge/CHANGELOG.md @@ -10,6 +10,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Add `LedgerKeyringV2` class implementing `KeyringV2` interface ([#416](https://github.com/MetaMask/accounts/pull/416)) + +### Fixed + +- Normalize signature `v` value from Ledger devices for proper recovery ([#449](https://github.com/MetaMask/accounts/pull/449)) - Wraps legacy `LedgerKeyring` to expose accounts via the unified `KeyringV2` API and the `KeyringAccount` type. - Extends `EthKeyringWrapper` for common Ethereum logic. diff --git a/packages/keyring-eth-ledger-bridge/jest.config.js b/packages/keyring-eth-ledger-bridge/jest.config.js index 4068f3e29..9cb570d4f 100644 --- a/packages/keyring-eth-ledger-bridge/jest.config.js +++ b/packages/keyring-eth-ledger-bridge/jest.config.js @@ -23,9 +23,9 @@ module.exports = merge(baseConfig, { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 93.58, - functions: 98.16, - lines: 97.75, + branches: 93.61, + functions: 98.18, + lines: 97.74, statements: 97.76, }, }, 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 9df2a05ea..7d5be3a9b 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.test.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.test.ts @@ -947,6 +947,82 @@ describe('LedgerKeyring', function () { keyring.signPersonalMessage(fakeAccounts[0], 'some message'), ).rejects.toThrow(transportError); }); + + it('normalizes v=0 to v=27 for proper signature recovery', async function () { + await basicSetupToUnlockOneAccount(); + jest + .spyOn(keyring.bridge, 'deviceSignMessage') + .mockResolvedValue({ v: 0, r: 'aabbccdd', s: '11223344' }); + + jest + .spyOn(sigUtil, 'recoverPersonalSignature') + .mockReturnValue(fakeAccounts[0]); + + const result = await keyring.signPersonalMessage( + fakeAccounts[0], + 'some message', + ); + + // v=0 should be normalized to v=27 (0x1b) + expect(result).toBe('0xaabbccdd112233441b'); + }); + + it('normalizes v=1 to v=28 for proper signature recovery', async function () { + await basicSetupToUnlockOneAccount(); + jest + .spyOn(keyring.bridge, 'deviceSignMessage') + .mockResolvedValue({ v: 1, r: 'aabbccdd', s: '11223344' }); + + jest + .spyOn(sigUtil, 'recoverPersonalSignature') + .mockReturnValue(fakeAccounts[0]); + + const result = await keyring.signPersonalMessage( + fakeAccounts[0], + 'some message', + ); + + // v=1 should be normalized to v=28 (0x1c) + expect(result).toBe('0xaabbccdd112233441c'); + }); + + it('preserves v=27 unchanged', async function () { + await basicSetupToUnlockOneAccount(); + jest + .spyOn(keyring.bridge, 'deviceSignMessage') + .mockResolvedValue({ v: 27, r: 'aabbccdd', s: '11223344' }); + + jest + .spyOn(sigUtil, 'recoverPersonalSignature') + .mockReturnValue(fakeAccounts[0]); + + const result = await keyring.signPersonalMessage( + fakeAccounts[0], + 'some message', + ); + + // v=27 should remain as 0x1b + expect(result).toBe('0xaabbccdd112233441b'); + }); + + it('preserves v=28 unchanged', async function () { + await basicSetupToUnlockOneAccount(); + jest + .spyOn(keyring.bridge, 'deviceSignMessage') + .mockResolvedValue({ v: 28, r: 'aabbccdd', s: '11223344' }); + + jest + .spyOn(sigUtil, 'recoverPersonalSignature') + .mockReturnValue(fakeAccounts[0]); + + const result = await keyring.signPersonalMessage( + fakeAccounts[0], + 'some message', + ); + + // v=28 should remain as 0x1c + expect(result).toBe('0xaabbccdd112233441c'); + }); }); describe('signMessage', function () { @@ -1279,7 +1355,9 @@ describe('LedgerKeyring', function () { ).rejects.toThrow('Ledger: Unknown error while signing message'); }); - it('returns signature when recoveryId length < 2', async function () { + it('normalizes v=0 to v=27 for proper signature recovery', async function () { + // Ledger may return v as 0 or 1 (modern format), but signature + // recovery expects 27 or 28 (legacy format) jest.spyOn(keyring.bridge, 'deviceSignTypedData').mockResolvedValue({ v: 0, r: '72d4e38a0e582e09a620fd38e236fe687a1ec782206b56d576f579c026a7e5b9', @@ -1292,8 +1370,45 @@ describe('LedgerKeyring', function () { version: sigUtil.SignTypedDataVersion.V4, }, ); + // v=0 should be normalized to v=27 (0x1b) expect(result).toBe( - '0x72d4e38a0e582e09a620fd38e236fe687a1ec782206b56d576f579c026a7e5b946759735981cd0c3efb02d36df28bb2feedfec3d90e408efc93f45b894946e3200', + '0x72d4e38a0e582e09a620fd38e236fe687a1ec782206b56d576f579c026a7e5b946759735981cd0c3efb02d36df28bb2feedfec3d90e408efc93f45b894946e321b', + ); + }); + + it('normalizes v=1 to v=28 for proper signature recovery', async function () { + jest.spyOn(keyring.bridge, 'deviceSignTypedData').mockResolvedValue({ + v: 1, + r: '72d4e38a0e582e09a620fd38e236fe687a1ec782206b56d576f579c026a7e5b9', + s: '46759735981cd0c3efb02d36df28bb2feedfec3d90e408efc93f45b894946e32', + }); + + // v=1 should be normalized to v=28 (0x1c), but this will fail + // address recovery since the correct v for this signature is 27 + await expect( + keyring.signTypedData(fakeAccounts[15], fixtureData, { + version: sigUtil.SignTypedDataVersion.V4, + }), + ).rejects.toThrow( + 'Ledger: The signature doesnt match the right address', + ); + }); + + it('preserves v=27 unchanged', async function () { + jest.spyOn(keyring.bridge, 'deviceSignTypedData').mockResolvedValue({ + v: 27, + r: '72d4e38a0e582e09a620fd38e236fe687a1ec782206b56d576f579c026a7e5b9', + s: '46759735981cd0c3efb02d36df28bb2feedfec3d90e408efc93f45b894946e32', + }); + const result = await keyring.signTypedData( + fakeAccounts[15], + fixtureData, + { + version: sigUtil.SignTypedDataVersion.V4, + }, + ); + expect(result).toBe( + '0x72d4e38a0e582e09a620fd38e236fe687a1ec782206b56d576f579c026a7e5b946759735981cd0c3efb02d36df28bb2feedfec3d90e408efc93f45b894946e321b', ); }); diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts index 62295534d..7b643f616 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts @@ -458,10 +458,9 @@ export class LedgerKeyring implements Keyring { ); } - let modifiedV = parseInt(String(payload.v), 10).toString(16); - if (modifiedV.length < 2) { - modifiedV = `0${modifiedV}`; - } + const modifiedV = this.#normalizeRecoveryParam( + parseInt(String(payload.v), 10), + ); const signature = `0x${payload.r}${payload.s}${modifiedV}`; const addressSignedWith = recoverPersonalSignature({ @@ -551,10 +550,9 @@ export class LedgerKeyring implements Keyring { ); } - let recoveryId = parseInt(String(payload.v), 10).toString(16); - if (recoveryId.length < 2) { - recoveryId = `0${recoveryId}`; - } + const recoveryId = this.#normalizeRecoveryParam( + parseInt(String(payload.v), 10), + ); const signature = `0x${payload.r}${payload.s}${recoveryId}`; const addressSignedWith = recoverTypedSignature({ data, @@ -697,4 +695,19 @@ export class LedgerKeyring implements Keyring { #getChecksumHexAddress(address: string): Hex { return getChecksumAddress(add0x(address)); } + + /** + * Normalizes the signature recovery parameter (v) to legacy format. + * Ledger devices may return v as 0 or 1 (modern format), but signature + * recovery expects 27 or 28 (legacy format per EIP-191/EIP-712). + * + * @param recoveryParam - The recovery parameter from Ledger. + * @returns The normalized recovery parameter as a hex string. + */ + #normalizeRecoveryParam(recoveryParam: number): string { + if (recoveryParam === 0 || recoveryParam === 1) { + return (recoveryParam + 27).toString(16); + } + return recoveryParam.toString(16); + } }