Fix three critical security vulnerabilities in bridge, dispute, and oracle attestation#1040
Open
Mayne-X wants to merge 1 commit into
Open
Fix three critical security vulnerabilities in bridge, dispute, and oracle attestation#1040Mayne-X wants to merge 1 commit into
Mayne-X wants to merge 1 commit into
Conversation
1. SHA256→Keccak256 in initial message signing (C-1)
- Changed SignInitialMessage and EVMAddressFromSignatures to use
Keccak256 for the inner hash, matching the bridge contract's
SHA256(Keccak256(...)) signature scheme
2. PrevDisputeIds append wrong ID (C-4)
- Append prevDisputeId instead of disputeId to prevent
double-counting votes in dispute tally rounds
3. Missing signature verification in SetOracleAttestation (C-5)
- Added ECDSA recover verification to ensure attestation
signatures match the validator's registered EVM address
before storing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix 1: SHA256 → Keccak256 hash mismatch in bridge signing (app/extend_vote.go, x/bridge/keeper/keeper.go)
The initial message signing (SignInitialMessage) and EVM address recovery (EVMAddressFromSignatures) used SHA256 for the inner message hash. Changed to crypto.Keccak256 to align with Ethereum conventions and the bridge contract's _verifySig scheme, which uses sha256(abi.encodePacked(keccak256(data))). The signed content is now SHA256(Keccak256(message)) on both the Cosmos signing side and the bridge verification side, ensuring cross-chain signature compatibility.
Fix 2: PrevDisputeIds appends wrong ID — vote double-counting (x/dispute/keeper/dispute.go:283)
AddDisputeRound appended disputeId (the new round's ID) to PrevDisputeIds instead of prevDisputeId (the previous round's ID). This caused GetSumOfUserAndReporterVotesAllRounds to double-count votes from the current round — once from direct VoteCountsByGroup lookup and again from PrevDisputeIds iteration, inflating vote power and corrupting dispute outcomes.
Fix 3: Missing signature verification in SetOracleAttestation (x/bridge/keeper/keeper.go:812-853)
SetOracleAttestation stored attestation signatures from vote extensions without verifying they were actually produced by the claimed validator's key. Added ECDSA recovery verification: computes SHA256(snapshot) (the effective signed hash after keyring auto-hash), tries both recovery IDs via crypto.SigToPub, and compares the recovered EVM address against the validator's registered address. Fake attestations are now rejected with an error instead of being blindly stored.