fix: bound ABI decoding in Zone transaction paths - #1379
Conversation
5ee3a6e to
304cff9
Compare
|
cyclops audit |
|
cc @0xalpharush Cyclops audit event published. View workflow run Config: config: |
tempoxyz-bot
left a comment
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
Found 3 actionable items. Inline comments cover findings on changed lines; the non-diff finding is included below.
🛡️ [DEFENSE-IN-DEPTH] SPF proof replay canonically re-encodes advanceTempo instead of replaying committed bytes
Severity: Medium
File: crates/spf/src/execution/evm.rs:229
execute_advance_tempo rebuilds the system transaction with IZoneInbox::advanceTempoCall { ... }.abi_encode() from fields extracted by crates/sequencer/src/prover.rs:660, which non-strictly decodes and discards the original calldata. Pre-T11 block validation does not enforce canonical outer advanceTempo calldata, so a consensus-valid non-canonical system transaction can be replayed as different bytes in SPF, changing the transaction root and assembled block hash and making the submitted batch unprovable.
Recommended Fix: Carry the original system-transaction calldata from extract_zone_block into SPF and execute exactly those committed bytes for both system transactions. Separately, consider adding a pre-T11 is_advance_tempo_calldata canonicality gate and relying on strict dispatch from T11 onward.
Reviewer Callouts
- ⚡
crates/precompiles/src/inbox/mod.rs:153-166: The comment says partial deposit processing is supported and that requiring equality would forbid it, but the code immediately requirestempo_current_hash == current_hash. If partial processing is intended, this path needs a human design check. - ⚡
crates/l1/src/event.rs:104-124:validate_advance_tempo_inputsdeliberately excludes the sequencer-controlledrejectedflag from deposit identity; confirm that no downstream indexer/prover path treats that flag as L1-authenticated data. - ⚡ Fork-gating asymmetry:
executor.rsis fork-gated forfinalizeWithdrawalBatch, whileoutbox/mod.rs,inbox/mod.rs, andreplication.rsare not. The un-gated strict-decoder changes were mostly shown equivalent, but permanent equivalence tests would help prevent future historical-consensus drift.
| signed.tx().input.as_ref(), | ||
| AbiDecoderConfig::new() | ||
| .memory_limit(ABI_DECODER_MEMORY_LIMIT) | ||
| .strict(true), |
There was a problem hiding this comment.
🚨 [POTENTIAL-VULNERABILITY] Fork-unaware strict advanceTempo decoding can make followers reject valid pre-T11 blocks
This unconditional strict decoder runs before engine.new_payload, but pre-T11 ZoneInbox execution uses fork-aware non-strict decoding and the executor classifies advanceTempo by selector prefix. A leader can add otherwise-ignored ABI trailing data that execution accepts while followers reject during replication import, stalling backfill.
Recommended Fix:
Use the same fork-aware decoder config as ZoneInbox dispatch for the block's active spec, or keep only the memory limit pre-T11. Add a regression test that replication and execution accept the same advanceTempo calldata set across forks.
There was a problem hiding this comment.
leader is normal ABI encoding
| call.abi_encode() == calldata | ||
| IZoneOutbox::finalizeWithdrawalBatchCall::abi_decode_with_config( | ||
| calldata, | ||
| AbiDecoderConfig::new().strict(true), |
There was a problem hiding this comment.
🛡️ [DEFENSE-IN-DEPTH] Outbox canonical-call predicate omits the shared ABI decoder memory limit
This predicate uses AbiDecoderConfig::new().strict(true) without memory_limit(ABI_DECODER_MEMORY_LIMIT), leaving Alloy's default 1 GiB decoder bound on a path still used by pre-T11 executor classification. Verification did not show a practical allocation DoS, but the classifier now differs from the bounded configuration used by precompile dispatch and the other ABI hardening in this PR.
Recommended Fix:
Add .memory_limit(ABI_DECODER_MEMORY_LIMIT) before .strict(true), or reuse the shared decoder configuration, so classification and dispatch remain consistently bounded.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 304cff9ff2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
a96d045 to
20511e6
Compare
| if tx.calls().any(|(kind, input)| { | ||
| kind.to() == Some(&ZONE_OUTBOX_ADDRESS) && is_finalize_withdrawal_batch_calldata(input) | ||
| kind.to() == Some(&ZONE_OUTBOX_ADDRESS) | ||
| && if spec.is_t11() { |
There was a problem hiding this comment.
tbh i think it does not need to be gated
There was a problem hiding this comment.
yeah we can remove. the encoding is kinda guaranteed to be correct rn
20511e6 to
6482149
Compare
Voight-Kampff: dismissing this approval review because it targets an earlier commit. Voight-Kampff verdicts apply only to the pull request's current head (6482149). Re-submit the review on the current head and complete the Voight-Kampff prompt if you want this verdict to count.
3 participants
Summary
Safety
This PR is stacked on #1372.
For unrestricted AccountKeychain selectors, the Zone wrapper forwards the original calldata and caller without decoding them first. The upstream Tempo precompile remains authoritative: it charges input gas, enforces selector scheduling, performs bounded ABI decoding, and applies AccountKeychain authorization and state checks. Zone privacy-sensitive getters still decode before applying their caller restrictions.
System-transaction classification now performs only constant-time destination and selector checks for both advanceTempo and finalizeWithdrawalBatch. Classification only proposes the next block phase. The phase commits after successful authoritative precompile execution, so calldata rejected during execution cannot advance it.
Nested Inbox decoding uses strict mode and the shared ABI_DECODER_MEMORY_LIMIT. Strict mode replaces the previous decode-and-reencode canonicality check. Replication applies the same bound before processing peer-supplied advanceTempo data.
Testing
A full zone-node check remains blocked by an unrelated existing zone-spf API mismatch around HashedStorage.wiped/wipe.