Verify proposer attestation identity matches block proposer#220
Verify proposer attestation identity matches block proposer#220pablodeymo merged 1 commit intomainfrom
Conversation
The spec (block.py:186) requires that proposer_attestation.validator_id equals block.proposer_index before verifying the proposer signature. Without this check, a malicious proposer could set a different validator_id in their attestation, which passes signature verification (keyed on proposer_index) but gets stored under the fake validator_id, hijacking that validator's fork choice vote.
🤖 Kimi Code ReviewReview SummaryThe PR adds a validation check for proposer attestation validator ID against block proposer index. This is a critical consensus-layer security fix that prevents malicious blocks from including attestations from non-proposer validators. Issues Found
Positive Aspects
Automated review by Kimi (Moonshot AI) · custom prompt |
🤖 Codex Code ReviewFindings (ordered by severity):
Overall: the core change looks correct and beneficial. The new check closes a meaningful validation gap by ensuring proposer attestation identity matches the block proposer before proposer signature verification. Automated review by OpenAI Codex · custom prompt |
🤖 Claude Code ReviewNow I have enough context for a thorough review. PR Review: Verify proposer attestation
|
Greptile SummaryThis PR closes a spec-compliance gap in Key changes:
Minor observations:
Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| crates/blockchain/src/store.rs | Adds a new ProposerAttestationMismatch error variant and inserts an identity-equality guard in verify_signatures() to ensure proposer_attestation.validator_id == block.proposer_index before proceeding with cryptographic verification. Fix is correct and well-placed; minor gaps around the absence of a dedicated unit test and a slightly ambiguous error-field name. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[on_block] --> B[on_block_core\nverify=true]
B --> C[verify_signatures]
C --> D{attestations.len ==\nsignatures.len?}
D -- No --> E[Err: AttestationSignatureMismatch]
D -- Yes --> F[For each attestation:\nverify aggregated sig]
F -- Invalid --> G[Err: AggregateVerificationFailed]
F -- All valid --> H{proposer_attestation.validator_id\n== block.proposer_index?}
H -- No --> I["Err: ProposerAttestationMismatch ✨ NEW"]
H -- Yes --> J[Decode proposer signature]
J --> K[Look up proposer pubkey\nvia block.proposer_index]
K --> L{proposer_signature.is_valid?}
L -- No --> M[Err: ProposerSignatureVerificationFailed]
L -- Yes --> N[Ok — proceed to\nstate_transition & store]
style I fill:#f90,color:#000
Prompt To Fix All With AI
This is a comment left during a code review.
Path: crates/blockchain/src/store.rs
Line: 1222-1227
Comment:
**No unit test for the new mismatch check**
The new validation path (`ProposerAttestationMismatch`) is security-critical but has no dedicated unit test. The existing module already has `verify_signatures_rejects_participants_mismatch` as a pattern to follow. Without a test here, a future refactor could accidentally remove or bypass this guard and no regression would catch it.
Consider adding a unit test analogous to the existing one, e.g.:
```rust
#[test]
fn verify_signatures_rejects_proposer_attestation_mismatch() {
let state = State::from_genesis(1000, vec![]);
let attestation_data = AttestationData {
slot: 0,
head: Checkpoint::default(),
target: Checkpoint::default(),
source: Checkpoint::default(),
};
// block.proposer_index = 0, but proposer_attestation.validator_id = 1 — mismatch
let signed_block = SignedBlockWithAttestation {
message: BlockWithAttestation {
block: Block {
slot: 0,
proposer_index: 0, // actual proposer
parent_root: H256::ZERO,
state_root: H256::ZERO,
body: BlockBody { attestations: AggregatedAttestations::new(vec![]).unwrap() },
},
proposer_attestation: Attestation {
validator_id: 1, // different validator — triggers the new guard
data: attestation_data,
},
},
signature: BlockSignatures {
attestation_signatures: ssz_types::VariableList::new(vec![]).unwrap(),
proposer_signature: ssz_types::FixedVector::from_elem(0),
},
};
let result = verify_signatures(&state, &signed_block);
assert!(
matches!(
result,
Err(StoreError::ProposerAttestationMismatch {
attestation_id: 1,
proposer_index: 0,
})
),
"Expected ProposerAttestationMismatch, got: {result:?}"
);
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: crates/blockchain/src/store.rs
Line: 913-919
Comment:
**Field name `attestation_id` is ambiguous**
The error message renders the field as `validator_id` (the description says "validator_id {attestation_id}"), but the struct field is named `attestation_id`. This slight mismatch between the struct field name and the error message text can make it harder to correlate a runtime error string back to the source struct when debugging.
A more self-documenting name would make the relationship explicit:
```suggestion
#[error(
"Proposer attestation validator_id {attestation_validator_id} does not match block proposer_index {proposer_index}"
)]
ProposerAttestationMismatch {
attestation_validator_id: u64,
proposer_index: u64,
},
```
(The construction site at line 1224 would need to update the field name accordingly.)
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: b788bd2
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if proposer_attestation.validator_id != block.proposer_index { | ||
| return Err(StoreError::ProposerAttestationMismatch { | ||
| attestation_id: proposer_attestation.validator_id, | ||
| proposer_index: block.proposer_index, | ||
| }); | ||
| } |
There was a problem hiding this comment.
No unit test for the new mismatch check
The new validation path (ProposerAttestationMismatch) is security-critical but has no dedicated unit test. The existing module already has verify_signatures_rejects_participants_mismatch as a pattern to follow. Without a test here, a future refactor could accidentally remove or bypass this guard and no regression would catch it.
Consider adding a unit test analogous to the existing one, e.g.:
#[test]
fn verify_signatures_rejects_proposer_attestation_mismatch() {
let state = State::from_genesis(1000, vec![]);
let attestation_data = AttestationData {
slot: 0,
head: Checkpoint::default(),
target: Checkpoint::default(),
source: Checkpoint::default(),
};
// block.proposer_index = 0, but proposer_attestation.validator_id = 1 — mismatch
let signed_block = SignedBlockWithAttestation {
message: BlockWithAttestation {
block: Block {
slot: 0,
proposer_index: 0, // actual proposer
parent_root: H256::ZERO,
state_root: H256::ZERO,
body: BlockBody { attestations: AggregatedAttestations::new(vec![]).unwrap() },
},
proposer_attestation: Attestation {
validator_id: 1, // different validator — triggers the new guard
data: attestation_data,
},
},
signature: BlockSignatures {
attestation_signatures: ssz_types::VariableList::new(vec![]).unwrap(),
proposer_signature: ssz_types::FixedVector::from_elem(0),
},
};
let result = verify_signatures(&state, &signed_block);
assert!(
matches!(
result,
Err(StoreError::ProposerAttestationMismatch {
attestation_id: 1,
proposer_index: 0,
})
),
"Expected ProposerAttestationMismatch, got: {result:?}"
);
}Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/blockchain/src/store.rs
Line: 1222-1227
Comment:
**No unit test for the new mismatch check**
The new validation path (`ProposerAttestationMismatch`) is security-critical but has no dedicated unit test. The existing module already has `verify_signatures_rejects_participants_mismatch` as a pattern to follow. Without a test here, a future refactor could accidentally remove or bypass this guard and no regression would catch it.
Consider adding a unit test analogous to the existing one, e.g.:
```rust
#[test]
fn verify_signatures_rejects_proposer_attestation_mismatch() {
let state = State::from_genesis(1000, vec![]);
let attestation_data = AttestationData {
slot: 0,
head: Checkpoint::default(),
target: Checkpoint::default(),
source: Checkpoint::default(),
};
// block.proposer_index = 0, but proposer_attestation.validator_id = 1 — mismatch
let signed_block = SignedBlockWithAttestation {
message: BlockWithAttestation {
block: Block {
slot: 0,
proposer_index: 0, // actual proposer
parent_root: H256::ZERO,
state_root: H256::ZERO,
body: BlockBody { attestations: AggregatedAttestations::new(vec![]).unwrap() },
},
proposer_attestation: Attestation {
validator_id: 1, // different validator — triggers the new guard
data: attestation_data,
},
},
signature: BlockSignatures {
attestation_signatures: ssz_types::VariableList::new(vec![]).unwrap(),
proposer_signature: ssz_types::FixedVector::from_elem(0),
},
};
let result = verify_signatures(&state, &signed_block);
assert!(
matches!(
result,
Err(StoreError::ProposerAttestationMismatch {
attestation_id: 1,
proposer_index: 0,
})
),
"Expected ProposerAttestationMismatch, got: {result:?}"
);
}
```
How can I resolve this? If you propose a fix, please make it concise.| #[error( | ||
| "Proposer attestation validator_id {attestation_id} does not match block proposer_index {proposer_index}" | ||
| )] | ||
| ProposerAttestationMismatch { | ||
| attestation_id: u64, | ||
| proposer_index: u64, | ||
| }, |
There was a problem hiding this comment.
Field name attestation_id is ambiguous
The error message renders the field as validator_id (the description says "validator_id {attestation_id}"), but the struct field is named attestation_id. This slight mismatch between the struct field name and the error message text can make it harder to correlate a runtime error string back to the source struct when debugging.
A more self-documenting name would make the relationship explicit:
| #[error( | |
| "Proposer attestation validator_id {attestation_id} does not match block proposer_index {proposer_index}" | |
| )] | |
| ProposerAttestationMismatch { | |
| attestation_id: u64, | |
| proposer_index: u64, | |
| }, | |
| #[error( | |
| "Proposer attestation validator_id {attestation_validator_id} does not match block proposer_index {proposer_index}" | |
| )] | |
| ProposerAttestationMismatch { | |
| attestation_validator_id: u64, | |
| proposer_index: u64, | |
| }, |
(The construction site at line 1224 would need to update the field name accordingly.)
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/blockchain/src/store.rs
Line: 913-919
Comment:
**Field name `attestation_id` is ambiguous**
The error message renders the field as `validator_id` (the description says "validator_id {attestation_id}"), but the struct field is named `attestation_id`. This slight mismatch between the struct field name and the error message text can make it harder to correlate a runtime error string back to the source struct when debugging.
A more self-documenting name would make the relationship explicit:
```suggestion
#[error(
"Proposer attestation validator_id {attestation_validator_id} does not match block proposer_index {proposer_index}"
)]
ProposerAttestationMismatch {
attestation_validator_id: u64,
proposer_index: u64,
},
```
(The construction site at line 1224 would need to update the field name accordingly.)
How can I resolve this? If you propose a fix, please make it concise.
2 participants
Motivation
A spec-to-code compliance audit identified a missing validation check in
verify_signatures(). The leanSpec reference implementation requires:Our implementation was missing this check.
The problem
In
verify_signatures(), the proposer's signature is verified using the public key looked up viablock.proposer_index(L1218-1224). This correctly proves the proposer signed the attestation, but it does not verify thatproposer_attestation.validator_idmatchesblock.proposer_index.Later, in
on_block_core()(L614), the attestation is stored using thevalidator_idfrom the attestation itself:This value is then used as the key when inserting the attestation into the fork choice store (L634 or L640-644).
Attack scenario
Without this check, a malicious proposer for slot N could:
block.proposer_index = 5(their real index, required for slot assignment)proposer_attestation.validator_id = 7(a different validator)What passes: Signature verification succeeds because it uses
block.proposer_index(5) to look up the key, and validator 5 did sign the attestation.What goes wrong: The attestation is stored under validator 7's identity in the fork choice latest-attestation map. This:
In a small validator set (e.g., 4-node devnet), this represents a 25% advantage per malicious proposal. In larger sets the impact shrinks proportionally.
Fix
One check added to
verify_signatures(), directly matching the spec assertion:Test plan
make fmtpassesmake lintpasses (clippy with-D warnings)make testpasses (all 112 tests, including forkchoice and signature spec tests)