fix(derivation): reject invalid blockCount to prevent batch-parse DoS#995
Merged
tomatoishealthy merged 1 commit intoJun 16, 2026
Merged
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
7f05c01 to
cac5442
Compare
A malformed L1 commitBatch could crash layer1-verify nodes (no recover() in the derivation goroutine) via two panics rooted in an unvalidated blockCount in ParseBatch: - blockCount == 0 (lastBlockNumber == parent) produced empty blockContexts; derive() then returned a nil header that derivation.go dereferenced. Zero-block batches are not supported, so reject them at parse. - a huge blockCount let blockCount*60 overflow uint64 and bypass the length guard, driving an out-of-range make([]*BlockContext). Bound blockCount by payload length using division (overflow-safe) before the multiply. Both become clean parse errors instead of process-killing panics. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cac5442 to
2766d34
Compare
tomatoishealthy
approved these changes
Jun 16, 2026
curryxbo
pushed a commit
that referenced
this pull request
Jun 16, 2026
_commitBatch accepted an unconstrained lastBlockNumber, so a single active staker could commit a batch with lastBlockNumber <= parent.lastBlockNumber (blockCount == 0). The field is not signature-bound (the _getBLSMsgHash BLS path is still a bytes32(0) stub), so this needs no collusion. Derivation nodes in layer1 verify mode then panic (pre-#995) or stall permanently (post-#995) on the malformed batch — a network-wide DoS whose root cause is on-chain. Enforce a non-empty, monotonic block span in _commitBatchWithBatchData (shared by commitBatch / commitState / commitBatchWithProof): when the parent header carries a lastBlockNumber (V1+), require lastBlockNumber > parentLastBlockNumber. This mirrors the node's blockCount = lastBlockNumber - parentLastBlockNumber derivation. The V0->V1 transition (parent version 0) carries no lastBlockNumber and is a historical one-time event, so it is not guarded here. Tests: empty span (== parent, the off-by-one the original underflow guard missed) reverts with "empty block span"; a strictly increasing span still commits (the only happy-path coverage for V1 parents — the existing suite is all V0). Full Rollup suite passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fixes two DoS panics in
node/derivation/batch_info.go::ParseBatchthat let a malformed L1commitBatchcrash any node in layer1 verify mode (the derivation goroutine has norecover()). Both originate from an unvalidatedblockCount, whereLastBlockNumberis attacker-controlled L1 calldata.blockCount == 0(lastBlockNumber == parent): the underflow guard rejected<but not==, producing emptyblockContexts;derive()then returned a nil header thatderivation.godereferenced → nil-pointer panic. Zero-block batches are not supported, so this is rejected at parse.blockCount:blockCount * 60wrappeduint64to a small value, bypassing the length guard, thenmake([]*BlockContext, int(blockCount))attempted an out-of-range allocation →makeslicepanic / OOM.Fix (targeted, one file)
In
ParseBatch:blockCountby payload length via division (blockCount > len(batchBytes)/60) before the multiply — overflow-safe, andmakeis bounded by real payload size.blockCount == 0once after both branches — covers every path (parent-equal, zero prefix, v0-nil).Both become clean parse errors instead of process-killing panics.
derivation.gois unchanged.Note: single-block batches are unaffected
The guard rejects only
blockCount == 0. A single-block batch isblockCount == 1and passes untouched — including the fork-boundary case (e.g. the zk→MPT / Morph203 transition block committed in its own batch). The V0→V1 transition path independently computeslastBlockNumber - startBlock + 1, so it is structurally≥ 1and can never be zero.Test
TestParseBatchBlockCountBounds: zero count →ParseBatchreturns an error; overflowing count → error (no panic). ExistingParseBatchtests and the fullderivationpackage test suite pass.Follow-up (out of scope)
Root cause is contract-side:
Rollup.sol::_commitBatchdoes not constrainlastBlockNumbervs. parent and_getBLSMsgHashis still areturn bytes32(0)stub, so the field isn't signature-bound. A span cap (and tighter signing) is recommended separately (see #994).Closes #994