fix: tighten attestation future-slot bound (leanSpec #682)

[MegaRedHand]

Summary

Ports leanSpec PR #682 — forkchoice: tighten validate_attestation future-slot bound.

The previous time check accepted votes up to a full slot ahead of the local clock. With 4 s slots that leaves ~3.2 s of free pre-positioning room: an adversary can publish next-slot aggregates before any honest validator could produce them, and the next proposer happily includes them.

The bound is now expressed in interval units against the store's local time and gated by a new GOSSIP_DISPARITY_INTERVALS = 1 constant — the lean analogue of mainnet's MAXIMUM_GOSSIP_CLOCK_DISPARITY (one ~800 ms interval).

Before / after

	Tolerance	Computation
Before	1 slot (~3.2 s)	data.slot > store.time() / INTERVALS_PER_SLOT + 1 rejects
After	1 interval (~800 ms)	data.slot * INTERVALS_PER_SLOT > store.time() + GOSSIP_DISPARITY_INTERVALS rejects

Changes

• crates/blockchain/src/lib.rs: add GOSSIP_DISPARITY_INTERVALS = 1 constant.
• crates/blockchain/src/store.rs: rewrite the time-check arm of validate_attestation_data in interval units; rename the current_slot field of StoreError::AttestationTooFarInFuture to store_time so the variant accurately reflects what was compared.

Notes

• leanSpec is pinned at bc17f7a (2026-04-20). PR #682 was authored 2026-04-25 and is still open upstream, so this port is ahead of the pinned spec; the new boundary regression tests added in #682 will land here when the pin is bumped.
• Zeam's data.slot <= current_slot rule was strictly correct for producer/receiver alignment but left no room for clock skew. The new rule allows exactly one interval, so a Zeam node and an ethlambda node tolerate each other under normal NTP drift.

Test plan

• cargo build -p ethlambda-blockchain
• cargo clippy --workspace --all-targets -- -D warnings
• cargo fmt --all -- --check
• cargo test --workspace --lib (69 passed, 6 ignored)
• Forkchoice spec tests will run on CI (fixtures not fetched locally on this worktree).

[github-actions]

🤖 Kimi Code Review

Security: Integer overflow in slot calculation

crates/blockchain/src/store.rs:181

let attestation_start_interval = data.slot * INTERVALS_PER_SLOT;

The multiplication can overflow if data.slot is maliciously set to a large value (e.g., u64::MAX). In release mode, this wraps around, potentially causing the validation to pass incorrectly. Use checked arithmetic:

let attestation_start_interval = data
    .slot
    .checked_mul(INTERVALS_PER_SLOT)
    .ok_or_else(|| StoreError::InvalidAttestationData {
        reason: "slot value too large".into(),
    })?;

Consensus correctness: Good

The change from slot-based (current_slot + 1) to interval-based (store.time() + 1) validation correctly prevents pre-publication attacks where adversaries gossip next-slot attestations early. This aligns with the spec's MAXIMUM_GOSSIP_CLOCK_DISPARITY intent.

Minor: Error message clarity

crates/blockchain/src/store.rs:801

The error message mixes units (slots vs. intervals). Consider including the calculated start interval for debugging:

#[error("Attestation slot {attestation_slot} is too far in future (attestation starts at interval {attestation_start_interval}, store time: {store_time})")]

Style: OK

Import reformatting in store.rs:23-25 is acceptable. The constant GOSSIP_DISPARITY_INTERVALS is well-documented with the Lean spec reference.

---

Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

[greptile-apps]

Greptile Summary

This PR tightens the attestation future-slot bound in validate_attestation_data, replacing a whole-slot tolerance (~4 s) with a single-interval margin (~800 ms) expressed in interval units. It also renames the current_slot field of StoreError::AttestationTooFarInFuture to store_time so the error message accurately reflects what was compared.

• The new condition data.slot * INTERVALS_PER_SLOT > store.time() + GOSSIP_DISPARITY_INTERVALS introduces a multiplication on data.slot, which comes from an untrusted network peer. A crafted attestation with a near-u64::MAX slot value will panic in debug builds (DoS) or silently wrap and bypass the check in release builds. Using saturating_mul would close the gap without changing any valid-slot behaviour.

Confidence Score: 3/5

Do not merge until the overflow in data.slot * INTERVALS_PER_SLOT is addressed; all other changes are correct.

A single P1 logic bug: an adversary-supplied data.slot near u64::MAX causes the new interval multiplication to overflow, which panics in debug builds (DoS) and silently bypasses the time check in release builds. The fix is a one-line change to saturating_mul. Everything else — the constant, the tighter bound, the error-field rename — looks correct.

crates/blockchain/src/store.rs line 183 — the data.slot * INTERVALS_PER_SLOT multiplication.

Important Files Changed

Filename	Overview
crates/blockchain/src/lib.rs	Adds GOSSIP_DISPARITY_INTERVALS = 1 constant with clear doc comment; no issues found.
crates/blockchain/src/store.rs	Rewrites attestation time-check to interval units and renames error field to store_time; introduces a potential integer overflow in data.slot * INTERVALS_PER_SLOT when slot value is attacker-controlled.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Receive gossip attestation] --> B[validate_attestation_data]
    B --> C[Availability Check\nblocks exist in store?]
    C -->|no| E1[Err: UnknownBlock]
    C -->|yes| D[Topology Check\nsource ≤ target ≤ head?]
    D -->|no| E2[Err: SourceExceedsTarget / HeadOlderThanTarget]
    D -->|yes| F[Consistency Check\ncheckpoint slots match block slots?]
    F -->|no| E3[Err: SlotMismatch]
    F -->|yes| G["Time Check (NEW)\nattestation_start_interval = data.slot * INTERVALS_PER_SLOT\nattestation_start_interval > store.time() + GOSSIP_DISPARITY_INTERVALS?"]
    G -->|yes – too far future| E4[Err: AttestationTooFarInFuture\nreports store_time in intervals]
    G -->|no| H[Ok – attestation accepted]

    style G fill:#fffde7,stroke:#f9a825
    style E4 fill:#ffebee,stroke:#c62828

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: crates/blockchain/src/store.rs
Line: 183-184

