Skip to content

fix(revm): carry the token fee transfer's SSTORE refund into the tx refund - #207

Closed
kriss39 wants to merge 1 commit into
morph-l2:mainfrom
kriss39:fix/token-fee-refund-parity
Closed

kriss39 wants to merge 1 commit into
morph-l2:mainfrom
kriss39:fix/token-fee-refund-parity

Conversation

@kriss39

@kriss39 kriss39 commented Sep 13, 2026

Copy link
Copy Markdown

Summary

For fee tokens registered without a balanceSlot (EVM-call path), morph-geth and morph-reth disagree on gasUsed whenever the protocol-level fee transfer() earns an SSTORE refund. This PR makes morph-reth match morph-geth.

What morph-geth does

buyAltTokenGas() runs the ERC20 transfer() through evm.Call inside preCheck(), i.e. before StateDB.Prepare. Prepare resets the access list and transient storage but not the refund counter, so whatever the token contract adds to StateDB.refund during that call (for example the 4800-gas clearing refund when the payer's balance slot goes to zero) is still there when refundGas() computes the user's refund from st.state.GetRefund().

What morph-reth did

transfer_erc20_with_evm executes the call with a frame-local Gas and discards frame_result.gas().refunded(), so the main transaction's refund counter starts at zero.

Observable divergence

Scenario morph-geth gasUsed morph-reth gasUsed (before)
21000-gas value transfer; fee clears the payer's token balance slot 16800 (21000 - min(4800, 21000/5)) 21000
Same, but the main call hands one token back to the payer (re-sets the cleared slot) 30974 (geth nets +4800 - 4800 = 0) 24780 (main frame ends at -4800; revm's set_final_refund casts the negative refund to u64 and caps it at gas_used / 5)

Both are header gasUsed / receipts-root mismatches, so a morph-reth follower rejects the block. It also reaches the state root: the refund changes gas_used, which changes the fee charged and therefore the payer's/coinbase's balances. Replaying PR #200's geth-derived fixture (fee_token_internal_calls.json, golden roots from morph-geth 5744b8f66) with the carry-over disabled fails 4 cases on both Emerald and Jade with state root mismatch (deduct_clear = scenario 1, main_restores_cleared_slot = scenario 2; reproduced by @panos-xyz, see below).

The path is live on mainnet: L2TokenRegistry (0x5300…0021) token ids 2 and 6 have balanceSlot = 0 and take the EVM-call path (both are USDC, FiatTokenV2, active), Jade (jade_fork_time = 1775628000, 2026-04-08) strictly enforces state-root validation, and the trigger (the fee transfer consuming exactly the remaining token balance, controllable via gas_limit / gas_price) is something a user can construct.

The morph-geth numbers were confirmed with a standalone core test that builds the same slotless token and runs ApplyMessage (not included here; happy to share).

Fix

Return the net refund recorded by the fee-transfer frame from transfer_erc20_with_evm, keep it on MorphEvm::pre_fee_gas_refund, and record_refund it in the refund() hook before post_execution::refund applies the EIP-3529 cap. The reimbursement-path call is unaffected (geth reads the counter before that transfer runs).

Tests

Two unit tests in crates/revm/src/handler.rs reproduce both scenarios with a hand-assembled slotless ERC20 and assert the morph-geth values (16800 and 30974). Both fail without the fix with exactly the numbers above.

  • cargo fmt --all -- --check
  • cargo clippy -p morph-revm --all-targets -- -D warnings
  • cargo test -p morph-revm -p morph-evm
  • cargo test -p morph-node --features test-utils --test it morph_tx

…efund

morph-geth runs the ERC20 fee `transfer()` for slotless fee tokens through
`evm.Call` inside `buyAltTokenGas()`, before `StateDB.Prepare`. `Prepare`
resets the access list and transient storage but not the refund counter,
so any SSTORE refund the token contract earns during that protocol call
(e.g. clearing the payer's balance slot) is settled against the user's own
gas in `refundGas()`.

morph-reth ran the same call with a frame-local `Gas` and dropped its
refund, so the two clients disagreed on `gasUsed` whenever the fee
transfer touched a refundable slot:

- a 21000-gas value transfer whose fee clears the payer's balance settles
  at 16800 on geth and 21000 on reth;
- if the main call then hands tokens back to the payer, geth nets +4800
  against -4800 and refunds nothing, while reth's main frame ended at
  -4800 and revm's final-refund cast turned that into the maximum
  `gas_used / 5` refund (30974 vs 24780).

Both paths are reachable on mainnet: token ids 2 and 6 in the L2 token
registry have no `balanceSlot` and take the EVM-call path.

Record the net refund returned by the fee transfer frame on `MorphEvm`
and fold it into the transaction's refund counter before the EIP-3529
cap, matching go-ethereum's accounting.
@coderabbitai

coderabbitai Bot commented Sep 13, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 403b9646-d603-4134-9239-a54e5559c34a

Warning

Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@panos-xyz

Copy link
Copy Markdown
Contributor

Thanks for this — it is a well-diagnosed bug with a clean, focused fix, and pinning the geth numbers with a standalone core test is exactly the right instinct. We reproduced the divergence and it is consensus-breaking rather than cosmetic, so it is worth landing.

It reaches the state root. Disabling the refund carry-over in morph-reth and replaying PR #200's geth-derived fixture (bin/morph-statetest/tests/fixtures/fee_token_internal_calls.json, golden roots from morph-geth 5744b8f66) fails 4 cases on both Emerald and Jade, and every failure is a state root mismatch — not a gasUsed mismatch. The refund shifts gas_used → fee charged → balances. deduct_clear is your scenario 1; main_restores_cleared_slot is scenario 2.

The trigger is live on mainnet. Mainnet jade_fork_time = 1775628000 (2026-04-08), so post-Jade state-root validation is strictly enforced. Reading L2TokenRegistry (0x5300…0021) live, ids 2 and 6 have balanceSlot = 0 exactly as you described — and both are USDC (FiatTokenV2, 6 decimals, active). The fee transfer must consume the payer's entire token balance, which the caller controls via gas_limit / gas_price, so it is constructible. (Mechanism reproduced in the fixture, registry read live; we have not sent a live mainnet transaction, so the recipe is reasoned rather than demonstrated on-chain.)

One overlap to settle: PR #200 (fix/txpool-token-fee-subpool) already carries an equivalent fix under the name pre_fee_refund — same transfer_erc20_with_evm signature change, same record_refund placement before post_execution::refund, plus a unit test asserting (16_800, 4_200). Both patch the same handler.rs revision, so they will conflict and we should land one rather than both. Yours is the easier one to review at two files — happy to follow your preference on direction.

@kriss39

kriss39 commented Sep 14, 2026

Copy link
Copy Markdown
Author

Thanks for reproducing it and for taking it through the state-root fixture — I had only pinned the gasUsed / receipts-root side, so the four state root mismatch failures on Emerald and Jade are a stronger result than what's in the description. I've updated the summary to say it reaches the state root and to note the USDC / Jade-activation context.

On the overlap: my suggestion would be to land #207 first as the standalone consensus fix and rebase #200 on top of it, dropping the pre_fee_refund commit there. Reasoning: it's a two-file change with a self-contained test, so it can go in independently of the txpool refactor's review cycle, and it keeps the consensus fix bisectable on main. If it helps, I'm happy to (a) rename to pre_fee_refund so the rebase is a no-op for #200, and (b) port the deduct_clear / main_restores_cleared_slot fixture cases from #200 into this PR so the state-root regression is covered here too.

That said, if you'd rather keep everything in #200 since it's already carrying the geth-derived fixture, I'm fine closing this one — just say the word.

@panos-xyz

Copy link
Copy Markdown
Contributor

Thanks — this was a correct diagnosis, and it is why the call path ended up fixed properly rather than patched.

Your fix is fully absorbed by #210 (fix/revm-call-mode-fee-parity), the execution-layer slice extracted so the consensus change can ship independently of the txpool refactor. Every part of it is there: the refund counter carried on the EVM and recorded before the EIP-3529 cap (same placement, after the L1-message early return), transfer_erc20_with_evm returning the frame's net refund, and the zero-amount early return. The only difference is cosmetic — the field is pre_fee_refund there, pre_fee_gas_refund here.

I measured your fix on its own, against the geth golden fixtures, since that is the clearest way to state what it does. Applied to main by itself it takes the failures from 8 of 12 case templates down to 4, and the four it clears are exactly the state-root cases — deduct_clear, main_restores_cleared_slot, main_revert and main_oog. main_restores_cleared_slot is your scenario 2, and it confirms your reading precisely: the fee transfer() clears the payer's slot, the main frame re-sets it, and the +4800 and -4800 cancel on one counter only if the fee frame's refund survives. Scenario 1 is pinned by deduction_sstore_refund_reaches_transaction_gas, which asserts (gas, refund) == (16_800, 4_200).

One caveat so you know exactly what became of your tests: your two named unit tests were not transplanted verbatim — there is no 30_974 assertion in #210, and scenario 2 is pinned by the fixture instead. Say the word and we will port both forms in.

Why it landed as #210 rather than #207. The remaining four templates are three further divergences on the same path, none of which your report could have surfaced:

  • origin_guard, gasprice_guard — internal calls ran with a default transaction environment, so balanceOf/transfer saw ORIGIN 0x0 and an effective gas price of 0; a guard on either read the wrong value and the payer's balance resolved to zero, rejecting an affordable transaction.
  • refund_false_keeps_transfer — a successful transfer whose return value failed the later business check was rolled back with its logs, where go-ethereum keeps them.
  • zero_fee — a zero fee did not skip transfer(0).

All four were state-root, receipts or admission divergences on the same path. #210 is 8/12 → 0/12, i.e. 24/24 outcomes across Emerald and Jade.

Why it is timely. The plan is to move every mainnet fee token to the call path, which makes this path universal rather than two tokens (ids 2 and 6, both USDC, today). Unmodified v1.3.0 rejects blocks containing such transactions, so the fix has to reach nodes before the registry entries are flipped.

So we would like to close this as superseded — with thanks. Your report is what surfaced the rest of it.

@panos-xyz

Copy link
Copy Markdown
Contributor

Closing as superseded by #210, which absorbs this fix along with the rest of the call-path divergences found while investigating it. Thanks again for the report — it is what surfaced them.

@panos-xyz panos-xyz closed this Sep 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants