fix(ISA-2025-003): Malicious validator can spoof votes from other val…#107
Conversation
…idators Port-of: GHSA-6jrf-4jv4-r9mw Closes #106
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a vulnerability where a malicious validator could spoof votes by intentionally duplicating validator addresses. The changes include adding new tests in the light client verifier for duplicate votes, updating voting power computation to detect duplicate validator votes, and enhancing validator address verification logic in the cometbft module.
- Added a test in light-client-verifier/src/verifier.rs to simulate and detect duplicate votes.
- Modified the has_voted interface and voting power tally in light-client-verifier/src/operations/voting_power.rs to ensure duplicate votes trigger an error.
- Updated validator and account modules in cometbft to enforce stricter validation of validator addresses.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| light-client-verifier/src/verifier.rs | Added tests to validate detection of duplicate votes in updates. |
| light-client-verifier/src/operations/voting_power.rs | Modified vote tracking to return an Option and check for duplicates. |
| light-client-verifier/Cargo.toml | Added serde_json dependency for JSON roundtrip tests. |
| cometbft/src/validator.rs | Exposed fields and enhanced validator address verification. |
| cometbft/src/account.rs | Updated conversion logic for validator public keys and IDs. |
| cometbft/Cargo.toml | Adjusted dependency configuration for sha2. |
| .changelog/unreleased/bug-fixes/2025-003-check-duplicate-votes.md | Documented the bug fix regarding duplicate votes. |
| total_voting_power: u64, | ||
| ) -> Result<VotingPowerTally, VerificationError> { | ||
| let mut power = VotingPowerTally::new(total_voting_power, trust_threshold); | ||
| let mut seen_vals = Vec::new(); |
There was a problem hiding this comment.
[nitpick] Using a Vec and calling contains() for each duplicate check results in O(n) complexity per lookup. Consider using a HashSet to achieve constant time lookups if the validator set might grow large.
There was a problem hiding this comment.
Vec is fast enough for our purposes: https://codspeed.io/chat/fd5a5ce3-aa3c-4b9e-8ba0-f3ac8ed18ef6
| clock = ["time/std"] | ||
| secp256k1 = ["rust-crypto", "dep:k256", "dep:ripemd"] | ||
| rust-crypto = ["dep:sha2", "dep:ed25519-consensus"] | ||
| rust-crypto = ["dep:ed25519-consensus"] |
There was a problem hiding this comment.
maybe we should drop rust-crypto flag entirely?
There was a problem hiding this comment.
yes but let's do it in a separate PR
| use digest::Digest; | ||
| use sha2::Sha256; | ||
|
|
||
| pub fn try_from_type_and_bytes(pub_key_type: &str, pub_key_bytes: &[u8]) -> Result<Id, Error> { |
There was a problem hiding this comment.
I wonder if we should have an intermediate struct like
type PubKeyTypeAndBytes struct {
pub t: String,
pub bz: Vec<u8>,
}and use it to implement impl TryFrom<PubKeyTypeAndBytes> for Id {
There was a problem hiding this comment.
we could but since we only use try_from_type_and_bytes in TryFrom<RawValidator> for Info at the moment we would not gain much
There was a problem hiding this comment.
we can still do this in a follow-up PR if you feel it's cleaner
|
@romac would be great if you found some time to review this 🙏 |
…idators
Port-of: GHSA-6jrf-4jv4-r9mw
Closes #106