fix(CHAIN-4357): add monotonicity guard to FinalizeTask to prevent finalized_head regression

[eric-ships]

CHAIN-4357 | Immunefi #75698

Context

Immunefi report #75698 identifies a finalized-head regression bug in the consensus engine.

EngineTask::Ord treats all same-variant FinalizeTask entries as Ordering::Equal. The engine uses a BinaryHeap<EngineTask> (max-heap), and Rust's BinaryHeap does not guarantee stable ordering for equal elements. When two FinalizeTasks with different block_numbers are enqueued, their relative pop order is non-deterministic.

Attack path:

1. FinalizeTask(block_number=105) is popped first, advancing finalized_head to 105.
2. FinalizeTask(block_number=100) is popped next. The existing safe-head check passes (safe_head >= 100). The task fetches block 100 and calls SynchronizeTask, which sets finalized_head back to 100.

This is a confirmed valid finding per Andreas Bigger.

Changes

crates/consensus/engine/src/task_queue/tasks/finalize/task.rs

Adds a monotonicity guard as the first check in FinalizeTask::execute(). If block_number is strictly less than the current finalized_head.number, the task returns Ok(()) immediately without fetching the block or dispatching a forkchoice update.

crates/consensus/engine/src/task_queue/tasks/finalize/task_test.rs

• Updated block_not_found_returns_error to explicitly set finalized_head below block_number so the guard does not fire (previously the test implicitly had finalized_head == safe_head == 10 while requesting block 7, which would now correctly no-op).
• Added stale_task_does_not_regress_finalized_head: directly exercises the vulnerability scenario (finalized_head = 10, block_number = 7, no RPC mocks registered). Verifies the task no-ops and finalized_head remains 10.

What was tried / considered

Fix EngineTask::Ord to order FinalizeTasks by block_number (descending): This would make the BinaryHeap always pop the higher-numbered task first, which would prevent the regression in the common case. However, it does not help when a lower-numbered task arrives after the higher one has already been executed and popped. The monotonicity guard is strictly more robust because it handles both ordering and late-arriving tasks.

Use <= instead of < for the guard: Andreas's gist used <=, which would also no-op on re-finalization of the same block number. This is safe in practice (finalized blocks cannot change), but it would break the genesis block finalization tests (finalized_head.number = 0, block_number = 0) where the initial all-zero finalized_head hash needs to be updated to the real genesis hash. Strict < avoids that edge case while still fully preventing regressions.

Test plan

• cargo test -p base-consensus-engine -- finalize passes (7/7 tests)
• CI green

[linear]

CHAIN-4357

[cb-heimdall]

🟡 Heimdall Review Status

Requirement	Status	More Info
Reviews	🟡 0/1	Denominator calculation Show calculation 1 if user is bot 0 1 if user is external 0 2 if repo is sensitive 0 From .codeflow.yml 1 Additional review requirements Show calculation Max 0 0 From CODEOWNERS 0 Global minimum 0 Max 1 1 1 if commit is unverified 1 Sum 2

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment

Project	Deployment	Actions	Updated (UTC)
base	Ignored		May 8, 2026 6:56pm

[github-actions]

Nit: consider adding a debug! or warn! log when the guard fires. The happy path already logs at info! level (line 75), and silently dropping a stale finalization task makes it harder to diagnose ordering issues in production. Something like:

Suggested change

	if self.block_number < state.sync_state.finalized_head().block_info.number {
	return Ok(());
	}
	if self.block_number < state.sync_state.finalized_head().block_info.number {
	debug!(
	target: "engine",
	block_number = self.block_number,
	finalized_head = state.sync_state.finalized_head().block_info.number,
	"skipping stale finalize task"
	);
	return Ok(());
	}

Non-blocking — the fix is correct as-is.

[github-actions]

Review Summary

The fix is correct and well-scoped. The monotonicity guard at FinalizeTask::execute prevents finalized_head from regressing when the BinaryHeap pops a stale FinalizeTask with a lower block_number after a higher one has already executed.

What's good:

• Strict < (not <=) correctly handles the genesis edge case where finalized_head.number == 0 and block_number == 0 — the initial all-zero hash needs updating to the real genesis hash.
• The guard is placed before any RPC calls, avoiding unnecessary network round-trips for stale tasks.
• The SynchronizeTask delegation is unmodified — the guard acts purely as a pre-filter.
• Test coverage is thorough: the existing block_not_found_returns_error test is correctly updated to set finalized_head below block_number, and the new stale_task_does_not_regress_finalized_head test directly exercises the vulnerability with no mock RPC registered (proving no calls are made).

One minor suggestion:

• Added an inline comment suggesting a debug! log when the guard fires, for operational observability. Non-blocking.

No correctness, safety, or architectural concerns.