Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: Ledger Devices signature miss match after Version 12.11.0 #30473

Open
bangjelkoski opened this issue Feb 20, 2025 · 15 comments · May be fixed by #31313
Open

[Bug]: Ledger Devices signature miss match after Version 12.11.0 #30473

bangjelkoski opened this issue Feb 20, 2025 · 15 comments · May be fixed by #31313
Assignees
Labels
external-contributor regression-prod-12.11.0 Regression bug that was found in production in release 12.11.0 Sev2-normal Normal severity; minor loss of service or inconvenience. team-hardware-wallets type-bug Something isn't working

Comments

@bangjelkoski
Copy link

Describe the bug

After the recent update, EIP712, which gets signed on Ledger through Metamask, has a signature mismatch issue.

Expected behavior

Before this upgrade, no issues were encountered with signing EIP712 through Metamask with Ledger devices. Using Ledger directly also has no issues.

Screenshots/Recordings

No response

Steps to reproduce

  1. Go to https://helixapp.com
  2. Try to sign any transaction when Metamask has a Ledger device selected,

Here is a JSON that you can also use to try in unit tests:

{"types":{"EIP712Domain":[{"name":"name","type":"string"},{"name":"version","type":"string"},{"name":"chainId","type":"uint256"},{"name":"verifyingContract","type":"address"},{"name":"salt","type":"string"}],"Tx":[{"name":"context","type":"string"},{"name":"msgs","type":"string"}]},"primaryType":"Tx","domain":{"name":"Injective Web3","version":"1.0.0","chainId":"0x1","verifyingContract":"0xCcCCccccCCCCcCCCCCCcCcCccCcCCCcCcccccccC","salt":"0"},"message":{"context":"{\"account_number\":28013,\"chain_id\":\"injective-1\",\"fee\":{\"amount\":[{\"denom\":\"inj\",\"amount\":\"105102000000000\"}],\"gas\":210204,\"payer\":\"inj1065f86fh88ptyrg8h5048zu0vyx7ex8ymwgr6h\"},\"memo\":\"\",\"sequence\":1096,\"timeout_height\":106702014}","msgs":"[{\"@type\":\"/injective.exchange.v1beta1.MsgCreateDerivativeLimitOrder\",\"sender\":\"inj1xpc7l2k9l7mqyctdqjpt37wwfc7gqj7nt3vhy4\",\"order\":{\"market_id\":\"0x4ca0f92fc28be0c9761326016b5a1a2177dd6375558365116b5bdda9abc229ce\",\"order_info\":{\"subaccount_id\":\"0x3071efaac5ffb602616d0482b8f9ce4e3c804bd3000000000000000000000000\",\"fee_recipient\":\"inj1jv65s3grqf6v6jl3dp4t6c9t9rk99cd8dkncm8\",\"price\":\"92500000000.000000000000000000\",\"quantity\":\"0.001000000000000000\",\"cid\":\"\"},\"order_type\":\"BUY\",\"margin\":\"92500000.000000000000000000\",\"trigger_price\":\"0.000000000000000000\"}}]"}}

Error messages or log output

Error thrown in the console


{
    "code": -32603,
    "message": "Keyring Controller signTypedMessage: Error: Ledger: The signature doesnt match the right address",
    "data": {
        "cause": {
            "stack": "Error: Keyring Controller signTypedMessage: Error: Ledger: The signature doesnt match the right address\n  at me.signTypedMessage (chrome-extension://nkbihfbeogaeaoehlefnkodbefgpgknn/common-1.js:1:480764)\n  at async L.d (chrome-extension://nkbihfbeogaeaoehlefnkodbefgpgknn/common-2.js:5:10943)\n  at async L.m (chrome-extension://nkbihfbeogaeaoehlefnkodbefgpgknn/common-2.js:5:10522)\n  at async L.c (chrome-extension://nkbihfbeogaeaoehlefnkodbefgpgknn/common-2.js:5:8635)\n  at async s (chrome-extension://nkbihfbeogaeaoehlefnkodbefgpgknn/background-0.js:1:31933)\n  at async chrome-extension://nkbihfbeogaeaoehlefnkodbefgpgknn/common-0.js:1:522117\n  at async chrome-extension://nkbihfbeogaeaoehlefnkodbefgpgknn/common-1.js:1:96936",
            "message": "Keyring Controller signTypedMessage: Error: Ledger: The signature doesnt match the right address"
        }
    }
}

Detection stage

In production (default)

Version

12.11.0

Build type

None

Browser

Chrome

Operating system

MacOS

Hardware wallet

No response

Additional context

No response

Severity

No response

@bangjelkoski bangjelkoski added the type-bug Something isn't working label Feb 20, 2025
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by severity Feb 20, 2025
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by team Feb 20, 2025
@metamaskbot metamaskbot added external-contributor regression-prod-12.11.0 Regression bug that was found in production in release 12.11.0 labels Feb 20, 2025
@sleepytanya sleepytanya added team-confirmations Push issues to confirmations team Sev2-normal Normal severity; minor loss of service or inconvenience. labels Feb 21, 2025
@vinistevam vinistevam added team-hardware-wallets and removed team-confirmations Push issues to confirmations team labels Feb 24, 2025
@vivek-consensys
Copy link

When swapping USDT to INJ, I am receiving another error on console.
bugsnag.js:2401 MetamaskException: Please connect directly with Ledger instead through to sign this transaction | {"type":"wallet-error","code":-1,"errorClass":"MetamaskException","message":"Please connect directly with Ledger instead through to sign this transaction","contextModule":"sign-transaction","originalMessage":"Please connect directly with Ledger instead through Metamask to sign this transaction","stack":["Error: Please connect directly with Ledger instead through Metamask to sign this transaction","at Qce.signEip712TypedData (https://helixapp.com/_nuxt/D7Y9Ii6W.js:60:196339)","at async W2e.broadcastWeb3WithFeeDelegation (https://helixapp.com/_nuxt/D7Y9Ii6W.js:3:18848)","at async W2e.broadcastWithFeeDelegation (https://helixapp.com/_nuxt/D7Y9Ii6W.js:3:15546)","at async Proxy.broadcastWithFeeDelegation (https://helixapp.com/_nuxt/B9etIFFg.js:8:4427643)","at async Proxy.X (https://helixapp.com/_nuxt/ZWJBKkSg.js:1:1892)"]} at Qce.signEip712TypedData (https://helixapp.com/_nuxt/D7Y9Ii6W.js:60:196339) at async W2e.broadcastWeb3WithFeeDelegation (https://helixapp.com/_nuxt/D7Y9Ii6W.js:3:18848) at async W2e.broadcastWithFeeDelegation (https://helixapp.com/_nuxt/D7Y9Ii6W.js:3:15546) at async Proxy.broadcastWithFeeDelegation (https://helixapp.com/_nuxt/B9etIFFg.js:8:4427643) at async Proxy.X (https://helixapp.com/_nuxt/ZWJBKkSg.js:1:1892)

Private Zenhub Video

@bangjelkoski
Copy link
Author

bangjelkoski commented Feb 25, 2025

When swapping USDT to INJ, I am receiving another error on console. bugsnag.js:2401 MetamaskException: Please connect directly with Ledger instead through to sign this transaction | {"type":"wallet-error","code":-1,"errorClass":"MetamaskException","message":"Please connect directly with Ledger instead through to sign this transaction","contextModule":"sign-transaction","originalMessage":"Please connect directly with Ledger instead through Metamask to sign this transaction","stack":["Error: Please connect directly with Ledger instead through Metamask to sign this transaction","at Qce.signEip712TypedData (https://helixapp.com/_nuxt/D7Y9Ii6W.js:60:196339)","at async W2e.broadcastWeb3WithFeeDelegation (https://helixapp.com/_nuxt/D7Y9Ii6W.js:3:18848)","at async W2e.broadcastWithFeeDelegation (https://helixapp.com/_nuxt/D7Y9Ii6W.js:3:15546)","at async Proxy.broadcastWithFeeDelegation (https://helixapp.com/_nuxt/B9etIFFg.js:8:4427643)","at async Proxy.X (https://helixapp.com/_nuxt/ZWJBKkSg.js:1:1892)"]} at Qce.signEip712TypedData (https://helixapp.com/_nuxt/D7Y9Ii6W.js:60:196339) at async W2e.broadcastWeb3WithFeeDelegation (https://helixapp.com/_nuxt/D7Y9Ii6W.js:3:18848) at async W2e.broadcastWithFeeDelegation (https://helixapp.com/_nuxt/D7Y9Ii6W.js:3:15546) at async Proxy.broadcastWithFeeDelegation (https://helixapp.com/_nuxt/B9etIFFg.js:8:4427643) at async Proxy.X (https://helixapp.com/_nuxt/ZWJBKkSg.js:1:1892)

Private Zenhub Video

Yes, we had to do this because signing using Ledger through Metamask doesn't work after the upgrade. If you want, I can also redeploy the product with logging for the original error.

@bangjelkoski
Copy link
Author

Any updates here?

@dawnseeker8
Copy link
Contributor

The error for
| Keyring Controller signTypedMessage: Error: Ledger: The signature doesnt match the right address

will happen only when your account address connected to wrong ledger devices, this issue most likely happen to multiple ledger devices connection issue

@bangjelkoski
Copy link
Author

The error for | Keyring Controller signTypedMessage: Error: Ledger: The signature doesnt match the right address

will happen only when your account address connected to wrong ledger devices, this issue most likely happen to multiple ledger devices connection issue

What does this mean? I'm sure the address connected on Metamask and the one that I request signature for are the same

@dawnseeker8
Copy link
Contributor

hi, @bangjelkoski I just check whether account your dapp connect to was from other ledger device or not. since we face a multiple ledger device support when user import multiple ledger accounts from more than two ledger devices, and we have enhance request logged to fix this issue.

Currently i am trying to replicate your your issue.

@dawnseeker8
Copy link
Contributor

Yes, we had to do this because signing using Ledger through Metamask doesn't work after the upgrade. If you want, I can also redeploy the product with logging for the original error.

HI, @bangjelkoski can you redeploy the product so that i can try to replicate the Error: Ledger: The signature doesnt match the right address. the error message MetamaskException: Please connect directly with Ledger instead through to sign this transaction look like not from our metamask code base and ledger code base.

@bangjelkoski
Copy link
Author

I've deployed a simple UI that you can use to replicate the error. It's basically a simple transfer between two wallets https://injective-mm-ledger-bug.netlify.app/

If you need some INJ on mainnet to test, let me know.

@quanglefed
Copy link

I’ve looked into this issue and found that removing the salt in EIP712Domain makes Metamask work. However, without the salt, we risk two issues: Replay Attacks and Collisions.

btw, this is a Metamask issue that has occurred in the past few weeks, and I’m not sure why.

@dawnseeker8
Copy link
Contributor

I’ve looked into this issue and found that removing the salt in EIP712Domain makes Metamask work. However, without the salt, we risk two issues: Replay Attacks and Collisions.

btw, this is a Metamask issue that has occurred in the past few weeks, and I’m not sure why.

Recently we include the ledger clear signing support which ledger team ask us to support. thank you for sharing, @quanglefed i will have a look and debug the code using @bangjelkoski dapp.

@bangjelkoski can you send me some INJ on mainnet to this ledger address 0xBf6916f52edC86D38Da09579936a2fDfa774f21B

thank you very much for your help.

@bangjelkoski
Copy link
Author

@dawnseeker8
Copy link
Contributor

hi, @bangjelkoski thank you fund and i have debugged the metamask extension code with your dapp. and has found out the root cause of the issue. and i have make a fix in one of our metamask library for ledger. here is PR

I have tested the code change and confirm it will work with your dapp with domain salt value. here is some transaction details confirm i have sent 0.001 INJ to my another ledger account. https://injscan.com/transaction/740492AF58317B0E6B546ED119C85941277380FDBBAF3B0BB2786BC8370CE94A/

i will talk to my manager to priority this so that the fix can be included in to next possible release as soon as possible.

@bangjelkoski
Copy link
Author

This is great news; thank you!

@quanglefed
Copy link

thanks @dawnseeker8, please ensure the domain salt value is added in tests and test-dapp for future compatibility.

@bangjelkoski
Copy link
Author

Any updates here?

github-merge-queue bot pushed a commit to MetaMask/accounts that referenced this issue Apr 1, 2025
… for `signTypedData<V4>` (#249)

This PR is to fix the extension bug
[30473](MetaMask/metamask-extension#30473)

Basically the issue was caused by domain salt was defined as string in
client DApp, and @metamask/eth-sig-util library we used define salt as
arrayType. which cause miss match with previous code of salt value.

This code has been tested with client dapp and our e2e test-dapp to
confirm it will work for both.

<!--
Thanks for your contribution! Take a moment to answer these questions so
that reviewers have the information they need to properly understand
your changes:

* What is the current state of things and why does it need to change?
* What is the solution your changes offer and how does it work?

Are there any issues or other links reviewers should consult to
understand this pull request better? For instance:

* Fixes
[#30473](MetaMask/metamask-extension#30473)
* See: #67890
-->

## Examples

<!--
Are there any examples of this change being used in another repository?

When considering changes to the MetaMask module template, it's strongly
preferred that the change be experimented with in another repository
first. This gives reviewers a better sense of how the change works,
making it less likely the change will need to be reverted or adjusted
later.
-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external-contributor regression-prod-12.11.0 Regression bug that was found in production in release 12.11.0 Sev2-normal Normal severity; minor loss of service or inconvenience. team-hardware-wallets type-bug Something isn't working
Projects
Status: To be fixed
Status: To be triaged
7 participants