diff --git a/CHANGELOG.md b/CHANGELOG.md index 4064e1bbc..6fa9bc933 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,15 @@ the executor with its signature never checked, letting a victim-signed transaction be replayed as the victim. `VerifySender` is now required before `ApplyTransaction`, so `From` is always the ECDSA-recovered signer regardless of how the message arrived. +- Charge the parent Cosmos gas meter for failed EVM calls in `CallEVMWithData` and + `DerivedEVMCallWithData`. Both functions returned on `res.Failed()` before reaching + `ctx.GasMeter().ConsumeGas(res.GasUsed)`, so a reverting — or deliberately out-of-gas — + internal call performed real EVM work that cost the enclosing Cosmos transaction nothing + beyond the incidental KV-store gas. `DerivedEVMCallWithData` additionally clamps a + caller-supplied `gasLimit` to `config.DefaultGasCap`; on an out-of-gas halt `res.GasUsed` + equals the gas cap, so without the clamp the caller would pick how much gas the enclosing + transaction is forced to consume and could panic it with `OutOfGas`. `CallEVMWithData` + needs no clamp — its message gas limit is already the hardcoded `config.DefaultGasCap`. - [\#690](https://github.com/cosmos/evm/pull/690) Fix Ledger hardware wallet support for coin type 60. - [\#769](https://github.com/cosmos/evm/pull/769) Fix erc20 ibc middleware to not to validate sender address format. - [\#790](https://github.com/cosmos/evm/pull/790) fix panic in historical query due to missing EvmCoinInfo. diff --git a/tests/integration/x/vm/test_call_evm.go b/tests/integration/x/vm/test_call_evm.go index 013b200f2..787ef3dd3 100644 --- a/tests/integration/x/vm/test_call_evm.go +++ b/tests/integration/x/vm/test_call_evm.go @@ -10,6 +10,8 @@ import ( utiltx "github.com/cosmos/evm/testutil/tx" "github.com/cosmos/evm/x/erc20/types" evmtypes "github.com/cosmos/evm/x/vm/types" + + storetypes "cosmossdk.io/store/types" ) func (s *KeeperTestSuite) TestCallEVM() { @@ -148,3 +150,55 @@ func (s *KeeperTestSuite) TestCallEVMWithData() { }) } } + +// TestCallEVMWithDataFailedCallChargesParentGas is the CallEVMWithData half of +// F-2026-18824. Like DerivedEVMCallWithData, this function returned on +// res.Failed() before reaching ctx.GasMeter().ConsumeGas, so a reverting call +// performed real EVM work for free at the Cosmos meter. Unlike the derived path +// it needs no clamp: msg.GasLimit here is the hardcoded config.DefaultGasCap, so +// res.GasUsed is already bounded and the caller cannot inflate it. +func (s *KeeperTestSuite) TestCallEVMWithDataFailedCallChargesParentGas() { + s.SetupTest() + + from := s.Keyring.GetAddr(0) + ctx := s.Network.GetContext().WithGasMeter(storetypes.NewGasMeter(parentGasMeterLimit)) + keeper := s.Network.App.GetEVMKeeper() + + burner := s.deployRawCode(ctx, gasBurnerRevertCode) + gasBefore := ctx.GasMeter().GasConsumed() + + res, err := keeper.CallEVMWithData(ctx, from, &burner, nil, true, nil) + s.Require().Error(err, "the burner reverts, so the call must surface an error") + s.Require().NotNil(res) + s.Require().True(res.Failed()) + s.Require().Greater(res.GasUsed, uint64(2_000_000)) + + charged := ctx.GasMeter().GasConsumed() - gasBefore + s.Require().GreaterOrEqual(charged, res.GasUsed, + "a failed CallEVMWithData must charge its EVM gas to the parent Cosmos meter (F-2026-18824)") + s.Require().LessOrEqual(charged, res.GasUsed+1_000_000, + "res.GasUsed must be charged once, not doubled") +} + +// TestCallEVMWithDataSuccessChargesGasUsed pins the CallEVMWithData happy path: +// res.GasUsed is charged exactly once, unchanged by the failure-path fix. +func (s *KeeperTestSuite) TestCallEVMWithDataSuccessChargesGasUsed() { + s.SetupTest() + + from := s.Keyring.GetAddr(0) + ctx := s.Network.GetContext().WithGasMeter(storetypes.NewGasMeter(parentGasMeterLimit)) + keeper := s.Network.App.GetEVMKeeper() + + burner := s.deployRawCode(ctx, gasBurnerSuccessCode) + gasBefore := ctx.GasMeter().GasConsumed() + + res, err := keeper.CallEVMWithData(ctx, from, &burner, nil, true, nil) + s.Require().NoError(err) + s.Require().False(res.Failed()) + s.Require().Greater(res.GasUsed, uint64(2_000_000)) + + charged := ctx.GasMeter().GasConsumed() - gasBefore + s.Require().GreaterOrEqual(charged, res.GasUsed) + s.Require().LessOrEqual(charged, res.GasUsed+1_000_000, + "res.GasUsed must be charged exactly once on the success path") +} diff --git a/tests/integration/x/vm/test_derived_call.go b/tests/integration/x/vm/test_derived_call.go index b1d73f80a..5cacb679b 100644 --- a/tests/integration/x/vm/test_derived_call.go +++ b/tests/integration/x/vm/test_derived_call.go @@ -5,14 +5,21 @@ import ( "strconv" "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/crypto" + "github.com/holiman/uint256" abcitypes "github.com/cometbft/cometbft/abci/types" "github.com/cosmos/evm/contracts" + "github.com/cosmos/evm/server/config" testconstants "github.com/cosmos/evm/testutil/constants" "github.com/cosmos/evm/testutil/integration/evm/utils" + utiltx "github.com/cosmos/evm/testutil/tx" + "github.com/cosmos/evm/x/vm/statedb" evmtypes "github.com/cosmos/evm/x/vm/types" + storetypes "cosmossdk.io/store/types" + sdk "github.com/cosmos/cosmos-sdk/types" ) @@ -189,3 +196,184 @@ func (s *KeeperTestSuite) TestDerivedEVMCallWithDataRevertDoesNotMutateBloom() { s.Require().Equal(bloomBefore, keeper.GetBlockBloomTransient(ctx).Bytes(), "a reverted derived execution must not mutate the block bloom (F-2026-17738)") } + +// --------------------------------------------------------------------------- +// F-2026-18824: failed EVM calls must charge the parent Cosmos gas meter +// --------------------------------------------------------------------------- + +// gasBurnerRevertCode expands memory to 1 MiB and then REVERTs: +// +// PUSH1 0x00 // MSTORE value +// PUSH3 0x100000 // MSTORE offset -> forces a 1 MiB memory expansion +// MSTORE +// PUSH1 0x00 +// PUSH1 0x00 +// REVERT +// +// The memory expansion costs ~2.2M gas, deterministically, and REVERT hands the +// rest back — so res.GasUsed lands far above the incidental KV-store gas the +// call also draws, which is what makes it a usable signal in these tests. +var gasBurnerRevertCode = []byte{0x60, 0x00, 0x62, 0x10, 0x00, 0x00, 0x52, 0x60, 0x00, 0x60, 0x00, 0xfd} + +// gasBurnerSuccessCode is gasBurnerRevertCode with the trailing REVERT swapped +// for STOP: the same ~2.2M of memory expansion, but a successful execution. +var gasBurnerSuccessCode = []byte{0x60, 0x00, 0x62, 0x10, 0x00, 0x00, 0x52, 0x00} + +// invalidOpcodeCode is a single INVALID opcode. An exceptional halt burns every +// unit of gas handed to the frame, so res.GasUsed comes back equal to the gas +// cap — the same shape as running out of gas, without waiting for a loop to +// actually consume tens of millions of gas. +var invalidOpcodeCode = []byte{0xfe} + +// parentGasMeterLimit bounds the enclosing "Cosmos tx" in these tests. It sits +// above config.DefaultGasCap (25M) so a correctly clamped failure fits, and far +// below an unclamped caller-supplied limit so an unclamped charge panics. +const parentGasMeterLimit = 60_000_000 + +// deployRawCode installs raw runtime bytecode at a fresh address. +func (s *KeeperTestSuite) deployRawCode(ctx sdk.Context, code []byte) common.Address { + addr := utiltx.GenerateAddress() + codeHash := crypto.Keccak256Hash(code) + + k := s.Network.App.GetEVMKeeper() + k.SetCode(ctx, codeHash.Bytes(), code) + s.Require().NoError(k.SetAccount(ctx, addr, statedb.Account{ + Nonce: 1, + Balance: new(uint256.Int), + CodeHash: codeHash.Bytes(), + })) + return addr +} + +// TestDerivedEVMCallWithDataFailedCallChargesParentGas is the regression test for +// F-2026-18824. A reverting derived call used to return before the parent +// GasMeter().ConsumeGas, so the EVM work it had just performed was free at the +// Cosmos meter — only the incidental KV-store gas leaked through. The call now +// charges res.GasUsed on the failure path exactly as it does on success. +func (s *KeeperTestSuite) TestDerivedEVMCallWithDataFailedCallChargesParentGas() { + s.SetupTest() + + from := s.Keyring.GetAddr(0) + ctx := s.Network.GetContext().WithGasMeter(storetypes.NewGasMeter(parentGasMeterLimit)) + keeper := s.Network.App.GetEVMKeeper() + + burner := s.deployRawCode(ctx, gasBurnerRevertCode) + gasBefore := ctx.GasMeter().GasConsumed() + + res, err := keeper.DerivedEVMCallWithData( + ctx, from, &burner, nil, + true, // commit + false, // gasless + false, // isModuleSender + big.NewInt(0), big.NewInt(5_000_000), nil, + ) + s.Require().Error(err, "the burner reverts, so the call must surface an error") + s.Require().NotNil(res) + s.Require().True(res.Failed()) + s.Require().Greater(res.GasUsed, uint64(2_000_000), + "the burner must actually burn multi-million gas for this test to mean anything") + + charged := ctx.GasMeter().GasConsumed() - gasBefore + s.Require().GreaterOrEqual(charged, res.GasUsed, + "a failed derived call must charge its EVM gas to the parent Cosmos meter (F-2026-18824)") + s.Require().LessOrEqual(charged, res.GasUsed+1_000_000, + "res.GasUsed must be charged once, not doubled") +} + +// TestDerivedEVMCallWithDataGaslessFailureChargesParentGas covers the same +// failure path with gasless=true. The flag only zeroes the TxGasUsed event +// attribute; it never exempted the parent meter, and must not become an exemption +// now that failures are charged. +func (s *KeeperTestSuite) TestDerivedEVMCallWithDataGaslessFailureChargesParentGas() { + s.SetupTest() + + from := s.Keyring.GetAddr(0) + ctx := s.Network.GetContext().WithGasMeter(storetypes.NewGasMeter(parentGasMeterLimit)) + keeper := s.Network.App.GetEVMKeeper() + + burner := s.deployRawCode(ctx, gasBurnerRevertCode) + gasBefore := ctx.GasMeter().GasConsumed() + + res, err := keeper.DerivedEVMCallWithData( + ctx, from, &burner, nil, + true, // commit + true, // gasless + false, + big.NewInt(0), big.NewInt(5_000_000), nil, + ) + s.Require().Error(err) + s.Require().True(res.Failed()) + s.Require().GreaterOrEqual(ctx.GasMeter().GasConsumed()-gasBefore, res.GasUsed, + "gasless only zeroes the reported event attribute, not the parent meter") +} + +// TestDerivedEVMCallWithDataClampsCallerGasLimit is the safety half of +// F-2026-18824. Charging res.GasUsed on failure is only sound because the +// effective gas cap is clamped to config.DefaultGasCap: on an exceptional halt +// res.GasUsed equals the cap, so an unclamped caller-supplied gasLimit would let +// the caller choose how much gas the enclosing Cosmos tx is forced to consume and +// panic it with OutOfGas — which, on the MsgVoteInbound path, means a lost +// validator vote. Here the caller asks for 10x DefaultGasCap and the call halts +// on an INVALID opcode; the parent meter must absorb at most the clamped cap. +func (s *KeeperTestSuite) TestDerivedEVMCallWithDataClampsCallerGasLimit() { + s.SetupTest() + + from := s.Keyring.GetAddr(0) + ctx := s.Network.GetContext().WithGasMeter(storetypes.NewGasMeter(parentGasMeterLimit)) + keeper := s.Network.App.GetEVMKeeper() + + halter := s.deployRawCode(ctx, invalidOpcodeCode) + gasBefore := ctx.GasMeter().GasConsumed() + + //nolint:gosec // test-only constant, no overflow + callerGasLimit := big.NewInt(int64(config.DefaultGasCap) * 10) + + var res *evmtypes.MsgEthereumTxResponse + var err error + s.Require().NotPanics(func() { + res, err = keeper.DerivedEVMCallWithData( + ctx, from, &halter, nil, + true, false, false, + big.NewInt(0), callerGasLimit, nil, + ) + }, "a caller-supplied gasLimit above DefaultGasCap must not be able to panic the parent gas meter") + + s.Require().Error(err) + s.Require().True(res.Failed()) + s.Require().Equal(config.DefaultGasCap, res.GasUsed, + "an exceptional halt burns the whole cap, and the cap must be the clamped DefaultGasCap") + + charged := ctx.GasMeter().GasConsumed() - gasBefore + s.Require().GreaterOrEqual(charged, config.DefaultGasCap, + "the clamped cap is still charged to the parent meter") + s.Require().LessOrEqual(charged, config.DefaultGasCap+1_000_000, + "the parent meter must never be charged beyond the clamped cap") +} + +// TestDerivedEVMCallWithDataSuccessChargesGasUsed pins the happy path: a +// successful derived call charges res.GasUsed once, unchanged by the failure-path +// fix. +func (s *KeeperTestSuite) TestDerivedEVMCallWithDataSuccessChargesGasUsed() { + s.SetupTest() + + from := s.Keyring.GetAddr(0) + ctx := s.Network.GetContext().WithGasMeter(storetypes.NewGasMeter(parentGasMeterLimit)) + keeper := s.Network.App.GetEVMKeeper() + + burner := s.deployRawCode(ctx, gasBurnerSuccessCode) + gasBefore := ctx.GasMeter().GasConsumed() + + res, err := keeper.DerivedEVMCallWithData( + ctx, from, &burner, nil, + true, false, false, + big.NewInt(0), big.NewInt(5_000_000), nil, + ) + s.Require().NoError(err) + s.Require().False(res.Failed()) + s.Require().Greater(res.GasUsed, uint64(2_000_000)) + + charged := ctx.GasMeter().GasConsumed() - gasBefore + s.Require().GreaterOrEqual(charged, res.GasUsed) + s.Require().LessOrEqual(charged, res.GasUsed+1_000_000, + "res.GasUsed must be charged exactly once on the success path") +} diff --git a/x/vm/keeper/call_evm.go b/x/vm/keeper/call_evm.go index 3d8668bff..de9507c6b 100644 --- a/x/vm/keeper/call_evm.go +++ b/x/vm/keeper/call_evm.go @@ -72,10 +72,9 @@ func (k Keeper) CallEVMWithData( AccessList: ethtypes.AccessList{}, } - // Use a cache context so that a reverting EVM call does not corrupt the - // parent gas meter. On success we commit the cache and charge the actual - // gas used; on revert we discard the cache and leave the parent meter - // untouched (matching DerivedEVMCallWithData semantics). + // Use a cache context so that a reverting EVM call does not commit its state + // changes. The cache shares the parent gas meter, so gas is metered against + // the caller either way; only the store writes are discarded on failure. tmpCtx, commitState := ctx.CacheContext() res, err := k.ApplyMessage(tmpCtx, msg, nil, commit, true) if err != nil { @@ -83,6 +82,13 @@ func (k Keeper) CallEVMWithData( } if res.Failed() { + // A failed execution still burned real EVM work, so charge it to the parent + // Cosmos gas meter exactly like a successful one. Skipping this made a + // revert (or a deliberate out-of-gas) a free way to consume block compute. + // msg.GasLimit above is the hardcoded config.DefaultGasCap, so res.GasUsed + // — which equals the gas limit on out-of-gas — is bounded by that cap and + // cannot be inflated by the caller. + ctx.GasMeter().ConsumeGas(res.GasUsed, "apply evm message (failed)") return res, errorsmod.Wrap(types.ErrVMExecution, res.VmError) } @@ -191,7 +197,15 @@ func (k Keeper) DerivedEVMCallWithData( gasCap = gasRes.Gas } if gasLimit != nil { - gasCap = gasLimit.Uint64() + // Clamp the caller-supplied limit to DefaultGasCap. The failure path below + // charges res.GasUsed to the parent Cosmos gas meter, and on out-of-gas + // res.GasUsed equals this cap — so an unclamped caller value would let the + // caller choose how much gas the enclosing Cosmos tx is forced to consume, + // and panic it with OutOfGas. DefaultGasCap is already the ceiling on every + // other path into this function: the estimator above runs with + // GasCap: config.DefaultGasCap, and the remaining branch uses the cap + // directly. The clamp therefore only ever narrows an outlier. + gasCap = min(gasLimit.Uint64(), config.DefaultGasCap) } msg := core.Message{ @@ -323,6 +337,12 @@ func (k Keeper) DerivedEVMCallWithData( } if res.Failed() { + // A failed execution still burned real EVM work, so charge it to the parent + // Cosmos gas meter exactly like a successful one. Skipping this made a + // revert (or a deliberate out-of-gas) a free way to consume block compute, + // and left the `gasless` flag zeroing only the reported event attribute. + // res.GasUsed is bounded by gasCap, clamped to config.DefaultGasCap above. + ctx.GasMeter().ConsumeGas(res.GasUsed, "apply evm message (failed)") return res, errorsmod.Wrapf(types.ErrVMExecution, "%s: ret 0x%x", res.VmError, res.Ret) }