fix: evm fees refund#17
Conversation
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/x/vm/test_gas.go`:
- Line 260: The RefundGas method call in the integration test is passing an
extra gasUsed argument that is no longer part of the method signature. Remove
the gasUsed parameter from the RefundGas call on the
unitNetwork.App.GetEVMKeeper().RefundGas line, keeping only the ctx, *coreMsg,
leftoverGas, and baseDenom arguments to match the actual method signature in
x/vm/keeper/gas.go.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9732a2d5-f9f4-4fd5-a403-54af34017e2e
📒 Files selected for processing (2)
tests/integration/x/vm/test_gas.gox/vm/keeper/gas.go
mattkii
force-pushed
the
fix/evm-fee-refunds
branch
from
15471c7 to
983ade3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
x/vm/keeper/gas.go (1)
69-73: ⚡ Quick winGuard the denominator computation against overflow/zero before division.
gasUsed + leftoverGasis summed asuint64and fed tobig.Int.Div. If that sum overflows to zero, this path can panic on division-by-zero. Add an explicit checked add before constructinggasLimit.Proposed hardening
+ totalGas, carry := bits.Add64(gasUsed, leftoverGas, 0) + if carry != 0 || totalGas == 0 { + return errorsmod.Wrap(types.ErrInvalidRefund, "invalid gas inputs for refund calculation") + } - gasLimit := new(big.Int).SetUint64(gasUsed + leftoverGas) + gasLimit := new(big.Int).SetUint64(totalGas) remaining = new(big.Int).Div( new(big.Int).Mul(amount, new(big.Int).SetUint64(leftoverGas)), gasLimit, )import "math/bits"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@x/vm/keeper/gas.go` around lines 69 - 73, The denominator computation in the division operation contains a potential overflow vulnerability where gasUsed + leftoverGas could overflow as a uint64 and wrap to zero, causing a division-by-zero panic. Import the math/bits package and replace the direct addition of gasUsed and leftoverGas with a checked add operation using math/bits.Add64 to detect overflow before constructing the gasLimit variable. Handle the overflow case appropriately (either return early with an error or use a safe fallback) to ensure the denominator in the Div call is never zero.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@x/vm/keeper/gas.go`:
- Around line 69-73: The denominator computation in the division operation
contains a potential overflow vulnerability where gasUsed + leftoverGas could
overflow as a uint64 and wrap to zero, causing a division-by-zero panic. Import
the math/bits package and replace the direct addition of gasUsed and leftoverGas
with a checked add operation using math/bits.Add64 to detect overflow before
constructing the gasLimit variable. Handle the overflow case appropriately
(either return early with an error or use a safe fallback) to ensure the
denominator in the Div call is never zero.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 070cb690-8956-40e1-99b7-6abc45ad0f08
📒 Files selected for processing (4)
tests/integration/x/vm/test_gas.gotests/integration/x/vm/test_state_transition.gox/vm/keeper/gas.gox/vm/keeper/state_transition.go
✅ Files skipped from review due to trivial changes (1)
- x/vm/keeper/state_transition.go
mattkii
force-pushed
the
fix/evm-fee-refunds
branch
2 times, most recently
from
67daf8b to
98a6942
Compare
mattkii
force-pushed
the
fix/evm-fee-refunds
branch
3 times, most recently
from
4310666 to
e203e4a
Compare
mattkii
force-pushed
the
fix/evm-fee-refunds
branch
from
e203e4a to
9029908
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@x/vm/keeper/gas.go`:
- Around line 69-71: The early return condition at line 42 that checks `if
gasUsed == 0 { return nil }` prevents the refund calculation from executing when
no gas is used, which blocks valid full refunds. Since the denominator now uses
`msg.GasLimit` (line 71), when `gasUsed == 0`, the calculation would correctly
compute the full refund as the remaining amount equals the original amount.
Remove or modify this early return condition to allow the refund calculation to
proceed and handle the full-refund case properly, such as the TestGasRefundGas
test case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 12ac2967-ba5a-4c0c-916a-ee737a21c3d0
📒 Files selected for processing (3)
CHANGELOG.mdtests/integration/x/vm/test_gas.gox/vm/keeper/gas.go
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/x/vm/test_gas.go
2 participants
Description
Fixes an over-refund bug in the fee-abstraction EVM gas refund path that allowed permissionless draining of the
fee_collectormodule account.RefundGas(x/vm/keeper/gas.go) computes the fee-abstraction refund asamount * leftoverGas / denominator, whereamountis the full upfront fee charged for the whole gas limit (recorded inContextPaidFeesKeyby the chain's EVM ante). The denominator was the gas actually consumed (gasUsed) instead of the gas limit. WhenevergasLimit > 2 × gasUsed, the refund exceeded the entire upfront fee, and the surplus was paid out offee_collector— the validator/staker reward pool. Repeating cheap, high-gas-limit transactions drains it.Fix: use the transaction gas limit as the denominator. Since the caller in
state_transition.gopassesleftoverGas = gasLimit - gasUsed, this is exactlygasUsed + leftoverGas, sorefund = amount * leftoverGas / gasLimit, which equals the value of the unused gas and is provably bounded by the amount paid (leftoverGas ≤ gasLimit).Files most critical to review
x/vm/keeper/gas.go— the one-line denominator fix (gasUsed→msg.GasLimit).tests/integration/x/vm/test_gas.go— new regression testTestGasRefundGasNoOverRefund.How to review / reproduce
Run the refund suite (requires the
testbuild tag, executed from theevmdmodule):The new
TestGasRefundGasNoOverRefundreproduces the attack at the keeper level (gasLimit=100,000, gasUsed=21,000, paid fee recorded inContextPaidFeesKey) and asserts the refund equals only the unused-gas value and never exceeds the paid fee.Verification results:
gasUsed): refund =45,142,857,142vs. the correct9,480,000,000—fee_collectorover-drained. Test FAILS.msg.GasLimit): refund =9,480,000,000, bounded by the paid fee. Test PASSES, along with all pre-existingTestGasRefundGascases.End-to-end verification was also performed through the consuming chain's production EVM ante (
NewEVMMonoDecorator→fee_collector→RefundGas) using the issue's PoC config (gasLimit=100,000, gasPrice=1e6, paidFee=1e11):376,190,476,190;fee_collectornet drain−276,190,476,190(matches the reported PoC exactly).79,000,000,000;fee_collectorretains the21,000,000,000fee for gas actually used; attacker net profit = 0.Author Checklist
All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.
I have...
mainbranch — N/A: this targets the activev0.6.xmaintenance branch (fix/distribution-32byte-withdraw), which is the lineage the consuming chain pins viaKiiChain/evm v0.6.0-fork.1.mainis an older divergent branch that already uses the gas-limit denominator and is not vulnerable. A new fork tag should be cut from this branch and the consuming chain'sgo.modbumped to it.