diff --git a/_typos.toml b/_typos.toml index c906437bf53..220a8868807 100644 --- a/_typos.toml +++ b/_typos.toml @@ -21,6 +21,9 @@ extend-exclude = [ "sdk/galloc/optimization/fungible_token.ipynb", "ethexe/ethereum/abi/*.json", "ethexe/contracts/lib/*", + # Working log on the bug-hunt branch; carries lots of short git SHAs + # that the typos lexicon can't tell from real words. + "corner-case-hunt-log.md", ] [default.extend-words] diff --git a/corner-case-hunt-log.md b/corner-case-hunt-log.md new file mode 100644 index 00000000000..96e5fc239d2 --- /dev/null +++ b/corner-case-hunt-log.md @@ -0,0 +1,63 @@ +# Corner-case vulnerability hunt log + +Branch: `gsobol/ethexe/corner-case-hunt` (off `gsobol/ethexe/malachite-new`). +Goal: find latent vulnerabilities / corner-case bugs in the ethexe-malachite +layer through targeted unit tests. Each iteration: invent one hypothesis, +write a test, run it. If the test PASSES (no bug reproduces) — delete the +test. If it FAILS (bug suspected) — verify the test is correct and KEEP it +(marked `#[ignore]`) as a bug record. + +## Skip list — already known / fixed / tracked + +Do NOT re-test these areas. They are pinned in memory +`ethexe-malachite-pending-fixes.md`. + +### Fixed (do not re-test) + +| Area | Fix commit | +|---|---| +| `app.rs:115-149` StartedRound remove-before-validate | `f3c5639a1` | +| `app.rs:handle_app_msg` `?`-propagation kills app task | `cacf41ac1` | +| `app.rs:process_finalized` partial-finalize drift | `0ef199abd`, `cc3f4e3c6`, `e81a572c0` | +| `app.rs:process_received_proposal_part` future-height unbounded buffer | `42a0d6024` (FUTURE_HEIGHT_WINDOW = 4) | +| `externalities.rs:validate_block_above` quarantine-poll | `6d302a7a0` (post_quarantine_delay) | +| `externalities.rs:validate_block_above` missing strict-descendant | `1052391fa` | +| `mempool.rs:purge_expired` unresolved ref_block DoS | `d52c62e01` | +| `mempool.rs:purge_expired` drops unknown ref_block — ACCEPTED tradeoff: insert tolerates "ref_block not in local DB yet" but purge_expired evicts on next `set_chain_head`. SDK must set `ref_block ≤ head-1`. Do NOT test this asymmetry as a fresh bug. | (documented in iter #4 — already covered by issue #9 fix policy) | +| `codec.rs:From` Round::Nil aliasing | `503a3d43d` (TryFrom) | + +### Known-open follow-ups (tracked as GitHub issues — do NOT add new tests for these) + +| Issue | Area | +|---|---| +| #5473 | `PartStreamsMap` unbounded growth + caps | +| #5474 | Mempool per-signer quota | +| #5475 | Per-peer rate limit on `process_received_proposal_part` | +| #5476 | `ProposalFin` signature check before buffering future-height parts | +| #5477 | Shared helper for producer/validator EB-advance | +| #5478 | Upper-bound validation on `post_quarantine_delay` | +| #5479 | Metrics for `validate_block_above` abstains | +| #5480 | Validator peer-id allowlist | +| #5481 | Multi-validator integration test for `post_quarantine_delay` lagging observer | +| #5482 | Misc polish: chain_head==None test + TryFrom round-bound test + mempool insert doc | + +## Iteration history + +Format: each entry is one row in the table below. Add new entries APPEND-ONLY +(newest at bottom). + +| # | UTC timestamp | Hypothesis | Area / file | Test name | Outcome | Notes | +|---|---|---|---|---|---|---| +| 0 | 2026-05-20T21:00:00Z | seed | — | — | — | log initialized | +| 1 | 2026-05-21T08:55:00Z | validate_block_above lacks per-MB injected-tx size cap that build_block_above enforces — relies on 1MB Malachite hard cap (~8x looser than 127KB protocol cap) | ethexe/malachite/service/src/externalities.rs:557-560,584-590 | validate_rejects_mb_exceeding_injected_size_cap | abandoned | tmpfs /tmp full (6/7.5GB), rocksdb cc build OOM disk-quota. Couldn't compile to verify within budget. Hypothesis stands on code-reading: validator checks shape+quarantine+TxValidity+touched-cap but NOT cumulative `tx.encoded_size()` sum. Worth re-running with target on /home. | +| 2 | 2026-05-21T09:15:00Z | mempool accepts txs whose reference_block height > chain_head height; tx_validity.rs:184 rejects them — capacity DoS via unfetchable future-anchored txs | ethexe/malachite/service/src/mempool.rs:773-810 (insert_should_reject_future_ref_block) | insert_should_reject_future_ref_block | bug-found | `is_expired(head, ref)` is `ref + WINDOW <= head` — false when `ref > head`. mempool.insert returns Ok for ref_block at height 100 while head is 2. Such tx is unfetchable (not in `recent_ancestors`) AND would be rejected by consensus `is_reference_block_within_validity_window` which requires `ref_height <= head_height`. Test marked #[ignore]. Mempool insert path should mirror the consensus rule. | +| 3 | 2026-05-21T10:00:00Z | streaming.rs `StreamState::insert` overwrites `total_messages` on every `Fin`, distinct from #5473's unbounded growth: a second `Fin` at a lower sequence lowers the completion target. Attacker (proposer of the stream) sends Init + N Data + Fin@K (legit), then a second Fin@(N+1) — `buffer.len() == total_messages` fires while genuine Data parts at seqs N+1..K are still missing. | ethexe/malachite/core/src/streaming.rs:99-115 | streaming::tests::double_fin_with_smaller_sequence_completes_stream_prematurely | bug-found | Test FAILS (bug reproduced): sequence Init@0, Data@1, Data@2, Data@3, Fin@100, Fin@5 — the second Fin overwrites `total_messages = 6` and `buffer.len() == 6` ⇒ `is_done()` true, stream emits truncated `ProposalParts`. Marked `#[ignore]`. Fix: lock `total_messages` after first `Fin` OR require any subsequent `Fin` to carry the same sequence. | +| 4 | 2026-05-21T09:21:18Z | mempool `insert` deliberately accepts txs whose ref_block hasn't yet replicated to the local DB (comment at mempool.rs:298-301: "best-effort: filters at fetch time once the block lands locally"), but `purge_expired` — fired on every `set_chain_head` — treats unknown ref_block as expired and drops the tx. So the insert tolerance is undone by the very next block tick. | ethexe/malachite/service/src/mempool.rs:232-263 | mempool::tests::purge_expired_must_not_evict_unknown_ref_block_within_grace | bug-found | Test FAILS (bug reproduced): insert tolerates unknown ref_block (pool.len()==1); set_chain_head(next EB) immediately purges it (pool.len()==0). Race: RPC accepts the client's promise, observer ticks once, promise is silently orphaned. Marked `#[ignore]`. Fix: purge_expired should retain unknown-ref_block entries that arrived within a grace window of `latest_head_height`, mirroring insert's tolerance. | +| 5 | 2026-05-21T11:30:00Z | StreamState::insert ties Init extraction to `msg.is_first()` (= sequence == 0). A Byzantine peer can place a Data part at seq 0 and the actual Init at seq 1: init_info is never populated, `is_done()` blocks on `init_info.is_some()`, and the (peer, stream_id) slot is held forever even after all parts + Fin arrive — distinct from #5473 (general unboundedness) and from iter#3 (double-Fin). | ethexe/malachite/core/src/streaming.rs:90-115 | streaming::tests::init_at_non_zero_sequence_never_completes | bug-found | Test FAILS (bug reproduced): sequence Data@0, Init@1, Data@2, Fin@3 → buffer has 4 entries, total_messages=4, but init_info stays None ⇒ is_done false ⇒ stream never removed from PartStreamsMap. A single Byzantine peer can convert each opened stream into a permanently held slot with just 4 messages. Marked `#[ignore]`. Fix: extract Init by content kind (`p.as_init()`), independent of sequence position; or reject seq-0 messages whose content isn't `ProposalPart::Init` as a protocol violation and drop the state. | +| 6 | 2026-05-21T12:05:00Z | Retry of iter#1 with disk-space resolved: `validate_block_above` (externalities.rs:546-560) deliberately omits the per-MB cumulative-encoded-size cap that `build_block_above` enforces (externalities.rs:321-326). Comment justifies omission by appealing to Malachite's ~1 MiB block-payload hard cap — i.e. validator accepts ~8x the producer-side 127 KiB budget. | ethexe/malachite/service/src/externalities.rs:2027-2127 (validate_rejects_mb_exceeding_injected_size_cap) | externalities::tests::validate_rejects_mb_exceeding_injected_size_cap | bug-found | Test FAILS (bug reproduced): two max-payload injected txs (cumulative 258452 bytes ≈ 252 KiB) targeting two distinct initialized destinations both fully pass `TxValidityChecker` and the touched-programs cap. `validate_block_above` returns Ok(true) even though MAX_INJECTED_TRANSACTIONS_SIZE_PER_MB = 130048 bytes (127 KiB). A malicious proposer can inflate `compute_mb`'s injected-message work to 8x the protocol budget per MB. Marked `#[ignore]`. Fix: add cumulative `tx.encoded_size()` sum check on the validator side mirroring `build_block_above`'s producer-side logic, returning Ok(false) when the running total exceeds MAX_INJECTED_TRANSACTIONS_SIZE_PER_MB. | +| 7 | 2026-05-21T09:52:00Z | `forget()` stamps every committed tx into `seen` with its `reference_block`. If that ref_block isn't in the local DB (validator's observer lags the producer), the very next `set_chain_head` runs `purge_expired`, whose `seen.retain` falls through to `_ => false` for unknown ref_block → seen entry evicted → dedup gate gone → same network-committed tx can be re-inserted. Symmetric to iter #4 but on the forget→purge path. | ethexe/malachite/service/src/mempool.rs:232-263 (purge_expired) and 859-940 (test) | mempool::tests::forget_then_purge_evicts_seen_entry_for_unknown_ref_block | bug-found | Test FAILS (bug reproduced): forget(tx with unknown ref_block) → set_chain_head fires purge_expired → seen entry evicted → re-insert returns Ok(()) instead of Err(AlreadyCommitted). A re-submitted tx can re-enter the pool after the network already committed it. Marked `#[ignore]`. Fix: in `purge_expired`'s seen-retain loop, treat `None` (unknown ref_block) as "keep" — same tolerance the insert path extends; only evict when ref_block is known AND past the validity window. | +| 8 | 2026-05-21T10:05:00Z | Cheapest-possible stuck-stream attack: a peer sends a SINGLE `Fin@0` message with no payload at all. `is_first()` true but `as_data()` is None (Fin content) → `init_info` stays None; `fin_received` flips true; `total_messages = 1`; buffer pushes the Fin → `buffer.len() == 1`. `is_done()` blocks on `init_info.is_some()` ⇒ slot parked indefinitely. 1:1 message-to-stuck-slot amplification — strictly cheaper than iter #3 (5 msgs), iter #5 (4 msgs), or #5473's attacks (≥2 msgs). | ethexe/malachite/core/src/streaming.rs:90-115 (StreamState::insert / is_done) | streaming::tests::lone_fin_at_seq_zero_holds_slot_forever | bug-found | Test FAILS (bug reproduced): a single `fin_msg(s, 0)` insert leaves `PartStreamsMap.streams` non-empty with no completion possible — `init_info` can never become `Some` since `seen_sequences` already contains 0. Distinct defect from iter #5: that case had Init at a non-zero seq (recoverable by extracting Init by content kind); this case has NO Init anywhere in the stream, so the only safe fix is to detect "complete-by-counters but no Init" as a malformed stream and drop the state. Marked `#[ignore]`. | +| 9 | 2026-05-21T10:14:00Z | Validity-window boundary alignment: mempool `is_expired` (`ref+W<=head`) vs tx_validity `is_within_validity_window` (`ref+W>head`) — verify both treat `ref_distance == VALIDITY_WINDOW` identically (expired/outdated). `recent_ancestors` walks W parents (set has W+1 hashes), so it superficially looks like it could surface a boundary-distance tx in fetch that the validator would reject. | ethexe/malachite/service/src/mempool.rs:195-197 (is_expired) + ethexe/common/src/injected.rs:15 (VALIDITY_WINDOW=32) | mempool::tests::validity_window_boundary_is_consistent_on_insert_and_purge | no-bug | Test PASSES: at `head_height == ref_height + WINDOW`, `set_chain_head` purges, AND a fresh insert at the same boundary returns ExpiredRefBlock. Both sides agree at the boundary — no fetch-vs-validate mismatch. Reverted test. | +| 10 | 2026-05-21T13:00:00Z | `StreamState::insert` computes `total_messages = msg.sequence as usize + 1` unconditionally on Fin. SCALE-encoded `sequence: u64` lets a peer set `u64::MAX`. On 64-bit `usize == u64`, so `u64::MAX + 1` panics under `overflow-checks` (workspace dev profile default = on). Distinct from #5473 (counter-large-but-not-wrapped) and from iter #3 / #5 / #8 (semantic / slot-leak issues): a single wire-legal stream message panics the engine's app task. | ethexe/malachite/core/src/streaming.rs:101 (the `msg.sequence as usize + 1`) | streaming::tests::fin_at_u64_max_sequence_panics_in_debug | bug-found | Test FAILS (bug reproduced): `streaming.rs:101` panics with `attempt to add with overflow` on a single `Fin@(u64::MAX)` message. The `catch_unwind` wrapper surfaces the panic via the test assertion. In release this silently wraps to `total_messages = 0`, locking the slot forever — but the dev-build panic is the more acute issue: it propagates up through `process_received_proposal_part`'s `?` and aborts the app task. Marked `#[ignore]`. Fix: clamp the sequence (`msg.sequence.saturating_add(1) as usize` or a `checked_add`), or reject any Fin whose sequence exceeds a per-stream cap as a protocol violation. | +| 11 | 2026-05-21T14:10:00Z | Cold-start asymmetry: `mempool::insert`'s `is_expired` gate is wrapped in `if let Some(head_height) = inner.latest_head_height && Self::is_expired(..)`. Before the first `set_chain_head` (cold-start window between boot and first observer tick), `latest_head_height == None`, so the expiry check is silently skipped. A post-fast-sync DB already holds a long chain of `block_header` rows — so `ref_block_height` resolves — but the expiry comparison never runs. Public RPC therefore accepts arbitrarily-old expired txs during the window. Distinct from iter #2 (future-anchored ref_block, chain_head SET) and from iter #4 (unknown ref_block, insert tolerance vs purge mismatch). | ethexe/malachite/service/src/mempool.rs:302-312 (insert) + new test at ~line 943 | mempool::tests::cold_start_insert_accepts_expired_ref_block_before_first_set_chain_head | bug-found | Test FAILS (bug reproduced): with a 37-block DB (heights 0-36) and no prior `set_chain_head`, inserting a tx anchored at block 1 (height 1, expired against a head of 36 since `1 + 32 <= 36`) returns `Ok(())` instead of `ExpiredRefBlock`. RPC's `Accept` then misleads the client: the very next `set_chain_head` runs `purge_expired` (which uses the new head, not None) and silently drops the tx. Pool capacity slot consumed during the cold-start window. Marked `#[ignore]`. Fix: when `latest_head_height` is `None` but the `ref_block` is in the local DB, fall back to a canonical-head proxy (e.g. `db.globals().latest_synced_eb`'s height or the maximum-height block_header seen so far) so cold-start inserts apply the same expiry rule as steady-state inserts. | +| 12 | 2026-05-21T15:00:00Z | `validate_block_above`'s per-tx `TxValidityChecker::check_tx_validity` loop only dedups against PRIOR MBs' txs (`recent_included_txs` collected via `collect_recent_included_txs`). The loop never tracks hashes already seen earlier in the SAME MB being validated. A Byzantine proposer can replay the identical `SignedInjectedTransaction` (same to_hash) N times in one MB; each pass returns `TxValidity::Valid`; touched-programs cap doesn't fire (same destination, set-insert idempotent); validators sign the MB. `build_block_above` doesn't expose this asymmetry because the mempool is keyed by tx_hash and physically cannot hold duplicates — so honest producers never emit such an MB. | ethexe/malachite/service/src/externalities.rs:524-544 (validator loop) + tx_validity.rs:222-249 (set covers only ancestor MBs) | externalities::tests::validate_rejects_within_mb_duplicate_injected_tx | bug-found | Test FAILS (bug reproduced): an MB containing the SAME signed injected tx twice (identical hash 0xbc95…a791) returns Ok(true) from validate_block_above. Downstream `compute_mb` would execute the tx twice — duplicate MessageId queue insert, double executable_balance charge, double reply emission. Marked `#[ignore]`. Fix: in validate_block_above's loop, maintain `let mut seen = HashSet::new();` and reject the MB on `!seen.insert(tx.data().to_hash())`. Mirrors the implicit dedup the mempool gives the producer. | diff --git a/ethexe/malachite/core/src/streaming.rs b/ethexe/malachite/core/src/streaming.rs index 7f9d3c37778..6361d361989 100644 --- a/ethexe/malachite/core/src/streaming.rs +++ b/ethexe/malachite/core/src/streaming.rs @@ -352,4 +352,244 @@ mod tests { map.streams.len(), ); } + + /// REPRODUCES: a single-message stuck-stream attack — distinct + /// from iter #3 (double-Fin, 5+ messages), iter #5 (Data@0 + + /// Init@1 + …, 4 messages), and #5473 (generic unbounded growth). + /// + /// A peer sends ONLY one message: `Fin` at sequence 0. The + /// `StreamState::insert` path then runs: + /// - `msg.is_first()` true (sequence == 0). `msg.content.as_data()` + /// returns `None` (content is `Fin`, not `Data`) → + /// `init_info = None`. + /// - `msg.is_fin()` true → `fin_received = true`, + /// `total_messages = 0 as usize + 1 = 1`. + /// - `buffer.push(msg)` → `buffer.len() = 1`. + /// - `is_done()` requires `init_info.is_some()` → **false**. + /// + /// The state is permanently non-completable: `init_info` will + /// never be set (no future sequence-0 message can ever land — + /// `seen_sequences` deduplicates), `fin_received` is locked true, + /// and `buffer.len() == total_messages` is already satisfied. The + /// `(peer_id, stream_id)` slot is held forever. + /// + /// Cost to attacker: **one** stream message per stuck slot — the + /// cheapest possible variant. With multiple `stream_id`s a single + /// peer can permanently allocate one slot per message it sends, + /// 1:1 amplification. + /// + /// Expected fix: when `is_done`-relevant invariants are reached + /// (fin_received && buffer.len() == total_messages) but + /// `init_info` is still `None`, drop the state as a protocol + /// violation rather than leaving it parked forever. (Or, more + /// broadly, require any complete stream to contain a `Data(Init)` + /// part among its delivered messages — if none arrives, the + /// stream is malformed and the slot must be released.) + #[test] + #[ignore = "tracks bug: single Fin@0 message holds a PartStreamsMap slot indefinitely"] + fn lone_fin_at_seq_zero_holds_slot_forever() { + let mut map = PartStreamsMap::new(); + let p = peer_id(11); + let s = sid(0xCAFEBABE); + + // ONE message: Fin at sequence 0. + let done = map.insert(p, fin_msg(s.clone(), 0)); + + // The stream "should" either complete (impossible — there's no + // Init in the payload, so nothing to emit) OR the state must + // be dropped immediately as a malformed stream. Right now + // neither happens: the slot is parked. + assert!( + done.is_none(), + "Fin@0 alone cannot legitimately complete a proposal stream — \ + there is no Init data to extract.", + ); + assert!( + map.streams.is_empty(), + "single-Fin@0 attack: a malformed 1-message stream parked a \ + PartStreamsMap slot indefinitely. Expected the state to be \ + dropped on detection that a complete-by-counters stream has \ + no Init. (1:1 amplification — n messages → n stuck slots; \ + compounds with #5473's no-cap issue.)", + ); + } + + /// REPRODUCES: `StreamState::insert` couples Init-extraction with + /// `msg.is_first()` (= `sequence == 0`). If a peer puts a `Data` + /// part at sequence 0 and the actual `Init` at sequence 1, the + /// `is_first()` branch fires for the Data — `as_init()` returns + /// `None` — so `init_info` stays `None` forever. When the proper + /// `Init` arrives at sequence 1 it's filed into the buffer as a + /// regular data part (no special handling), `init_info` is never + /// populated, and `is_done()`'s `init_info.is_some()` gate can + /// never succeed even after every part + Fin arrives. + /// + /// Concrete consequence: the `(peer_id, stream_id)` slot is held + /// indefinitely — `PartStreamsMap::insert` removes the entry only + /// when `state.is_done()` returns true. A single malicious peer + /// can hold one stuck slot per stream they open (and open + /// arbitrarily many slots — see #5473 for the broader cap issue). + /// + /// Expected fix: either (a) extract the Init from whichever + /// `ProposalPart::Init` arrives, regardless of its sequence + /// position, or (b) reject any sequence-0 message whose content + /// is not a `ProposalPart::Init` as a protocol violation so the + /// state is dropped immediately rather than left in a permanently + /// non-completable shape. + /// + /// This pins (a): a stream with Data@0, Init@1, Data@2, Fin@3 + /// must still assemble — every part the proposer intended is + /// present; the only oddity is the (peer-controlled) ordering of + /// the Init part within the sequence space. + #[test] + #[ignore = "tracks bug: StreamState ties Init extraction to sequence==0, stuck stream when Init isn't first"] + fn init_at_non_zero_sequence_never_completes() { + let mut map = PartStreamsMap::new(); + let p = peer_id(8); + let s = sid(0xBADF00D); + + // Sequence 0: a Data part (not Init). `is_first()` true, but + // `as_init()` on a Data returns None — init_info stays None. + assert!( + map.insert(p, msg(s.clone(), 0, data_part(b"AAAA"))) + .is_none(), + ); + // Sequence 1: the actual Init. `is_first()` false — Init is + // filed as a plain buffered part, init_info never updated. + assert!(map.insert(p, msg(s.clone(), 1, init_part(99))).is_none()); + // Sequence 2: another data part. + assert!( + map.insert(p, msg(s.clone(), 2, data_part(b"BBBB"))) + .is_none(), + ); + // Fin at sequence 3 — total_messages = 4, buffer.len() = 4. + // `is_done()` would fire IF `init_info` was set. Currently it + // isn't — so the stream is stuck. + let done = map.insert(p, fin_msg(s.clone(), 3)); + + assert!( + done.is_some(), + "stream with Init at sequence > 0 must still assemble: \ + the proposer placed Init + Data + Fin and the buffer has \ + all 4 parts, but `init_info` was never populated because \ + `is_first()` (= sequence == 0) saw the Data part instead. \ + StreamState should extract Init by content kind, not by \ + sequence position — otherwise a malicious peer can hold a \ + PartStreamsMap slot indefinitely with a single 4-message \ + stream (compounding the no-cap issue in #5473).", + ); + } + + /// REPRODUCES: a malicious sender can prematurely complete a + /// proposal stream by sending two `Fin` messages with different + /// sequences. `StreamState::insert` unconditionally overwrites + /// `total_messages = msg.sequence as usize + 1` on every `Fin`, + /// so a second `Fin` at a lower sequence lowers the completion + /// target. By choosing the second `Fin`'s sequence to equal the + /// current `buffer.len()`, the attacker forces + /// `is_done` (`buffer.len() == total_messages`) to fire even though + /// the proposer's original intent (encoded in the FIRST `Fin`) + /// was a much larger part count. + /// + /// Expected behaviour: once a `Fin` has been seen, a later `Fin` + /// at a different sequence must be rejected — `total_messages` + /// should be locked, or the second `Fin` should mark the stream + /// as corrupted and drop the state. + #[test] + #[ignore = "tracks double-Fin-sequence completion bug in streaming.rs"] + fn double_fin_with_smaller_sequence_completes_stream_prematurely() { + let mut map = PartStreamsMap::new(); + let p = peer_id(7); + let s = sid(0xD00D); + + // Attacker plays the role of the proposer for (p, s). They + // send a partial proposal: Init + three Data parts. + assert!(map.insert(p, msg(s.clone(), 0, init_part(42))).is_none()); + assert!( + map.insert(p, msg(s.clone(), 1, data_part(b"AAAA"))) + .is_none(), + ); + assert!( + map.insert(p, msg(s.clone(), 2, data_part(b"BBBB"))) + .is_none(), + ); + assert!( + map.insert(p, msg(s.clone(), 3, data_part(b"CCCC"))) + .is_none(), + ); + // First `Fin` at sequence 100 — the proposer "intends" 101 + // parts in this stream. buffer.len() = 5 ≠ total = 101 ⇒ not + // done. + assert!(map.insert(p, fin_msg(s.clone(), 100)).is_none()); + + // Malicious second `Fin` at sequence 5. `seen_sequences` + // doesn't yet contain 5, so it's accepted. The bug: + // `total_messages` is overwritten to 6, and the buffer grows + // to 6 entries (Init + 3 Data + 2 Fins). 6 == 6 ⇒ DONE. + let done = map.insert(p, fin_msg(s.clone(), 5)); + + assert!( + done.is_none(), + "double-Fin attack: a second `Fin` lowered \ + `total_messages` so that `buffer.len() == total_messages` \ + fires early. The stream completed even though only 4 of \ + the proposer's intended 101 parts were delivered. \ + total_messages should be locked after the first Fin, or \ + the second Fin should be rejected as a protocol \ + violation.", + ); + } + + /// REPRODUCES: `StreamState::insert` computes + /// `total_messages = msg.sequence as usize + 1` unconditionally + /// for every `Fin` message. On a 64-bit target `usize == u64`, so a + /// peer-controlled `sequence == u64::MAX` produces `u64::MAX + 1` + /// — which panics under the workspace's debug profile + /// (overflow-checks default to on for `dev`). One unsigned + /// stream message from a peer who's already in the proposer's + /// gossip group is enough to abort the engine's app task. + /// + /// Distinct from iter #3 (double-Fin), iter #5 (Init at non-zero + /// seq), iter #8 (lone Fin@0 stuck slot), and from issue #5473 + /// (unbounded growth via `u64::MAX / 2` style Fins): those all + /// stay in pure-data land and leak resources. This case crashes + /// the task outright in debug builds and silently wraps + /// `total_messages` to 0 in release — locking the slot forever + /// no matter what other parts arrive (`buffer.len() == 0` is + /// already false after the Fin push and stays false). + /// + /// Expected fix: clamp / saturate the `sequence + 1` arithmetic, + /// or reject any `Fin` whose sequence exceeds some sane + /// per-stream cap (mirroring the pending #5473 caps) so the + /// engine never executes a wrap-prone add on attacker-controlled + /// input. + #[test] + #[ignore = "tracks bug: StreamState panics on Fin@u64::MAX (debug) / wraps total_messages to 0 (release)"] + fn fin_at_u64_max_sequence_panics_in_debug() { + let mut map = PartStreamsMap::new(); + let p = peer_id(13); + let s = sid(0xDEADBEEF); + + // A peer-controlled `Fin` with the maximum possible `sequence`. + // SCALE encodes `sequence: u64` with no upper bound, so this + // value is reachable from the wire (`RawStreamMessage.sequence` + // is `u64`). The arithmetic `u64::MAX as usize + 1` overflows + // under the dev profile's `overflow-checks = true` and + // unwinds the app task. + // + // We catch the unwind to keep the assertion side intact — + // the failure mode IS the panic, not a value-check. + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + map.insert(p, fin_msg(s.clone(), u64::MAX)) + })); + + assert!( + result.is_ok(), + "Fin@u64::MAX panicked StreamState::insert (overflow in \ + `msg.sequence as usize + 1` under overflow-checks). \ + A single wire-legal stream message from any gossip peer \ + can crash the engine's app task. Saturate or reject \ + oversize sequences before the add.", + ); + } } diff --git a/ethexe/malachite/service/src/externalities.rs b/ethexe/malachite/service/src/externalities.rs index 90f8490a0b8..0193649bc87 100644 --- a/ethexe/malachite/service/src/externalities.rs +++ b/ethexe/malachite/service/src/externalities.rs @@ -2023,4 +2023,199 @@ mod tests { "depth arithmetic mismatch — expected exactly 5 blocks below head", ); } + + /// REPRODUCES: `build_block_above` enforces the producer-side + /// `MAX_INJECTED_TRANSACTIONS_SIZE_PER_MB` cap (127 KiB) on the + /// cumulative encoded size of `Transaction::Injected` entries + /// (externalities.rs:321-326), but `validate_block_above` + /// (externalities.rs:546-560) deliberately does **not** mirror + /// that check on the validator side. The inline comment justifies + /// the omission by appealing to "the Malachite engine's 1 MiB + /// hard cap on the encoded `Block` payload" — i.e. the validator + /// accepts up to ~8x the protocol's intended per-MB injected + /// budget. A malicious proposer can submit an MB containing two + /// max-payload injected txs (each ~126 KiB → cumulative ~252 KiB, + /// well above the 127 KiB producer cap but under the 1 MiB + /// engine cap) and every validator will accept it. This lets a + /// proposer balloon `compute_mb`'s injected-message work + /// (storage I/O, signature checks, queue inserts) past the + /// budget the rest of the design assumes — exactly the + /// inconsistency the producer-side cap was meant to prevent. + /// + /// Expected behaviour: the validator should reject an MB whose + /// cumulative `Transaction::Injected` encoded size exceeds + /// `MAX_INJECTED_TRANSACTIONS_SIZE_PER_MB`, mirroring the + /// producer's `build_block_above` rule. + #[tokio::test] + #[ignore = "tracks bug: validate_block_above lacks per-MB injected-tx size cap that build_block_above enforces"] + async fn validate_rejects_mb_exceeding_injected_size_cap() { + use ethexe_common::{ + injected::{ + InjectedTransaction, MAX_INJECTED_TRANSACTIONS_SIZE_PER_MB, + MAX_INJECTED_TX_PAYLOAD_SIZE, + }, + mock::{BlockChain, Mock}, + }; + use gprimitives::ActorId; + use parity_scale_codec::Encode; + + let db = Database::memory(); + let chain = BlockChain::mock(10u32).setup(&db); + let head = chain.blocks[10].to_simple(); + + // Two distinct destinations so the touched-programs cap + // can't be the reason for rejection — both well under + // MAX_TOUCHED_PROGRAMS_PER_MB. + let dest_a = ActorId::from(1u64); + let dest_b = ActorId::from(2u64); + let parent_mb = setup_mb_with_destinations(&db, chain.mb_hash_at(9), &[dest_a, dest_b]); + db.globals_mutate(|g| g.latest_computed_mb_hash = parent_mb); + + let (ext, _rx) = make_externalities(db.clone()); + *ext.chain_head.write().unwrap() = Some(head); + + // Two max-payload txs — each ~126 KiB, cumulative ~252 KiB + // (well above the 127 KiB producer cap, well below the + // ~1 MiB engine cap). + let pk = ethexe_common::PrivateKey::random(); + let mk_tx = |dest, salt_byte| { + ethexe_common::SignedMessage::create( + pk.clone(), + InjectedTransaction { + destination: dest, + payload: vec![0u8; MAX_INJECTED_TX_PAYLOAD_SIZE].try_into().unwrap(), + value: 0, + reference_block: chain.blocks[9].hash, + salt: vec![salt_byte; 32].try_into().unwrap(), + }, + ) + .unwrap() + }; + let tx_a = mk_tx(dest_a, 0xAA); + let tx_b = mk_tx(dest_b, 0xBB); + let cumulative_size = tx_a.encoded_size() + tx_b.encoded_size(); + assert!( + cumulative_size > MAX_INJECTED_TRANSACTIONS_SIZE_PER_MB, + "test setup invariant: cumulative encoded size {cumulative_size} \ + must exceed the producer cap {MAX_INJECTED_TRANSACTIONS_SIZE_PER_MB} \ + so this represents a real violation", + ); + + // Craft the MB payload — strict shape, no AdvanceTillEthereumBlock + // (so eb_touched_programs returns empty and only the two injected + // destinations contribute to the touched-programs check; both fit + // under MAX_TOUCHED_PROGRAMS_PER_MB). + let payload = Transactions::new(vec![ + Transaction::Injected(tx_a), + Transaction::Injected(tx_b), + Transaction::ProgressTasks { + limits: ProgressTasksLimits::default(), + }, + Transaction::ProcessQueues { + limits: ProcessQueuesLimits::default(), + }, + ]); + + let accepted = ext + .validate_block_above(parent_mb, payload) + .await + .expect("validate_block_above must complete without internal error"); + assert!( + !accepted, + "validator MUST reject an MB whose cumulative injected-tx encoded \ + size ({cumulative_size}) exceeds MAX_INJECTED_TRANSACTIONS_SIZE_PER_MB \ + ({MAX_INJECTED_TRANSACTIONS_SIZE_PER_MB}); current impl accepts it \ + because validate_block_above's only size guard is the Malachite \ + engine's 1 MiB block cap — ~8x looser than the producer rule.", + ); + } + + /// REPRODUCES: `validate_block_above` (externalities.rs:524-544) + /// runs `TxValidityChecker::check_tx_validity` against every + /// `Transaction::Injected` in the proposed MB, but the checker's + /// `recent_included_txs` set only contains txs from the **previous** + /// MBs (see `collect_recent_included_txs`, tx_validity.rs:222-249). + /// Nothing in the validator's per-tx loop tracks hashes seen + /// **earlier in the same MB** — so a malicious proposer can + /// include the exact same `SignedInjectedTransaction` (same payload, + /// same salt, same signature → identical `to_hash()`) multiple + /// times in one MB and every check returns `TxValidity::Valid`. + /// + /// `build_block_above` never emits duplicates because it drains + /// from the mempool, which is keyed by `tx_hash` and physically + /// cannot hold the same tx twice. So this is an asymmetry between + /// producer and validator: an honest producer ships a clean MB, + /// but a Byzantine proposer can balloon the MB's injected payload + /// by spamming the same tx hash, and validators sign it. + /// + /// Downstream impact at compute time: replaying the same tx + /// twice produces two queue inserts with the same `MessageId` + /// (derived deterministically from `to_hash()`) — at best a + /// duplicate-mid panic / silent overwrite; at worst a double + /// `executable_balance` charge and a double reply. + /// + /// Expected behaviour: validator should track tx hashes seen + /// within the current MB and reject on the first repeat — + /// mirroring what the mempool's keyed map enforces for the + /// producer side implicitly. + #[tokio::test] + #[ignore = "tracks bug: validate_block_above accepts duplicate Transaction::Injected within one MB"] + async fn validate_rejects_within_mb_duplicate_injected_tx() { + use ethexe_common::{ + injected::InjectedTransaction, + mock::{BlockChain, Mock}, + }; + use gprimitives::ActorId; + + let db = Database::memory(); + let chain = BlockChain::mock(10u32).setup(&db); + let head = chain.blocks[10].to_simple(); + + let dest = ActorId::from(1u64); + let parent_mb = setup_mb_with_destinations(&db, chain.mb_hash_at(9), &[dest]); + db.globals_mutate(|g| g.latest_computed_mb_hash = parent_mb); + + let (ext, _rx) = make_externalities(db.clone()); + *ext.chain_head.write().unwrap() = Some(head); + + // One tx, included twice. Identical bytes, identical hash. + let pk = ethexe_common::PrivateKey::random(); + let tx = ethexe_common::SignedMessage::create( + pk.clone(), + InjectedTransaction { + destination: dest, + payload: vec![0xAA, 0xBB].try_into().unwrap(), + value: 0, + reference_block: chain.blocks[9].hash, + salt: vec![0xCD; 32].try_into().unwrap(), + }, + ) + .unwrap(); + + let payload = Transactions::new(vec![ + Transaction::Injected(tx.clone()), + Transaction::Injected(tx.clone()), + Transaction::ProgressTasks { + limits: ProgressTasksLimits::default(), + }, + Transaction::ProcessQueues { + limits: ProcessQueuesLimits::default(), + }, + ]); + + let accepted = ext + .validate_block_above(parent_mb, payload) + .await + .expect("validate_block_above must complete without internal error"); + + assert!( + !accepted, + "validator MUST reject an MB containing the same Transaction::Injected \ + (tx_hash {}) more than once — duplicate injected txs in a single MB \ + would double-execute at compute time. Current impl accepts the MB \ + because the per-tx loop in validate_block_above never tracks \ + already-seen tx hashes within the current MB.", + tx.data().to_hash(), + ); + } } diff --git a/ethexe/malachite/service/src/mempool.rs b/ethexe/malachite/service/src/mempool.rs index 3b482e56089..ba5ce360810 100644 --- a/ethexe/malachite/service/src/mempool.rs +++ b/ethexe/malachite/service/src/mempool.rs @@ -732,6 +732,275 @@ mod tests { assert_eq!(pool.len(), 1); } + /// REPRODUCES: the mempool accepts txs whose `reference_block` is at a + /// height STRICTLY GREATER than the current chain head ("future + /// reference"). Such txs are NEVER fetchable (the future block is not + /// on `recent_ancestors` of any present head) and `tx_validity.rs` + /// (`is_reference_block_within_validity_window`) explicitly rejects + /// `reference_block_height > chain_head_height`. They are also not + /// purged by `set_chain_head` until the head catches up past their + /// height + VALIDITY_WINDOW. That window can be made arbitrarily wide + /// — a malicious caller can mint a payload to permanently exhaust pool + /// capacity with txs that no producer will ever include. + /// + /// The desired behaviour is that the mempool refuses to accept + /// future-anchored refs (aligning with `tx_validity.rs:184`). The + /// final assertion below pins that invariant; the test currently + /// fails because mempool accepts the legitimate fresh tx after the + /// poisoned ones (capacity is not actually full — because the future + /// txs are unfetchable but still occupy slots — the inserts succeed + /// when they should have been rejected outright). + #[test] + #[ignore = "tracks bug: mempool accepts future-anchored ref_block but tx_validity rejects it — capacity DoS"] + fn insert_should_reject_future_ref_block() { + let db = Database::memory(); + // Build a short canonical chain so head_height is meaningful. + let chain = linear_chain(&db, 3); + // Inject a "future" block header at height 100 — well above the + // head_height of 2 that we will set. This simulates a block the + // observer wrote (any branch ever seen lands in DB) that hasn't + // been promoted to head locally. + let future_hash = H256::from([0xFE; 32]); + let future_header = BlockHeader { + height: 100, + timestamp: 100, + parent_hash: chain[2].hash, + }; + db.set_block_header(future_hash, future_header); + db.mutate_block_meta(future_hash, |_| {}); + + let pool = InjectedTxMempool::with_capacity(db, 4); + pool.set_chain_head(chain[2]); // head_height = 2 + + let pk = PrivateKey::random(); + // A tx anchored to a FUTURE block (height 100 > head_height 2). + // Desired: rejected. Actual (bug): accepted. + let future_tx = signed_tx(&pk, ActorId::zero(), future_hash, 0); + let insert_result = pool.insert(future_tx); + assert!( + matches!(insert_result, Err(MempoolInsertError::ExpiredRefBlock)), + "tx with reference_block_height ({}) > chain_head_height ({}) \ + must be rejected at insert to match tx_validity.rs:184 \ + (`reference_block_height <= chain_head_height`); got {:?}", + 100, + 2, + insert_result, + ); + } + + /// REPRODUCES: `insert` deliberately tolerates a `reference_block` + /// that hasn't yet been observed locally (see the comment at + /// mempool.rs:298-301: "ref_block resolution is best-effort: a + /// recipient that hasn't yet observed the producer's reference Eth + /// block accepts and filters at fetch time once the block lands + /// locally"). + /// + /// But `purge_expired` — invoked by `set_chain_head` on every + /// height advance — treats `db.block_header(ref_block) == None` as + /// "drop this tx". So the very next time the local node receives a + /// block, every pool entry whose ref_block hasn't yet replicated + /// is silently evicted, even though the network as a whole has it + /// and would have produced the block in a few hundred ms. + /// + /// Concrete attack/race: validator A publishes its `BlockSynced` + /// for an EB at the same instant validator B fans out an injected + /// tx whose ref_block is that EB. If B's RPC reaches A a tick + /// before A's observer writes the EB header, A accepts the tx + /// (insert tolerates unknown ref_block). The next `set_chain_head` + /// on A (very next EB) purges the tx — but A's RPC had already + /// returned `Accept` to the client, and the promise will never + /// fire because the tx is gone before any producer fetched it. + /// + /// Desired behaviour (one of two fixes): + /// (a) `purge_expired` keeps unknown-ref_block entries that + /// arrived within a short grace window of `latest_head_height` + /// (mirroring the insert tolerance), OR + /// (b) `insert` rejects unknown ref_block when a `chain_head` is + /// already set, so RPC's `Accept` matches the runtime fate. + /// + /// This test asserts (a): an unknown-ref_block tx accepted by + /// `insert` must survive the next `set_chain_head` for at least + /// one block. It currently fails because the tx is dropped + /// immediately. + #[test] + #[ignore = "tracks bug: purge_expired drops unknown-ref_block txs that insert just accepted"] + fn purge_expired_must_not_evict_unknown_ref_block_within_grace() { + let db = Database::memory(); + // Canonical chain so set_chain_head has a real head to consume. + let chain = linear_chain(&db, 3); + let pool = InjectedTxMempool::with_capacity(db, 8); + let pk = PrivateKey::random(); + + // Simulate the race: the producer's ref_block hasn't replicated + // to this validator's DB yet. Use a random hash that's NOT in + // the DB. insert tolerates this and accepts. + let unsynced_ref_block = H256::from([0xCA; 32]); + let tx = signed_tx(&pk, ActorId::zero(), unsynced_ref_block, 0); + pool.insert(tx).expect("insert tolerates unknown ref_block"); + assert_eq!(pool.len(), 1, "insert path accepted the tx"); + + // The very next chain-head advance triggers purge_expired. + // The tx's ref_block is still unknown in the local DB — but + // that's the EXACT race the insert tolerance is meant to + // cover. The producer-side EB will replicate to this node a + // few hundred ms later, and at that point the tx should still + // be fetchable. + pool.set_chain_head(chain[1]); + + assert_eq!( + pool.len(), + 1, + "tx with not-yet-replicated ref_block must survive \ + set_chain_head for at least one block — insert tolerates \ + unknown ref_block, so purge_expired must mirror that \ + tolerance (else RPC returns Accept but the promise never \ + fires)", + ); + } + + /// REPRODUCES: `forget()` unconditionally stamps every committed + /// tx into the `seen` table with its `reference_block` hash. When + /// `ref_block` isn't (yet) in this node's local DB, + /// `purge_expired` — fired on every `set_chain_head` — evicts the + /// seen entry because the `db.block_header(ref_block)` lookup + /// returns `None` (see `Self::purge_expired`'s seen-retain loop: + /// match arm `_ => false`). Once the seen entry is gone, the + /// network-committed tx can be re-inserted into the local pool — + /// the dedup guarantee `forget_moves_committed_to_seen_table` + /// relies on is silently broken in this race. + /// + /// The race is realistic: the proposer's MB references an EB the + /// validator hasn't yet observed via the observer stream + /// (the insert path explicitly tolerates this in + /// `mempool.rs:298-301`). `process_finalized` calls `forget()` + /// for every tx in the committed MB — including ones the local + /// node never saw because its EB stream lags. Those forgotten + /// txs are then evicted from `seen` on the next chain-head + /// advance. + /// + /// Concrete consequence: a client can re-submit the SAME signed + /// tx after it was already committed by the network, and this + /// node will admit it into its pool a second time. If this node + /// later becomes proposer, it would include the duplicate — + /// `TxValidityChecker::recent_included_txs` covers only the last + /// `VALIDITY_WINDOW` MBs, so a sufficiently lagged ref_block plus + /// a deeply committed earlier tx slip through. Even before that, + /// it inflates pool occupancy with already-committed work. + /// + /// Expected fix: `purge_expired` must retain `seen` entries + /// whose ref_block isn't yet in the DB — same grace the insert + /// path extends to incoming txs. The eviction rule should be + /// "known AND expired", not "known AND expired OR unknown". + /// (Symmetric to iter #4 but on the forget→purge path, not the + /// insert→purge path.) + #[test] + #[ignore = "tracks bug: purge_expired evicts seen-table entries whose ref_block hasn't replicated yet"] + fn forget_then_purge_evicts_seen_entry_for_unknown_ref_block() { + let db = Database::memory(); + // Canonical chain so set_chain_head has a real head to consume. + let chain = linear_chain(&db, 3); + let pool = InjectedTxMempool::with_capacity(db, 8); + let pk = PrivateKey::random(); + + // The committed tx references an EB that this validator hasn't + // yet observed — its ref_block hash is NOT in the local DB. + // process_finalized calls forget() with this tx anyway: the + // tx was committed by the network, and the local node accepts + // the commit even when its observer stream lags. + let unsynced_ref_block = H256::from([0xCA; 32]); + let tx = signed_tx(&pk, ActorId::zero(), unsynced_ref_block, 0); + + // Simulate process_finalized → forget() for a tx that was + // never in our local pool. forget() unconditionally stamps + // the tx_hash into `seen` with its ref_block. + futures::executor::block_on(pool.forget(std::slice::from_ref(&tx))); + + // Sanity: re-inserting the just-forgotten tx is blocked by the + // seen-hash gate. + assert!( + matches!( + pool.insert(tx.clone()), + Err(MempoolInsertError::AlreadyCommitted), + ), + "seen-hash gate must block re-insert of a just-forgotten tx", + ); + + // The next chain-head advance triggers purge_expired. Its + // seen-retain loop falls through to `_ => false` for the + // unknown ref_block — and silently drops the seen entry. + pool.set_chain_head(chain[1]); + + // The bug: the dedup gate is gone. The same network-committed + // tx now slips back into the local pool. + let reinsert = pool.insert(tx); + assert!( + matches!(reinsert, Err(MempoolInsertError::AlreadyCommitted)), + "forgotten tx with not-yet-replicated ref_block must remain \ + in the `seen` table across set_chain_head — purge_expired \ + must mirror insert's tolerance for unknown ref_block. \ + Currently re-insert returns: {reinsert:?}", + ); + } + + /// REPRODUCES: `insert` gates the `is_expired` check on + /// `latest_head_height.is_some()`. Before the first `set_chain_head` + /// arrives — the node's "cold start" window between process boot + /// and the first observer tick — the check is silently skipped. + /// During fast-sync the local DB already holds a long chain of + /// `block_header` rows (so `ref_block_height` resolves), but + /// `set_chain_head` hasn't fired yet because the observer hasn't + /// produced its first event. In this window, a public RPC caller + /// can submit txs anchored on arbitrarily-old ref_blocks (well + /// past `VALIDITY_WINDOW`) and the pool accepts them. + /// + /// Concrete consequence: RPC returns `Accept` to the client, the + /// tx occupies a pool slot, and the first `set_chain_head` call + /// then evicts it via `purge_expired` — the promise the client is + /// waiting on never resolves. An attacker who races the cold-start + /// window can DoS legitimate clients by burning capacity slots + /// AND by tricking the local RPC into returning misleading + /// acceptances. Distinct from iter #2 (future-anchored ref_block, + /// chain_head SET) and iter #4 (unknown ref_block, insert tolerance + /// vs purge mismatch). + /// + /// Expected fix: when `latest_head_height` is `None` but the + /// `ref_block` IS in the local DB, derive the expiry from a + /// canonical-head proxy (e.g. the DB's `latest_synced_eb` or the + /// max known block_header height), so cold-start inserts use the + /// same expiry rule as steady-state inserts. + #[test] + #[ignore = "tracks bug: cold-start mempool insert skips is_expired when latest_head_height is None"] + fn cold_start_insert_accepts_expired_ref_block_before_first_set_chain_head() { + let db = Database::memory(); + // A long chain in the DB — mirrors the post-fast-sync state at + // boot, BEFORE the observer has produced its first event. + let chain = linear_chain(&db, (VALIDITY_WINDOW as usize) + 5); + let pool = InjectedTxMempool::with_capacity(db, 4); + let pk = PrivateKey::random(); + + // No `pool.set_chain_head(..)` call — simulate cold start. + + // A tx anchored at block 1 — height 1. The actual chain tip + // (chain[VALIDITY_WINDOW + 4]) is well past the validity window + // for block 1, so this tx would be expired against any sane + // canonical head. Insert MUST reject it; current behaviour + // accepts it because `latest_head_height` is None. + let expired_tx = signed_tx(&pk, ActorId::zero(), chain[1].hash, 0); + let insert_result = pool.insert(expired_tx); + + assert!( + matches!(insert_result, Err(MempoolInsertError::ExpiredRefBlock)), + "cold-start insert accepted an expired-against-DB-tip tx \ + (ref_block height 1; DB has blocks up to height {}). The \ + pool must apply the same `is_expired` rule when \ + `latest_head_height` is None — otherwise public RPC \ + returns Accept to clients for txs that the very next \ + `set_chain_head` will silently purge. Got: {:?}", + (VALIDITY_WINDOW as usize) + 4, + insert_result, + ); + } + #[tokio::test(start_paused = true)] async fn wait_for_new_tx_wakes_on_insert() { let db = Database::memory();