wevm / viem

TypeScript Interface for Ethereum
https://viem.sh
Other
2.52k stars 815 forks source link

bug: Invalid signature when signing with hardware wallet #537

Closed wyze closed 1 year ago

wyze commented 1 year ago

Is there an existing issue for this?

Package Version

0.3.30

Current Behavior

Signature is invalid.

Expected Behavior

Signature is valid.

Steps To Reproduce

  1. Connect Wallet via MetaMask

  2. Sign message

  3. Valid: Yes

  4. Connect Hardware (I used Ledger) Wallet via Metamask

  5. Sign message

  6. Valid: No

Link to Minimal Reproducible Example (StackBlitz, CodeSandbox, GitHub repo etc.)

https://github.com/wyze/viem-verify-sign-issue

Anything else?

It works as expected when using a regular MetaMask wallet. When I try to sign a message using my Ledger via MetaMask, the signature comes back invalid.

jxom commented 1 year ago

Can you confirm, does this also happen using the verifyMessage util (not the public client one)?

wyze commented 1 year ago

Thanks for the quick response! It does not work with hardware wallet, but here is the error that happens:

Error: recovery id invalid
    at Signature.recoverPublicKey (http://localhost:3007/build/_shared/secp256k1-UQXZ4KZU.js:1287:15)
    at recoverPublicKey (http://localhost:3007/build/routes/_index-YZOSDE5U.js:3614:108)
    at async recoverAddress (http://localhost:3007/build/routes/_index-YZOSDE5U.js:3620:29)
    at async verifyMessage (http://localhost:3007/build/routes/_index-YZOSDE5U.js:3662:46)
    at async onClick (http://localhost:3007/build/routes/_index-YZOSDE5U.js:6570:28)

It does still work through a MetaMask account (non-hardware wallet).

I've pushed updated code to my repo.

jxom commented 1 year ago

Hmm ok interesting, let me do some digging

wyze commented 1 year ago

I did some digging as well, and seeing how ethers has handled it in the past. The recovery bit from the hardware wallet is coming in as 0, so its passed in as -27, where as from a regular MetaMask wallet the bit is b0 (27), so it gets passed in as 0.

https://github.com/wagmi-dev/viem/blob/55ca10ff918a68954e2be84b956dd19c7f035b30/src/utils/signature/recoverPublicKey.ts#LL27C28-L27C28

On that line above I updated it like so:

  const publicKey = secp256k1.Signature.fromCompact(
    signatureHex.substring(2, 130),
  )
-    .addRecoveryBit(v - 27)
+    .addRecoveryBit(v < 27 ? v : v - 27)
    .recoverPublicKey(hashHex.substring(2))
    .toHex(false)

Based on the logic found here: https://github.com/ethers-io/ethers.js/blob/bf65ddbff0036f6eb8e99c145f30edff157687f5/packages/bytes/src.ts/index.ts#L343-L350

So this would make the verifyMessage util say Yes (valid), but still fails on the publicClient.verifyMessage.

jxom commented 1 year ago

Added a fix in #545, and added you as a co-author of the commit.

Thanks!

github-actions[bot] commented 4 months ago

This issue has been locked since it has been closed for more than 14 days.

If you found a concrete bug or regression related to it, please open a new bug report with a reproduction against the latest Viem version. If you have any questions or comments you can create a new discussion thread.