Remove aggregated signature verification bypass for small proof_data

[pablodeymo]

Motivation

During a security review, we identified that verify_aggregated_signature() in crates/common/crypto/src/lib.rs silently returned Ok(()) for any proof_data under 10 bytes — including empty proofs — without performing any cryptographic verification.

This was reachable from two production P2P network paths:

1. Gossip aggregated attestations (store.rs:459) — any peer can send a SignedAggregatedAttestation with empty proof_data. No proposer key required.
2. Block attestation signatures (store.rs:1197) — a malicious proposer can include attestations with fabricated aggregation_bits and empty proof_data.

A malicious peer could forge attestation votes to manipulate fork choice (LMD-GHOST head selection) and influence justification/finalization thresholds.

Note: proposer individual XMSS signatures are NOT affected — they use ValidatorSignature::is_valid(), a separate code path with no size-based bypass.

Origin of the bypass

AggregatedSignatureProof::empty() creates proofs with empty proof_data for the verify=false test path. The < 10 guard was likely added to prevent SSZ deserialization errors when these empty proofs reached the verifier during development. However, the verify=false path never calls verify_signatures, so the guard was unnecessary — and it converted what should be an error into a silent success on the production verify=true path.

Description

Remove the early-return bypass in verify_aggregated_signature():

// REMOVED:
if proof_data.len() < 10 {
    return Ok(());
}

Without this guard, empty/small proof_data will naturally fail at the SSZ deserialization step (Devnet2XmssAggregateSignature::from_ssz_bytes) and return VerificationError::DeserializationFailed.

How to test

All spec tests pass without the bypass:

cargo test -p ethlambda-blockchain --test forkchoice_spectests --release     # 26/26 passed
cargo test -p ethlambda-state-transition --test stf_spectests --release      # 14/14 passed
cargo test -p ethlambda-blockchain --test signature_spectests --release      #  8/8 passed

Related

• StoreError::ParticipantsMismatch is defined at store.rs:899 but never raised. Could be addressed in a follow-up.

[github-actions]

🤖 Kimi Code Review

Security Vulnerability: Signature Bypass

File: crates/common/crypto/src/lib.rs
Lines: 143-146

The removal of the length check if proof_data.len() < 10 { return Ok(()); } introduces a critical security vulnerability. This check was likely serving as a guard against malformed or empty signature data that could otherwise pass verification incorrectly.

Impact: Without this check, empty or very short proof_data could potentially be accepted as valid aggregated signatures, allowing malicious blocks or attestations to bypass signature verification entirely.

Recommendation: Restore the length check or verify its necessity. If the check was overly permissive, replace it with a more precise validation (e.g., checking for exact expected lengths based on XMSS parameters). The magic number 10 should be replaced with a named constant explaining the minimum valid signature size.

Additional concern: The comment // Convert public keys on line 148 is misleading - this section converts public keys to the internal LeanSigPubKey format, not the signature data itself.

---

Automated review by Kimi (Moonshot AI) · custom prompt

[github-actions]

🤖 Codex Code Review

1. No blocking findings in this PR. The change removes a critical verification bypass in verify_aggregated_signature where proof_data.len() < 10 previously returned Ok(()), allowing malformed/forged aggregated proofs to be accepted without cryptographic checks. The new flow always deserializes + verifies, which is the correct security posture for consensus signatures (lib.rs).

2. Non-blocking improvement: add a regression test that passes a deliberately short/garbage proof_data and asserts Err(VerificationError::DeserializationFailed) (or equivalent failure), near the existing verification tests (lib.rs). This protects against accidental reintroduction of the bypass.

3. Consensus-layer scope check: this PR does not alter fork-choice, justification/finalization, slot/block transition, or SSZ/XMSS aggregation logic itself; it hardens the signature verification gate used by attestation processing paths.

---

Automated review by OpenAI Codex · custom prompt

[github-actions]

🤖 Claude Code Review

Now I have all the context I need to write the review.

---

Review: Remove aggregated signature verification bypass for small proof_data

Summary

A minimal, surgical fix removing a critical security bypass in verify_aggregated_signature(). The change is correct and well-motivated.

---

Security Impact

The removed guard was a critical vulnerability on two production P2P paths:

• store.rs:459 — gossip attestations: any peer could send a SignedAggregatedAttestation with 0–9 bytes of proof_data and have it accepted as valid
• store.rs:1197 — block attestation verification: a malicious proposer could build blocks with fabricated aggregation_bits and empty proof_data, forging arbitrary validator votes

Both paths feed directly into fork choice weight updates and the 3SF-mini justification/finalization thresholds. The attacker surface was unauthenticated (gossip), so no validator key was needed to exploit it.

The fix is correct: removing the guard causes Devnet2XmssAggregateSignature::from_ssz_bytes to fail on empty/undersized data, returning VerificationError::DeserializationFailed, matching Zeam/Ream behavior.

---

Code Review

crates/common/crypto/src/lib.rs:140–142 (removed)

if proof_data.len() < 10 {
    return Ok(());
}

Removal is correct. The verify=false test path that creates AggregatedSignatureProof::empty() (store.rs:632) never calls verify_signatures, so there is no regression risk. All 48 spec tests confirm this.

store.rs:1177–1197 — verify_signatures (unchanged, but worth noting)

The function correctly derives public_keys from attestation.aggregation_bits, not from aggregated_proof.participants. This means a malicious proposer could still submit a block where aggregated_proof.participants lists different validators than attestation.aggregation_bits — the proof would be verified against the wrong set of public keys and likely fail, but the mismatch itself is not explicitly rejected. StoreError::ParticipantsMismatch is defined at line 899 but never raised. Zeam and Lantern both enforce this check. The PR body flags this as a follow-up; it should not block merging, but should be tracked.

store.rs:459 — gossip path

ethlambda_crypto::verify_aggregated_signature(
    &aggregated.proof.proof_data,
    ...
)

After this fix, an empty proof_data will correctly return Err(VerificationError::DeserializationFailed). The calling code should propagate this as a protocol violation. Worth confirming the gossip path rejects (and optionally penalizes/disconnects) the sending peer rather than silently dropping the attestation — but that is pre-existing behavior, not introduced by this PR.

---

Test Coverage

The three spec test suites (forkchoice: 26/26, stf: 14/14, signature: 8/8) adequately cover the removal. There is no fast (non-#[ignore]) unit test asserting that verify_aggregated_signature returns Err on empty input. Consider adding one:

#[test]
fn test_empty_proof_data_is_rejected() {
    let empty = ByteListMiB::new(vec![]).unwrap();
    let result = verify_aggregated_signature(&empty, vec![], &H256::ZERO, 0);
    assert!(result.is_err(), "empty proof_data must not verify");
}

This would act as a regression guard without requiring leanVM and wouldn't need #[ignore].

---

Verdict

Approve. The fix is minimal, correct, and closes a critical unauthenticated bypass. The follow-up items (ParticipantsMismatch enforcement, fast regression test) are non-blocking but worth tracking.

---

Automated review by Claude (Anthropic) · custom prompt

[greptile-apps]

Greptile Summary

This PR removes a 4-line early-return bypass in verify_aggregated_signature() that silently returned Ok(()) for any proof_data shorter than 10 bytes — including empty proofs — without performing any cryptographic verification. The fix closes a security vulnerability reachable from two production P2P paths (gossip aggregated attestations and block attestation signatures), where a malicious peer could forge attestation votes to manipulate fork-choice head selection and influence justification/finalization thresholds.

Key points:

• The fix is minimal, correct, and well-motivated: removing the guard means short/empty proof_data now fails at SSZ deserialization (Devnet2XmssAggregateSignature::from_ssz_bytes) and returns VerificationError::DeserializationFailed, consistent with Ream and Zeam behavior.
• The verify=false test path (which uses AggregatedSignatureProof::empty()) never calls verify_signatures, so the bypass was always unnecessary on that path.
• All 48 spec tests (forkchoice, state-transition, signature) pass without the bypass, confirming no legitimate code path depended on it.
• No dedicated regression test exists to prevent reintroduction of this bypass; the existing verify_aggregated_signature tests are all marked #[ignore = "too slow"]. A fast unit test with empty/small proof_data would be a valuable follow-up.
• The PR also notes that StoreError::ParticipantsMismatch (defined at store.rs:899) is never raised; Zeam and Lantern enforce this check. This is deferred to a follow-up but worth tracking.

Confidence Score: 5/5

• This PR is safe to merge — it removes a clearly incorrect bypass with no legitimate production use, and all spec tests confirm correctness.
• The change is a well-scoped 4-line deletion of a verified security bypass. The PR description provides thorough cross-client comparison and root-cause analysis. All 48 spec tests pass. The only gap is a missing fast regression test, which is a style/best-practice concern rather than a blocking issue.
• No files require special attention; the single changed file is straightforward.

Important Files Changed

Filename	Overview
crates/common/crypto/src/lib.rs	Removes a 4-line early-return bypass that silently accepted any proof_data shorter than 10 bytes without cryptographic verification; empty/small proofs now correctly fail at SSZ deserialization. No regression test guards against reintroduction.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["P2P message received"] --> B{"verify == true?"}
    B -- No --> C["Skip verify_signatures\nEmpty proofs OK"]
    B -- Yes --> D["verify_signatures"]
    D --> E["verify_aggregated_signature\nproof_data, pubkeys, msg, slot"]

    E --> F{"BEFORE: proof_data.len() less than 10?"}
    F -- Yes --> G["return Ok - BYPASS\nNo crypto performed"]
    F -- No --> H["from_ssz_bytes deserialization"]

    E --> K["AFTER: from_ssz_bytes deserialization"]
    K -- "Fails for empty/short data" --> L["return Err DeserializationFailed"]
    K -- Succeeds --> M["xmss_verify_aggregated_signatures"]
    H --> M
    M --> N["return Ok"]

    style G fill:#ff4444,color:#fff
    style L fill:#44aa44,color:#fff
    style N fill:#44aa44,color:#fff

Loading

Comments Outside Diff (1)

1. crates/common/crypto/src/lib.rs, line 159 (link)

   Missing regression test for the removed bypass

   The test suite has no fast, non-ignored test verifying that empty or short proof_data now returns Err(VerificationError::DeserializationFailed). All the meaningful verify_aggregated_signature tests are marked #[ignore = "too slow"], which means CI won't catch a reintroduction of this bypass.

   Adding a dedicated unit test requires no XMSS key generation and would run in milliseconds:

   #[test]
   fn test_empty_proof_data_rejects_with_deserialization_error() {
       let empty_proof = ByteListMiB::new(vec![]).expect("empty list should be valid");
       let result = verify_aggregated_signature(&empty_proof, vec![], &H256::ZERO, 0);
       assert!(
           matches!(result, Err(VerificationError::DeserializationFailed)),
           "empty proof_data must be rejected, got: {:?}",
           result
       );
   }

   A complementary test with 1–9 bytes of junk data would also harden coverage of the formerly-bypassed range.

_{Last reviewed commit: ba28031}

[MegaRedHand]

Great catch!! 🎣