Comment:
**Integer overflow on adversary-supplied slot**

`data.slot` arrives from an untrusted peer. If an attacker sends an attestation with `data.slot ≥ u64::MAX / INTERVALS_PER_SLOT + 1` (≈ 3.69 × 10¹⁸), the multiplication `data.slot * INTERVALS_PER_SLOT` overflows. In a debug build this panics (remote DoS); in a release build the value wraps to a small number, causing the time-check to silently pass and admitting an astronomically far-future attestation.

The old code divided `store.time()` instead of multiplying `data.slot`, so it was immune to this. A checked or saturating multiply sidesteps the issue:

```suggestion
    let attestation_start_interval = data.slot.saturating_mul(INTERVALS_PER_SLOT);
    if attestation_start_interval > store.time() + GOSSIP_DISPARITY_INTERVALS {
```

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "fix: tighten attestation future-slot bou..." | Re-trigger Greptile}

[github-actions]

🤖 Claude Code Review

Now I have enough context for a thorough review.

---

PR #317 — fix: tighten attestation future-slot bound (leanSpec #682)

Overview

This PR ports leanSpec PR #682, replacing a per-slot future-attestation tolerance (data.slot > current_slot + 1) with a per-interval one (data.slot * INTERVALS_PER_SLOT > store.time() + GOSSIP_DISPARITY_INTERVALS). The new constant GOSSIP_DISPARITY_INTERVALS = 1 mirrors mainnet's MAXIMUM_GOSSIP_CLOCK_DISPARITY at the lean 800 ms granularity.

The security motivation is correct: allowing a full slot of future tolerance (~3.2 s of the 4 s slot) lets an adversary pre-publish next-slot aggregates before any honest validator could produce them. Tightening to one interval (~800 ms) closes that window while still absorbing ordinary NTP drift.

---

Correctness

The interval-arithmetic is sound. Verifying boundary cases:

• Mid-slot, e.g. store.time() = 3 (slot 0, interval 3):
  • Slot 1 attestation: 5 > 3 + 1 = 4 → rejected. ✓
  • Slot 0 attestation: 0 > 4 → accepted. ✓
• Last interval of a slot, store.time() = 4 (slot 0, interval 4):
  • Slot 1 attestation: 5 > 4 + 1 = 5 → accepted (one interval early). ✓ This is exactly the intended 1-interval margin.
• First interval of slot 1, store.time() = 5:
  • Slot 1: 5 > 5 + 1 = 6 → accepted. ✓
  • Slot 2: 10 > 6 → rejected. ✓

The logic is correctly tighter than the old bound.

---

Potential Issue: Integer Overflow (store.rs:183)

let attestation_start_interval = data.slot * INTERVALS_PER_SLOT;

