fix(levm): reorder fee token validations to prevent storage rollback issue #6045
Conversation
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
In `prepare_execution_fee_token`, fee deduction via `deduct_caller_fee_token` was happening BEFORE several validation checks. Since `transfer_fee_token` uses `vm.db.get_account_mut` directly (bypassing the backup mechanism), if any subsequent validation failed, the fee token storage changes could not be rolled back by `restore_cache_state`. This moves `deduct_caller_fee_token` to AFTER all validation checks, ensuring fee tokens are never deducted for transactions that will fail validation. The fix prevents a scenario where fee tokens could be locked in the fee token contract without a corresponding transfer to the sequencer.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a critical audit finding in the LEVM implementation where fee token storage changes were not properly rolled back when validation failed. The fix reorders the execution flow in prepare_execution_fee_token to ensure all transaction validations occur before fee token deduction.
Changes:
- Moved
deduct_caller_fee_tokencall from before validations to after all validation checks - Added comprehensive warning comments explaining why this ordering is critical
- Documented that fee token storage changes bypass the backup mechanism and cannot be rolled back
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
crates/vm/levm/src/hooks/l2_hook.rs
Outdated
Comment on lines
522
to
527
| // NOTE: Fee deduction is intentionally placed AFTER all validations above. | ||
| // This ensures that if any validation fails, fee tokens are never locked. | ||
| deduct_caller_fee_token(vm, gaslimit_price_product.saturating_mul(fee_token_ratio))?; | ||
|
|
||
| default_hook::transfer_value(vm)?; | ||
|
|
There was a problem hiding this comment.
While this PR correctly moves fee deduction after all transaction validations, there's still a theoretical edge case: if transfer_value fails due to recipient balance overflow (line 526), the fee token storage changes cannot be rolled back by restore_cache_state because they bypass the backup mechanism via vm.db.get_account_mut.
Though highly unlikely in practice (requires recipient balance near U256::MAX), consider either moving deduct_caller_fee_token after transfer_value, or documenting this limitation. The same pattern exists in the default hook, but only affects ETH which can be rolled back, whereas fee token storage changes cannot.
Suggested change
| // NOTE: Fee deduction is intentionally placed AFTER all validations above. | |
| // This ensures that if any validation fails, fee tokens are never locked. | |
| deduct_caller_fee_token(vm, gaslimit_price_product.saturating_mul(fee_token_ratio))?; | |
| default_hook::transfer_value(vm)?; | |
| // NOTE: Fee deduction is intentionally placed AFTER all validations above | |
| // and AFTER value transfer. This ensures that if any validation or value | |
| // transfer fails, fee tokens are never locked. | |
| default_hook::transfer_value(vm)?; | |
| deduct_caller_fee_token(vm, gaslimit_price_product.saturating_mul(fee_token_ratio))?; |
Benchmark Results ComparisonNo significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: FibonacciRecursive
Benchmark Results: ManyHashes
Benchmark Results: MstoreBench
Benchmark Results: Push
Benchmark Results: SstoreBench_no_opt
|
Oppen
reviewed
Jan 28, 2026
crates/vm/levm/src/hooks/l2_hook.rs
Outdated
| // Transaction is type 4 if authorization_list is Some | ||
| // NOT CHECKED: fee token transactions are not type 4 | ||
|
|
||
| // (3) INSUFFICIENT_ACCOUNT_FUNDS |
Contributor
There was a problem hiding this comment.
This number is inconsistent.
ElFantasma
approved these changes
Feb 9, 2026
… value transfer failure Addresses PR review feedback: - Remove inconsistent (3) numbering since the check is no longer in position 3 - Split deduct_caller_fee_token: ETH value deduction stays before transfer_value (correct accounting), but lock_fee_token moves after it so fee tokens are never locked if transfer_value fails (fee token storage bypasses cache backup) - Remove now-unused deduct_caller_fee_token function
Lines of code reportTotal lines added: Detailed view |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Motivation
Addresses audit finding: fee token storage changes are not rolled back when validation fails after fee deduction.
In
prepare_execution_fee_token, fee deduction viadeduct_caller_fee_tokenwas happening BEFORE several validation checks (e.g.,validate_sufficient_max_fee_per_gas,validate_priority_fee). Sincetransfer_fee_tokenusesvm.db.get_account_mutdirectly (bypassing the backup mechanism), if any subsequent validation failed, the fee token storage changes could not be rolled back byrestore_cache_state.This could lead to fee tokens being locked in the fee token contract without a corresponding transfer to the sequencer.
Description
Moves
deduct_caller_fee_tokento AFTER all validation checks that can fail, ensuring fee tokens are never deducted for transactions that will fail validation.The key insight is that
vm.db.get_account_mut(used bytransfer_fee_token) does NOT backup storage changes, whilevm.get_account_mutdoes. The code comment explicitly warns: "Use directly only if outside of the EVM, otherwise usevm.get_account_mutbecause it contemplates call frame backups."Checklist