fix(l1): add reverse check in BAL withdrawal validation#6463
fix(l1): add reverse check in BAL withdrawal validation#6463
Conversation
validate_bal_withdrawal_index only checked BAL->DB direction (every BAL claim at the withdrawal index matches the DB). A malicious builder could omit a withdrawal recipient from the BAL, causing the BAL-derived state root to exclude the withdrawal balance change while passing validation. Add Part B (DB->BAL): for each account modified during the withdrawal/request phase, verify the BAL has a corresponding entry. This mirrors the bidirectional validation already done in validate_tx_execution.
🤖 Kimi Code ReviewOverall Assessment: The PR implements critical bidirectional validation for Block Access Lists (BAL) to prevent malicious builders from omitting state changes that occur during the withdrawal/request phase. The logic is sound, but missing storage validation in Part B is a potential vulnerability if withdrawals can modify storage (e.g., via system contracts). Critical Issues1. Missing Storage Validation in Part B (Lines 1582-1689)Part B validates that every modified account has corresponding BAL entries for balance, nonce, and code, but storage changes are not validated. If the withdrawal/request phase modifies storage (e.g., through EIP-7002/EIP-7251 system contracts), a malicious builder could omit these changes from the BAL. Recommendation: Add storage validation to Part B: // Check if any storage slots were modified but not recorded in BAL
if let Some(bal_acct) = index.addr_to_idx.get(addr) {
let bal_acct = &bal.accounts()[*bal_acct];
// Verify all storage modifications at withdrawal_idx are recorded
for (slot, value) in &account.storage {
if !bal_acct.storage_changes.iter().any(|c| c.index == withdrawal_idx && c.slot == *slot && c.value == *value) {
// Check if this slot was actually modified vs pre-state
// ...
}
}
}Medium Issues2. Performance: Full State Iteration (Line 1583)Iterating over 3. Silent Error Handling (Lines 1597, 1623, 1641, 1680)Multiple uses of
Suggestion: Distinguish between "account not found" (None) and store errors (Err): let pre = match db.store.get_account_state(*addr) {
Ok(Some(acct)) => (acct.balance, acct.nonce, acct.code_hash),
Ok(None) => (U256::ZERO, 0, *EMPTY_KECCACK_HASH),
Err(e) => return Err(EvmError::Store(e)),
};Minor Issues4. Code Duplication (Lines 1612-1686)The pattern for fetching "last BAL value or pre-state" is repeated for balance, nonce, and code. Consider extracting a helper function to reduce duplication and potential inconsistencies. 5. Inconsistent Code Hash Handling (Lines 1669-1675)The code recomputes the keccak hash of BAL code changes, but compares against 6. Panic Risk (Line 1617)
let Some(acct) = bal.accounts().get(bal_acct_idx) else {
return Err(EvmError::Custom(format!("BAL index corruption: invalid index {bal_acct_idx}")));
};Positive Feedback
SummaryApprove with changes: Add storage validation to Part B to ensure complete state coverage, and consider handling store errors explicitly rather than silently defaulting. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
Lines of code reportTotal lines added: Detailed view |
🤖 Codex Code Review
Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Claude Code ReviewHere is my review of PR #6463: Review: fix(l1): add reverse check in BAL withdrawal validationSummaryThis PR closes a real security vulnerability: a malicious block builder could omit a withdrawal recipient from the BAL, and since The fix is logically sound and mirrors the structure of the already-existing Security / CorrectnessStorage changes are not reverse-checked in Part B
The withdrawal phase runs This is the same category of vulnerability that this PR is fixing, applied to storage rather than balance. Adding the same pattern as DB error silenced at the "absent from BAL" path Lines 1597–1602: let pre = db
.store
.get_account_state(*addr)
.ok() // DB errors discarded here
.map(|a| (a.balance, a.nonce, a.code_hash))
.unwrap_or_default(); // defaults to (0, 0, EMPTY_HASH)If Correctness Assumptions
Lines 1622–1631 (and analogous nonce/code blocks): the seeded balance is computed as MinorCode mismatch error lacks diagnostic values Lines 1683–1688: the code mismatch error reports that code changed but doesn't log the actual vs expected hash values: return Err(EvmError::Custom(format!(
"BAL validation failed for withdrawal: account {addr:?} code \
changed during withdrawal/request phase but BAL has no \
code change at index {withdrawal_idx}"
)));The balance and nonce errors log their values ( Storage not checked for accounts absent from BAL entirely Lines 1593–1614 handle accounts that have no BAL entry at all — they only compare VerdictThe fix correctly addresses the stated vulnerability for the balance/nonce/code dimension. Two structural gaps remain: (a) storage mutations during the withdrawal/request phase are not reverse-checked in Part B, and (b) the "absent from BAL" path also skips storage. Both are the same family of omission as the original bug. The test plan correctly notes the EEST test is pending — coverage of these storage cases should be included there. Automated review by Claude (Anthropic) · sonnet · custom prompt |
🤖 Kimi Code ReviewThe PR implements critical bidirectional validation for Block Access Lists (BAL) during the withdrawal/request phase. This prevents a consensus vulnerability where malicious builders could omit withdrawal recipients from the BAL, causing state root mismatches. Security & CorrectnessCorrectness: The bidirectional check (Part A: BAL→DB, Part B: DB→BAL) correctly ensures that:
Line 1588-1608: The warm-access artifact detection is sound—comparing pre-state vs post-state prevents false positives on accounts that were merely touched but not mutated during the phase. Line 1628-1639, 1654-1665, 1680-1696: The fallback logic to Suggestions1. Avoid redundant store lookups (Performance) // Instead of three separate lookups:
let pre_state = db.store.get_account_state(*addr).ok();
let pre_balance = pre_state.as_ref().map(|a| a.balance).unwrap_or_default();
let pre_nonce = pre_state.as_ref().map(|a| a.nonce).unwrap_or_default();
let pre_code_hash = pre_state.as_ref().map(|a| a.code_hash).unwrap_or(*EMPTY_KECCACK_HASH);2. Code deduplication (Maintainability) 3. Error message clarity return Err(EvmError::Custom(format!(
"BAL validation failed: account {addr:?} modified during withdrawal \
(pre: {pre:?}, post: {post:?}) but absent from BAL"
)));4. Hash calculation consistency VerdictThe security fix is correctly implemented and addresses a legitimate attack vector. The logic for distinguishing warm accesses from genuine mutations is sound. Minor performance optimizations (caching store lookups) are recommended but not blocking. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
Greptile SummaryThis PR adds the missing DB→BAL reverse check (
Confidence Score: 4/5Safe to merge for the balance-omission fix, but leaves a storage-mutation gap in Part B that mirrors the original vulnerability class for system-call predeploys. The primary vulnerability (omitted withdrawal recipient balance) is correctly fixed and all 1963 EF tests pass. However, the new Part B diverges from the crates/vm/backends/levm/mod.rs — specifically the Part B storage check absence in
|
| Filename | Overview |
|---|---|
| crates/vm/backends/levm/mod.rs | Adds Part B (DB→BAL) reverse check to validate_bal_withdrawal_index, correctly patching the omitted-withdrawal-recipient vulnerability; however, the new Part B omits the storage-slot check that the equivalent validate_tx_execution Part B performs, leaving system-call storage mutations (EIP-7002, EIP-7251 predeploys) uncovered. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["validate_bal_withdrawal_index(db, bal, withdrawal_idx, index)"] --> B
subgraph PartA["Part A: BAL → DB"]
B["For each acct in bal.accounts()"] --> C{Has change at withdrawal_idx?}
C -- balance/nonce/code/storage --> D[Check DB matches BAL claim]
D --> E{Match?}
E -- no --> F[Return Err: mismatch]
E -- yes --> B
end
PartA --> G
subgraph PartB["Part B: DB → BAL (new)"]
G["For each (addr, account) in current_accounts_state"] --> H{is_unmodified?}
H -- yes --> G
H -- no --> I{addr in BAL index?}
I -- no --> J[Compare pre-state vs post-state balance/nonce/code_hash only]
J --> K{Changed?}
K -- yes --> L[Return Err: absent from BAL]
K -- no --> G
I -- yes --> M[Check balance vs last BAL entry]
M --> N[Check nonce vs last BAL entry]
N --> O[Check code vs last BAL entry]
O --> P["⚠️ Storage NOT checked"]
P --> G
end
PartB --> Q[Ok]
Prompt To Fix All With AI
This is a comment left during a code review.
Path: crates/vm/backends/levm/mod.rs
Line: 1664-1689
Comment:
**Storage mutations missing from Part B**
Part B checks balance, nonce, and code, but does not verify storage slot changes — unlike `validate_tx_execution`'s Part B (lines 1442–1469) which iterates `account.storage` and checks every slot against its BAL entry. During the withdrawal/request phase, system calls (EIP-7002, EIP-7251) write to storage in their predeploy contracts. A block builder could omit those storage changes from the BAL; the BAL-derived state root would then exclude those slot mutations, but this check would not catch the omission.
The analogous pattern from `validate_tx_execution` Part B is:
```rust
// Storage: for each slot written during withdrawal/request phase,
// verify a corresponding BAL entry exists.
for (key_h256, &value) in &account.storage {
let slot_u256 = U256::from_big_endian(key_h256.as_bytes());
let pos = acct
.storage_changes
.partition_point(|sc| sc.slot < slot_u256);
if pos < acct.storage_changes.len() && acct.storage_changes[pos].slot == slot_u256 {
let sc = &acct.storage_changes[pos];
if !has_exact_change_storage(&sc.slot_changes, withdrawal_idx) {
let seeded = sc
.slot_changes
.last()
.map(|c| c.post_value)
.unwrap_or_default();
if value != seeded {
return Err(EvmError::Custom(format!(
"BAL validation failed for withdrawal: account {addr:?} storage slot \
{slot_u256} changed during withdrawal/request phase ({value}) but BAL \
has no change at index {withdrawal_idx} (last_bal={seeded})"
)));
}
}
}
// Slot not in BAL storage_changes: loaded from store, skip.
}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(l1): add reverse check in BAL withdr..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Code | ||
| if !has_exact_change_code(&acct.code_changes, withdrawal_idx) { | ||
| let seeded_hash = acct | ||
| .code_changes | ||
| .last() | ||
| .map(|c| { | ||
| if c.new_code.is_empty() { | ||
| *EMPTY_KECCACK_HASH | ||
| } else { | ||
| ethrex_common::utils::keccak(&c.new_code) | ||
| } | ||
| }) | ||
| .unwrap_or_else(|| { | ||
| db.store | ||
| .get_account_state(*addr) | ||
| .map(|a| a.code_hash) | ||
| .unwrap_or(*EMPTY_KECCACK_HASH) | ||
| }); | ||
| if account.info.code_hash != seeded_hash { | ||
| return Err(EvmError::Custom(format!( | ||
| "BAL validation failed for withdrawal: account {addr:?} code \ | ||
| changed during withdrawal/request phase but BAL has no \ | ||
| code change at index {withdrawal_idx}" | ||
| ))); | ||
| } | ||
| } |
There was a problem hiding this comment.
Part B checks balance, nonce, and code, but does not verify storage slot changes — unlike validate_tx_execution's Part B (lines 1442–1469) which iterates account.storage and checks every slot against its BAL entry. During the withdrawal/request phase, system calls (EIP-7002, EIP-7251) write to storage in their predeploy contracts. A block builder could omit those storage changes from the BAL; the BAL-derived state root would then exclude those slot mutations, but this check would not catch the omission.
The analogous pattern from validate_tx_execution Part B is:
// Storage: for each slot written during withdrawal/request phase,
// verify a corresponding BAL entry exists.
for (key_h256, &value) in &account.storage {
let slot_u256 = U256::from_big_endian(key_h256.as_bytes());
let pos = acct
.storage_changes
.partition_point(|sc| sc.slot < slot_u256);
if pos < acct.storage_changes.len() && acct.storage_changes[pos].slot == slot_u256 {
let sc = &acct.storage_changes[pos];
if !has_exact_change_storage(&sc.slot_changes, withdrawal_idx) {
let seeded = sc
.slot_changes
.last()
.map(|c| c.post_value)
.unwrap_or_default();
if value != seeded {
return Err(EvmError::Custom(format!(
"BAL validation failed for withdrawal: account {addr:?} storage slot \
{slot_u256} changed during withdrawal/request phase ({value}) but BAL \
has no change at index {withdrawal_idx} (last_bal={seeded})"
)));
}
}
}
// Slot not in BAL storage_changes: loaded from store, skip.
}Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/vm/backends/levm/mod.rs
Line: 1664-1689
Comment:
**Storage mutations missing from Part B**
Part B checks balance, nonce, and code, but does not verify storage slot changes — unlike `validate_tx_execution`'s Part B (lines 1442–1469) which iterates `account.storage` and checks every slot against its BAL entry. During the withdrawal/request phase, system calls (EIP-7002, EIP-7251) write to storage in their predeploy contracts. A block builder could omit those storage changes from the BAL; the BAL-derived state root would then exclude those slot mutations, but this check would not catch the omission.
The analogous pattern from `validate_tx_execution` Part B is:
```rust
// Storage: for each slot written during withdrawal/request phase,
// verify a corresponding BAL entry exists.
for (key_h256, &value) in &account.storage {
let slot_u256 = U256::from_big_endian(key_h256.as_bytes());
let pos = acct
.storage_changes
.partition_point(|sc| sc.slot < slot_u256);
if pos < acct.storage_changes.len() && acct.storage_changes[pos].slot == slot_u256 {
let sc = &acct.storage_changes[pos];
if !has_exact_change_storage(&sc.slot_changes, withdrawal_idx) {
let seeded = sc
.slot_changes
.last()
.map(|c| c.post_value)
.unwrap_or_default();
if value != seeded {
return Err(EvmError::Custom(format!(
"BAL validation failed for withdrawal: account {addr:?} storage slot \
{slot_u256} changed during withdrawal/request phase ({value}) but BAL \
has no change at index {withdrawal_idx} (last_bal={seeded})"
)));
}
}
}
// Slot not in BAL storage_changes: loaded from store, skip.
}
```
How can I resolve this? If you propose a fix, please make it concise.
🤖 Codex Code Review
Direction looks right, but I would not merge this until storage is included in the reverse check for both branches. Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Claude Code ReviewNow I have all the context I need for the review. Here's my analysis: Review: fix(l1): add reverse check in BAL withdrawal validationSummaryThis PR addresses a real security gap: Security Concern: Part B lacks storage reverse-checkThis is the most significant issue with the PR as written.
The relevant system calls during the withdrawal/request phase (
These predeploy contracts are almost certainly already present in the BAL from prior transaction accesses. That means they won't fall into the "absent from BAL entirely" branch — they'll take the A malicious block builder could record the predeploy in the BAL but omit the storage writes at Suggested addition at the end of the per-account block (after the code check), mirroring // Storage: reverse-check that each slot in execution state
// has a corresponding BAL claim if it was modified.
for (key_h256, &value) in &account.storage {
let slot_u256 = U256::from_big_endian(key_h256.as_bytes());
let pos = acct
.storage_changes
.partition_point(|sc| sc.slot < slot_u256);
if pos < acct.storage_changes.len()
&& acct.storage_changes[pos].slot == slot_u256
{
let sc = &acct.storage_changes[pos];
if !has_exact_change_storage(&sc.slot_changes, withdrawal_idx) {
let seeded = sc
.slot_changes
.last()
.map(|c| c.post_value)
.unwrap_or_else(|| {
db.store
.get_storage_value(*addr, *key_h256)
.unwrap_or_default()
});
if value != seeded {
return Err(EvmError::Custom(format!(
"BAL validation failed for withdrawal: account {addr:?} \
storage slot {slot_u256} changed during withdrawal/request \
phase ({value}) but BAL has no change at index {withdrawal_idx}"
)));
}
}
}
// Slot not in BAL storage_changes at all: skip (same known gap as tx validation)
}Minor:
|
Part B was missing storage validation. System contract calls (extract_all_requests_levm) run during the withdrawal/request phase and can write storage. Without this check, a malicious builder could omit storage changes from the BAL. Also check storage for accounts absent from BAL entirely.
edg-l
force-pushed
the
fix/bal-withdrawal-reverse-check
branch
from
e4894d6 to
c67cdb0
Compare
…reverse check Add else-branch to catch storage slots absent from BAL storage_changes by verifying against store pre-state. Add diagnostic values to code hash mismatch error message.
| let pre = db | ||
| .store | ||
| .get_account_state(*addr) | ||
| .ok() |
…6486) ## Summary - The Hive consume-engine Amsterdam tests for EIP-7778 and EIP-8037 were failing because ethrex's per-tx gas limit checks were incompatible with Amsterdam's new gas accounting rules. - **EIP-7778** uses pre-refund gas for block accounting, so cumulative pre-refund gas can exceed the block gas limit even when a block builder correctly included all transactions. - **EIP-8037** introduces 2D gas accounting (`block_gas = max(regular, state)`), meaning cumulative total gas (regular + state) can legally exceed the block gas limit. - The fix skips the per-tx cumulative gas check for Amsterdam and adds a **post-execution** block-level overflow check using `max(sum_regular, sum_state)` in all three execution paths (sequential, pipeline, parallel). ## Local test results - **200/201** EIP-7778 + EIP-8037 Hive consume-engine tests pass - **105/105** EIP-7778 + EIP-8037 EF blockchain tests pass (4 + 101) - The single remaining Hive failure (`test_block_regular_gas_limit[exceed=True]`) expects `TransactionException.GAS_ALLOWANCE_EXCEEDED` but we return `BlockException.GAS_USED_OVERFLOW` — the block is correctly rejected, just with a different error classification. ## Test plan - [x] All EIP-7778 EF blockchain tests pass locally - [x] All EIP-8037 EF blockchain tests pass locally - [x] 200/201 Hive consume-engine Amsterdam tests pass locally - [ ] Full CI Amsterdam Hive suite passes --------- Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
4 participants
Summary
validate_bal_withdrawal_indexonly checked the BAL->DB direction (every BAL claim matches actual state). It did not verify the reverse: that every account modified during the withdrawal/request phase has a corresponding BAL entry.bal_to_account_updatesderives the state root entirely from BAL entries, the omitted account's withdrawal balance would be excluded from the state trie. The BAL-derived state root would match the tampered header, passing all validation.validate_tx_execution.Perf notes
db.storewhen no BAL entry exists for that field.Test plan