data.slot is u64, received from an untrusted peer. If an adversary sends data.slot = u64::MAX / 5 + 1 (≈ 3.69 × 10¹⁸), this multiplication wraps to a small value in release builds (silent wrong result) or panics in debug builds. The availability and consistency checks earlier in the function do add practical protection — a fake data.slot with real block roots would be caught by the source_header.slot != data.source.slot check. But those checks guard against mismatched checkpoint slots, not against a data.slot value that is independently set to an astronomical number while pointing to real existing blocks.

Suggest using a saturating (or checked) multiply:

let attestation_start_interval = data.slot.saturating_mul(INTERVALS_PER_SLOT);

saturating_mul would produce u64::MAX, which is always greater than store.time() + GOSSIP_DISPARITY_INTERVALS, correctly rejecting the attestation without panicking.

The same applies to store.time() + GOSSIP_DISPARITY_INTERVALS on the right-hand side, though store.time() near u64::MAX is not realistic.

---

Minor: Error Message Units Mismatch (store.rs:801)

"Attestation slot {attestation_slot} is too far in future (store time: {store_time} intervals)"

attestation_slot is in slot units; store_time is in interval units. A developer reading a log message for slot 10 with store time 47 needs to mentally compute 47 / 5 = 9.4 to understand the delta. Consider including the slot equivalent in the message:

"Attestation slot {attestation_slot} is too far in future \
 (store time: {store_time} intervals = slot {})", store_time / INTERVALS_PER_SLOT

Or simply expose store_time_slots: u64 as a second variant field. This is a minor readability nit, not a correctness issue.

---

Spec Divergence Note

The port is ahead of the pinned leanSpec commit (bc17f7a, 2026-04-20) since PR #682 is still open upstream. The PR is transparent about this and the plan to add boundary regression tests when the pin is bumped. This is fine as long as CI's forkchoice spec tests (which run against the pinned fixtures) continue to pass — the existing fixtures won't exercise the tightened bound, so no regressions are expected until the pin advances.

---

Code Style and Conventions

• The new constant is well-placed alongside its sibling constants and has a clear doc comment referencing the upstream PR.
• The rename of current_slot → store_time in the error variant is accurate and correct — the old name was a misnomer since it was derived from store.time() and the comparison is no longer in slot units.
• Import ordering in store.rs:23–24 is alphabetical. ✓
• Comment in validate_attestation_data correctly explains the why of the interval-based approach. ✓

---

Summary

The core logic change is correct and the security motivation is sound. The one concrete change to make before merging:

• Use data.slot.saturating_mul(INTERVALS_PER_SLOT) at store.rs:183 to guard against adversarial overflow. This is a low-complexity fix with clear benefit given that attestation data comes from untrusted peers.

The error message unit mismatch is a minor DX nit. Everything else looks good.

---

Automated review by Claude (Anthropic) · sonnet · custom prompt

[github-actions]

🤖 Codex Code Review

Findings

1. High: store.rs, store.rs, store.rs
   data.slot * INTERVALS_PER_SLOT is now done on untrusted gossip input. For any slot > u64::MAX / 5, debug builds panic on the multiply; release builds wrap. Because multiplication modulo 2^64 is invertible here, a malicious peer can choose a huge slot whose wrapped product is <= store.time() + 1, bypass the new future-slot check, and then hit the try_into().expect("slot exceeds u32") panics in both XMSS verification paths. The previous direct comparison (data.slot > current_slot + 1) did not expose that remote-DoS path. This should avoid multiplying the untrusted slot directly, or use checked arithmetic and reject slots outside the XMSS u32 range before verification.

Aside from that, the interval-based tightening itself is consistent with the store’s interval clock model. This was a static review; I did not run the test suite.

---

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

[MegaRedHand]

Tested against the leanSpec PR head

Pinned leanSpec locally to PR leanSpec#682 head (d51690a), regenerated fixtures with uv run fill --fork devnet --scheme=prod, and ran the spec tests against this branch:

$ cargo test -p ethlambda-blockchain --release --test forkchoice_spectests
test result: ok. 80 passed; 0 failed; 0 ignored
$ cargo test -p ethlambda-blockchain --release
test result: ok. 110 passed; 0 failed; 6 ignored

That covers the 3 new boundary regressions added by leanSpec#682 (in both gossip- and aggregated-attestation suites) and all the restructured existing tests.

I'm leaving the LEAN_SPEC_COMMIT_HASH pin alone for now since leanSpec#682 is still open upstream, and bumping it would require relaxing the --single-branch clone in the Makefile to reach unmerged PR refs. We can bump it after leanSpec's PR is merged.