Skip to content

test(l2): pin operator-fee priority-fee underflow on validation gap#6537

Draft
avilagaston9 wants to merge 1 commit intomainfrom
security/operator-fee-priority-underflow
Draft

test(l2): pin operator-fee priority-fee underflow on validation gap#6537
avilagaston9 wants to merge 1 commit intomainfrom
security/operator-fee-priority-underflow

Conversation

@avilagaston9
Copy link
Copy Markdown
Contributor

Motivation

validate_sufficient_max_fee_per_gas_l2 only enforces
max_fee_per_gas >= base_fee_per_gas + operator_fee_per_gas. It does
not check max_priority_fee_per_gas >= operator_fee_per_gas. A user
can submit an EIP-1559 transaction that passes that validator with
max_priority_fee_per_gas set below the operator fee. The effective
gas_price = min(max_priority + base_fee, max_fee) then sits below
base_fee + operator_fee, and compute_priority_fee_per_gas computes
(gas_price - base_fee) - operator_fee which underflows. The error
surfaces as VMError::Internal(InternalError::Underflow) rather than
a TxValidationError. Internal errors are reserved for invariant
violations; a user-controlled input must never hit them.

Description

Adds a regression test priority_fee_below_operator_fee_underflows_in_finalize
in test/tests/levm/l2_hook_tests.rs that constructs the missing-case
inputs (base_fee=10, operator_fee=100, max_fee=200,
max_priority=50) and pins the current (buggy) outcome — the VM
returns VMError::Internal(InternalError::Underflow). The assertion is
written so that any change in error handling (for example, tightening
validate_sufficient_max_fee_per_gas_l2 to also reject
max_priority_fee_per_gas < operator_fee_per_gas, which would make
this case bounce out as a TxValidationError) breaks this test,
prompting an explicit update of the expected error.

The fix itself is intentionally not in this PR — surfacing the gap and
its reproducer first lets the right validation error variant be picked
deliberately. Two reasonable shapes:

  1. Extend validate_sufficient_max_fee_per_gas_l2 to also require
    tx_max_priority_fee_per_gas >= operator_fee_per_gas and return
    the existing TxValidationError::InsufficientMaxFeePerGas (or a
    more specific new variant).
  2. Saturate at zero in compute_priority_fee_per_gas so the
    coinbase simply receives no priority fee instead of crashing —
    weaker, but matches how some L2s "round down to zero" rather
    than reject.

Reproduction

cargo test -p ethrex-test --test ethrex_tests \
  priority_fee_below_operator_fee_underflows_in_finalize

The test passes on main (the bug is real and reachable).

Impact

A user-submitted L2 transaction with operator fee active and
max_priority_fee_per_gas < operator_fee_per_gas (with
max_fee_per_gas >= base_fee + operator_fee) reaches the
internal-error path during finalize. Whether the consequence is
"tx silently dropped from the block" or "block production aborts"
depends on the caller's handling of VMError::Internal, but in
either case the validator should have rejected the tx upfront —
internal errors are not the right escape hatch for malformed user
input.

Checklist

  • Updated STORE_SCHEMA_VERSION (crates/storage/lib.rs) if the PR includes breaking changes to the Store requiring a re-sync.

`validate_sufficient_max_fee_per_gas_l2` enforces only
`max_fee_per_gas >= base_fee_per_gas + operator_fee_per_gas`. It does
not check `max_priority_fee_per_gas >= operator_fee_per_gas`, so a
user-submitted EIP-1559 transaction can satisfy the validator and
still set `max_priority_fee_per_gas` below the operator fee. The
resulting `gas_price = min(max_priority + base_fee, max_fee)` then
sits below `base_fee + operator_fee`, and `compute_priority_fee_per_gas`
computes `(gas_price - base_fee) - operator_fee`, which underflows —
surfacing as `VMError::Internal(InternalError::Underflow)` rather
than a `TxValidationError`. Internal errors are reserved for
invariant violations; user-controlled inputs must never reach them.

The new test `priority_fee_below_operator_fee_underflows_in_finalize`
constructs a VM with base_fee=10, operator_fee=100, max_fee=200,
max_priority=50 — exactly the case the validator misses — and asserts
the current (buggy) behaviour where `vm.execute()` returns
`VMError::Internal(InternalError::Underflow)`. The assertion is
written so that any change in error handling (e.g. replacing the
internal-error path with a proper `TxValidationError` once the
validator is tightened) will fail this test, prompting an explicit
update.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

L2 Rollup client

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant