-
Notifications
You must be signed in to change notification settings - Fork 150
fix(eip-712)!: encoding was using default evm chain id instead of actual chain id #918
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #918 +/- ##
==========================================
- Coverage 65.49% 65.49% -0.01%
==========================================
Files 318 318
Lines 22222 22232 +10
==========================================
+ Hits 14555 14561 +6
- Misses 6502 6504 +2
- Partials 1165 1167 +2
🚀 New features to boost your workflow:
|
|
I've been investigating an error that's similar when transacting over IBC using EOA (Keplr/Leap), but the IBC via CLI works fine, This error was introduced in either v0.5.0 or v0.5.1 (Recently upgraded from v0.4.2) Update: I applied this change to my RPC node, and I got the same error, so the IBC error doesn't seem to be related. |
|
@MudDev This does not get applied to the RPC node (it can but it doesn't affect it). it has to do with the signer when encoding. So the bug is within keplr / leap on signing with the wrong chain id. The chain id refactor in 0.5 has caused so many issues |
|
Looks good - can we get some tests for this functionality? |
|
Agreed with @aljo242. It would be useful to have a happy path systemtest in the eip712 tests to make sure that this doesn't break again in the future. |
|
I tested on a 4x validator cluster and I don't feel like writing any unit tests for this. Just contributing the patch to fix what was broken in 0.5 refactor |
tldr; cosmos multisigs with ledgers break when signing due to the EIP-712 not taking into account the chain-id. Tested and this is breaking, required on the nodes
Description
Ledger multisig transactions using EIP-712 signing failed on-chain verification with
signature verification failed: unauthorized, even though local validate-signatures succeeded. This does not allow a ledger to be apart of a multisig whenWithKeyringOptions(evmkeyring.Option()).is used (CoinType 60 / EthSecpK1)The EIP-712 encoding uses a global eip155ChainID variable (262144) that is set during app initialization via encoding.MakeConfig(evmChainID). When CLI commands like tx multi-sign or tx validate-signatures run, they create a temporary app with simtestutil.EmptyAppOptions{}, which causes the EVM chain ID to default to 262144 instead of the chain's actual EVM chain ID.
This meant:
Testing
Before this fix it was impossible to submit any signature from a ledger (it works just fine for hotwallets though) via the multisig.
I have...
mainbranch