fix(keyring-eth-ledger-bridge): normalize signature v value for proper recovery

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.
 

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,
     },
   },

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',
         );
       });
 

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);
+  }
 }