From 51f54274c842ecbb9664977a9191f14ac42776a7 Mon Sep 17 00:00:00 2001 From: Sam Walker Date: Thu, 29 Jan 2026 15:48:59 -0500 Subject: [PATCH 1/4] fix(keyring-eth-ledger-bridge): normalize signature v value for proper recovery Ledger devices may return v as 0 or 1 (modern format), but signature recovery functions like `recoverPersonalSignature` and `recoverTypedSignature` expect v to be 27 or 28 (legacy format per EIP-191/EIP-712). This caused "The signature doesnt match the right address" errors when Ledger returned v=0 or v=1, as the hex conversion produced "00"/"01" instead of "1b"/"1c". The fix normalizes v before hex conversion in both `signPersonalMessage` and `signTypedData` methods. --- .../src/ledger-keyring.test.ts | 119 +++++++++++++++++- .../src/ledger-keyring.ts | 16 ++- 2 files changed, 131 insertions(+), 4 deletions(-) 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..c01a040d2 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts @@ -458,7 +458,13 @@ export class LedgerKeyring implements Keyring { ); } - let modifiedV = parseInt(String(payload.v), 10).toString(16); + let recoveryParam = parseInt(String(payload.v), 10); + // Normalize: Ledger may return 0 or 1 (modern format), but signature + // recovery expects 27 or 28 (legacy format per EIP-191) + if (recoveryParam === 0 || recoveryParam === 1) { + recoveryParam += 27; + } + let modifiedV = recoveryParam.toString(16); if (modifiedV.length < 2) { modifiedV = `0${modifiedV}`; } @@ -551,7 +557,13 @@ export class LedgerKeyring implements Keyring { ); } - let recoveryId = parseInt(String(payload.v), 10).toString(16); + let recoveryParam = parseInt(String(payload.v), 10); + // Normalize: Ledger may return 0 or 1 (modern format), but signature + // recovery expects 27 or 28 (legacy format per EIP-712) + if (recoveryParam === 0 || recoveryParam === 1) { + recoveryParam += 27; + } + let recoveryId = recoveryParam.toString(16); if (recoveryId.length < 2) { recoveryId = `0${recoveryId}`; } From 9fcf3816dbe0c685ea58d1c43168e81792847524 Mon Sep 17 00:00:00 2001 From: Sam Walker Date: Thu, 29 Jan 2026 15:52:24 -0500 Subject: [PATCH 2/4] docs: add changelog entry --- packages/keyring-eth-ledger-bridge/CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) 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. From 623b5cc58183f4d9176c17bc81e956b519024961 Mon Sep 17 00:00:00 2001 From: Sam Walker Date: Thu, 29 Jan 2026 16:03:29 -0500 Subject: [PATCH 3/4] fix: remove dead padding code, update coverage thresholds --- packages/keyring-eth-ledger-bridge/jest.config.js | 4 ++-- .../keyring-eth-ledger-bridge/src/ledger-keyring.ts | 10 ++-------- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/packages/keyring-eth-ledger-bridge/jest.config.js b/packages/keyring-eth-ledger-bridge/jest.config.js index 4068f3e29..7fc486c7f 100644 --- a/packages/keyring-eth-ledger-bridge/jest.config.js +++ b/packages/keyring-eth-ledger-bridge/jest.config.js @@ -23,10 +23,10 @@ module.exports = merge(baseConfig, { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 93.58, + branches: 93.71, functions: 98.16, lines: 97.75, - statements: 97.76, + statements: 97.77, }, }, }); diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts index c01a040d2..4448a6208 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts @@ -464,10 +464,7 @@ export class LedgerKeyring implements Keyring { if (recoveryParam === 0 || recoveryParam === 1) { recoveryParam += 27; } - let modifiedV = recoveryParam.toString(16); - if (modifiedV.length < 2) { - modifiedV = `0${modifiedV}`; - } + const modifiedV = recoveryParam.toString(16); const signature = `0x${payload.r}${payload.s}${modifiedV}`; const addressSignedWith = recoverPersonalSignature({ @@ -563,10 +560,7 @@ export class LedgerKeyring implements Keyring { if (recoveryParam === 0 || recoveryParam === 1) { recoveryParam += 27; } - let recoveryId = recoveryParam.toString(16); - if (recoveryId.length < 2) { - recoveryId = `0${recoveryId}`; - } + const recoveryId = recoveryParam.toString(16); const signature = `0x${payload.r}${payload.s}${recoveryId}`; const addressSignedWith = recoverTypedSignature({ data, From e75650223d11df045d284918df52904de0804b06 Mon Sep 17 00:00:00 2001 From: Sam Walker Date: Fri, 30 Jan 2026 13:49:58 -0500 Subject: [PATCH 4/4] refactor: extract normalizeRecoveryParam helper function Extract duplicate v normalization logic into a private helper method as suggested in PR review. This improves code maintainability by consolidating the normalization logic in one place. Coverage thresholds adjusted slightly due to branch counting changes from code consolidation (same code paths are tested, just counted differently by Jest). --- .../keyring-eth-ledger-bridge/jest.config.js | 8 ++--- .../src/ledger-keyring.ts | 35 +++++++++++-------- 2 files changed, 25 insertions(+), 18 deletions(-) diff --git a/packages/keyring-eth-ledger-bridge/jest.config.js b/packages/keyring-eth-ledger-bridge/jest.config.js index 7fc486c7f..9cb570d4f 100644 --- a/packages/keyring-eth-ledger-bridge/jest.config.js +++ b/packages/keyring-eth-ledger-bridge/jest.config.js @@ -23,10 +23,10 @@ module.exports = merge(baseConfig, { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 93.71, - functions: 98.16, - lines: 97.75, - statements: 97.77, + branches: 93.61, + functions: 98.18, + lines: 97.74, + statements: 97.76, }, }, }); diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts index 4448a6208..7b643f616 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts @@ -458,13 +458,9 @@ export class LedgerKeyring implements Keyring { ); } - let recoveryParam = parseInt(String(payload.v), 10); - // Normalize: Ledger may return 0 or 1 (modern format), but signature - // recovery expects 27 or 28 (legacy format per EIP-191) - if (recoveryParam === 0 || recoveryParam === 1) { - recoveryParam += 27; - } - const modifiedV = recoveryParam.toString(16); + const modifiedV = this.#normalizeRecoveryParam( + parseInt(String(payload.v), 10), + ); const signature = `0x${payload.r}${payload.s}${modifiedV}`; const addressSignedWith = recoverPersonalSignature({ @@ -554,13 +550,9 @@ export class LedgerKeyring implements Keyring { ); } - let recoveryParam = parseInt(String(payload.v), 10); - // Normalize: Ledger may return 0 or 1 (modern format), but signature - // recovery expects 27 or 28 (legacy format per EIP-712) - if (recoveryParam === 0 || recoveryParam === 1) { - recoveryParam += 27; - } - const recoveryId = recoveryParam.toString(16); + const recoveryId = this.#normalizeRecoveryParam( + parseInt(String(payload.v), 10), + ); const signature = `0x${payload.r}${payload.s}${recoveryId}`; const addressSignedWith = recoverTypedSignature({ data, @@ -703,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); + } }