fix(levm): charge the EIP-8250 first-use surcharge on the default-code approval - #7085
Conversation
Greptile SummaryThe PR moves EIP-8250 first-use surcharge accounting out of nonce consumption so ordinary APPROVE execution and default-code verification can charge their respective frame budgets explicitly.
Confidence Score: 4/5The PR should not merge until an out-of-gas surcharge rolls back the transaction-level approval metadata along with the approval’s database changes. Charging the surcharge after apply_approve creates a reachable failure path where database rollback removes the payer debit but leaves payer_address set, allowing final accounting to treat a failed approval as valid and credit an unmatched refund. Files Needing Attention: crates/vm/levm/src/opcode_handlers/frame_tx.rs
|
| Filename | Overview |
|---|---|
| crates/vm/levm/src/opcode_handlers/frame_tx.rs | Adds explicit surcharge charging to APPROVE and default verification, but ordinary APPROVE can retain payer metadata when the post-mutation charge fails. |
| crates/vm/levm/src/vm.rs | Refactors keyed-nonce consumption to return its first-use surcharge and adds a matching read-only surcharge calculation. |
Sequence Diagram
sequenceDiagram
participant Frame
participant Approve as apply_approve
participant Context as FrameTxContext
participant Gas as Gas accounting
participant Rollback
Frame->>Approve: APPROVE scope
Approve->>Context: Record payer/approval
Approve->>Approve: Debit balance and consume nonce
Approve-->>Frame: Return first-use surcharge
Frame->>Gas: Charge surcharge
Gas-->>Frame: OutOfGas
Frame->>Rollback: Restore call-frame database state
Note over Context,Rollback: Approval metadata is not restored
Prompt To Fix All With AI
### Issue 1
crates/vm/levm/src/opcode_handlers/frame_tx.rs:257-258
**Out-of-gas leaves stale payer state**
When a first-use surcharge exceeds the frame's remaining gas, `apply_approve` records the payer and mutates database state before `increase_consumed_gas` returns `OutOfGas`. Frame rollback restores the database-backed debit and nonce changes but not `FrameTxContext.payer_address`, so final accounting accepts the failed approval and credits an unmatched refund to the payer.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(levm): charge the EIP-8250 first-use..." | Re-trigger Greptile
| let surcharge = apply_approve(vm, scope_val, frame_target)?; | ||
| vm.current_call_frame.increase_consumed_gas(surcharge)?; |
There was a problem hiding this comment.
Out-of-gas leaves stale payer state
When a first-use surcharge exceeds the frame's remaining gas, apply_approve records the payer and mutates database state before increase_consumed_gas returns OutOfGas. Frame rollback restores the database-backed debit and nonce changes but not FrameTxContext.payer_address, so final accounting accepts the failed approval and credits an unmatched refund to the payer.
Knowledge Base Used: VM: LEVM Execution and the Database Glue
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/vm/levm/src/opcode_handlers/frame_tx.rs
Line: 257-258
Comment:
**Out-of-gas leaves stale payer state**
When a first-use surcharge exceeds the frame's remaining gas, `apply_approve` records the payer and mutates database state before `increase_consumed_gas` returns `OutOfGas`. Frame rollback restores the database-backed debit and nonce changes but not `FrameTxContext.payer_address`, so final accounting accepts the failed approval and credits an unmatched refund to the payer.
**Knowledge Base Used:** [VM: LEVM Execution and the Database Glue](https://app.greptile.com/lambdaclass/-/custom-context/knowledge-base/lambdaclass/ethrex/-/docs/vm-levm.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Valid. apply_approve records the payer and mutates database state, and the surcharge charge that follows can return OutOfGas; the frame rollback restores the database-backed debit and nonce consumption but not FrameTxContext, which is not database-backed, so the failed approval survived and the payer was refunded at end of transaction. The approval context is now snapshotted before apply_approve and restored when the surcharge charge fails.
…e runs out of gas
| } | ||
|
|
||
| /// The surcharge `consume_keyed_nonces` will charge, without consuming anything. | ||
| pub(crate) fn keyed_nonce_first_use_surcharge(&mut self) -> Result<u64, VMError> { |
There was a problem hiding this comment.
This reimplements the slot derivation from consume_keyed_nonces character-for-character - same [0u8; 64] preimage, same [12..32]/[32..64] layout, same keccak_hash. Two copies of a consensus-critical derivation that must agree: if either drifts, the pre-check prices a different slot than the one actually consumed, and the default-code path charges a surcharge unrelated to what it did.
Worth collapsing to one private helper, something like fn keyed_nonce_slot(sender: Address, key: &U256) -> H256, called from both. Cheap now, and it makes the invariant structural instead of something a future edit has to remember.
There was a problem hiding this comment.
Collapsed into keyed_nonce_slot(sender, key), called from both consume_keyed_nonces and keyed_nonce_first_use_surcharge.
|
|
||
| // The surcharge comes out of this frame's budget like any other gas, so a frame that cannot | ||
| // afford it approves nothing. | ||
| let surcharge = vm.keyed_nonce_first_use_surcharge()?; |
There was a problem hiding this comment.
apply_approve now returns the surcharge it actually consumed, but this path discards it and reports the pre-computed estimate instead (Ok((true, surcharge, ...)) below). The two agree today, though only because of an invariant that lives in another crate: validate_static_constraints rejects nonce_keys unless strictly ascending (transaction.rs:2445, w[0] >= w[1]), so no two keys can hash to the same slot and the pre-check cannot count a first-use that consumption then finds already set.
That is a real guarantee, not a gap - I verified it before writing this. But it is load-bearing for gas accounting and nothing here says so. Relax that ordering rule to allow duplicates and the estimate over-charges by 20k per repeated key, silently.
The pre-check has to stay where it is, since the affordability gate must run before any state is touched. But the charge could come from apply_approve's return value rather than the estimate, which makes this locally correct regardless of what the ordering rule does later:
let surcharge = vm.keyed_nonce_first_use_surcharge()?; // affordability gate only
if surcharge > frame.gas_limit {
return Ok((false, frame.gas_limit, Vec::new()));
}
let charged = apply_approve(vm, allowed_scope, target)?; // what was actually consumed
...
Ok((true, charged, Vec::new()))At minimum, a comment naming the strict-ordering rule as the reason the estimate is trustworthy.
There was a problem hiding this comment.
Changed as you wrote it. The pre-check stays where it is as the affordability gate, and the value reported is now what apply_approve returned rather than the estimate, so gas accounting here no longer rests on the strict-ascending rule in another crate.
2658e7e to
68a8461
Compare
…e what approval consumed
consume_keyed_noncesdeducted the 20k first-use surcharge from the current call frame. The default-code approval path runs outside an ordinary frame, so the deduction landed nowhere and the surcharge was silently free.The function now returns the surcharge instead of deducting it, and the default-code path charges it explicitly before applying the approval. Observed cross-client as a
16688117199824 weibalance divergence on a keyed-nonce transaction whose sender has no code.