fix: F-2026-18824 | [Dual Defense] Failed DerivedEVMCallWithData / CallEVMWithData Skip Cosmos ConsumeGas - #43
Open
0xNilesh wants to merge 1 commit into
Open
fix: F-2026-18824 | [Dual Defense] Failed DerivedEVMCallWithData / CallEVMWithData Skip Cosmos ConsumeGas#430xNilesh wants to merge 1 commit into
0xNilesh wants to merge 1 commit into
Conversation
CallEVMWithData and DerivedEVMCallWithData returned on res.Failed() before reaching ctx.GasMeter().ConsumeGas(res.GasUsed), so a reverting or deliberately out-of-gas internal call was free at the Cosmos meter. DerivedEVMCallWithData also clamps a caller-supplied gasLimit to config.DefaultGasCap: on an out-of-gas halt res.GasUsed equals the cap, so without the clamp the caller would choose how much gas the enclosing Cosmos tx is forced to consume and could panic it with OutOfGas.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The defect
x/vm/keeper/call_evm.gocharged the parent Cosmos gas meter only on success. Both internal-call entry points returned onres.Failed()before reachingctx.GasMeter().ConsumeGas(res.GasUsed):CallEVMWithData—if res.Failed() { return ... }sat aboveConsumeGas(res.GasUsed)DerivedEVMCallWithData— same shapeSo EVM work that reverted, or that deliberately ran itself out of gas, cost the enclosing Cosmos transaction nothing beyond the incidental KV-store gas the call happened to draw through the shared meter. A live PoC on an earlier build ran a ~52M-gas burner that looped and then reverted; the enclosing tx was charged 26,021 gas.
The
gaslessflag is unrelated to this: it only zeroes theTxGasUsedevent attribute, never the meter. Successful gasless derived calls already consumed parent gas — the asymmetry was failure-only.Why the clamp is a prerequisite, not a bonus
On an out-of-gas / exceptional halt,
res.GasUsedequals the gas cap. The two functions were bounded very differently:CallEVMWithDatabuilds its message withGasLimit: config.DefaultGasCap— hardcoded.res.GasUsedis already bounded, so charging on failure is safe as-is and needs no clamp here.DerivedEVMCallWithDatadidgasCap := config.DefaultGasCap; if gasLimit != nil { gasCap = gasLimit.Uint64() }— a caller-suppliedgasLimitoverrode the default with no clamp.Charging
res.GasUsedon the derived failure path without clamping would therefore have been a worse bug than the one being fixed: a caller setsUniversalPayload.GasLimitarbitrarily high, forces an out-of-gas halt, andConsumeGas(huge)panicsOutOfGason the parent meter — aborting the whole Cosmos tx, not just the EVM call (CacheContext()branches the multistore but shares the gas meter, so the panic is not contained). That path runs insideMsgVoteInbound, so an abort there is a lost validator vote. This is exactly why an earlier change (a05ad85a) removed the failure-path charge rather than fixing it.So: clamp first, then charge.
What changed
x/vm/keeper/call_evm.go:DerivedEVMCallWithData— the caller-supplied gas limit is clamped:gasCap = min(gasLimit.Uint64(), config.DefaultGasCap).DerivedEVMCallWithData— theres.Failed()branch now callsctx.GasMeter().ConsumeGas(res.GasUsed, "apply evm message (failed)")before returning the error.CallEVMWithData— same failure-pathConsumeGas. No clamp needed (see above); itsgasCapparameter stays ignored, which is tracked separately as F-2026-18818.CallEVMWithData's cache context ("leave the parent meter untouched") is corrected — the cache exists to discard state, and shares the gas meter either way.res.GasUsedis charged — notGasMeter().Limit(), which is what upstream'sResetGasMeterAndConsumeGas(ctx, ctx.GasMeter().Limit())did and which would burn the caller's entire remaining budget on any revert.The clamp does not narrow any real caller
Every other path into
DerivedEVMCallWithDatawas already ceilinged atDefaultGasCap(25M): thegasLimit == nilbranch uses the constant directly, and the estimating branch runsEstimateGasInternalwithGasCap: config.DefaultGasCap. On the chain side, exactly one caller passes a non-nilgasLimit—CallUEAExecutePayload, forwarding the attacker-controlledUniversalPayload.GasLimit. Every otherDerivedEVMCallinx/uexecutorandx/ucallbackpassesnil. The clamp therefore only ever narrows an outlier.Tests
tests/integration/x/vm/test_derived_call.goandtest_call_evm.go(run viaevmd/tests/integration,TestKeeperTestSuite). They install raw runtime bytecode at a fresh address: a ~2.2M-gas memory-expansion burner ending inREVERT, the same burner ending inSTOP, and a bareINVALIDopcode (an exceptional halt burns the entire frame, sores.GasUsed == gasCap— an out-of-gas shape without waiting on a 25M-gas loop). The parent meter is a real boundedstoretypes.NewGasMeter(60_000_000).TestDerivedEVMCallWithDataFailedCallChargesParentGas— a reverting derived call chargesres.GasUsed(>2M) to the parent meter, once, not twice.TestDerivedEVMCallWithDataGaslessFailureChargesParentGas—gasless: truedoes not exempt the meter.TestDerivedEVMCallWithDataClampsCallerGasLimit— caller asks for10 × DefaultGasCapand halts; asserts no panic,res.GasUsed == DefaultGasCap, and the parent meter absorbs at most the clamped cap.TestDerivedEVMCallWithDataSuccessChargesGasUsed— happy path unchanged:res.GasUsedcharged exactly once.TestCallEVMWithDataFailedCallChargesParentGas/TestCallEVMWithDataSuccessChargesGasUsed— the same failure/success coverage forCallEVMWithData.Regression-detector check — each half of the fix was reverted in turn and the suite re-run:
ConsumeGasCallEVMWithDatatests still pass...ClampsCallerGasLimitFAILS withshould not panic— reproducing exactly theOutOfGasabort the clamp preventsCallEVMWithDatafailure-pathConsumeGasTestCallEVMWithDataFailedCallChargesParentGasFAILS aloneThe fix was restored and the suite is green.
./x/vm/...passes. The one red test in the widerTestKeeperTestSuite,TestRefundGas/Case_invalid_GasPrice_in_message, fails identically onaudit-fixeswithout this change.Related, tracked separately
UniversalPayload.GasLimit. Worth landing: after this PR a failing derived call can still charge up toDefaultGasCap(25M) to the enclosing tx, which is correct metering but is a large bill for aMsgVoteInboundwhose own gas limit may be lower. A sane per-message budget on the chain side bounds that properly.CallEVMWithDataignores itsgasCapparameter (x/ibc/callbackspassesremainingGasand it is silently dropped). Not touched here: honouring it unclamped would reintroduce the same unbounded-charge problem this PR exists to avoid, so it needs its ownmin(gasCap, DefaultGasCap)treatment.