diff --git a/CHANGELOG.md b/CHANGELOG.md index f1088f08c..ed4ae895b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,12 @@ Follow the [migration document](docs/migrations/v0.5.x_to_v0.6.0.md) for upgrade ### BUG FIXES +- Re-apply the `validateApprovalEventDoesNotExist` guard (F-2026-18822) on the ERC20 -> coin + direction. v0.6.0 lifts that body out of `x/erc20/keeper/msg_server.go` into the new shared + `x/erc20/keeper/convert.go` (`ConvertERC20IntoCoinsForNativeToken`, also reached from the ICS20 + precompile) and drops the call on the way, while still promising it in the doc comment. The guard + is restored in the position it held before the move, and the `delayed malicious contract` case in + `TestConvertERC20IntoCoinsForNativeToken` expects a rejection again. - Report the block base fee (instead of `0`) as the `gasPrice` of derived EVM transactions in `eth_getTransactionByHash` / `eth_getBlockByNumber`, and as their receipt `effectiveGasPrice`. Derived txs carry zero fee caps, so consumers that model burn as `base_fee * gas_used` — such as @@ -60,6 +66,71 @@ Follow the [migration document](docs/migrations/v0.5.x_to_v0.6.0.md) for upgrade ### BUG FIXES +- Honour the `gasCap` argument in `CallEVMWithData`. The parameter was accepted and then ignored - + `GasLimit` was hardcoded to `config.DefaultGasCap` - so callers passing a cap to sandbox EVM work + (the IBC callback keeper passes its remaining Cosmos gas) got a message that could burn up to 25M + gas regardless. `DefaultGasCap` remains the ceiling, so a caller can only narrow the limit. This + matches `DerivedEVMCallWithData` and the shape upstream cosmos/evm settled on. Also routes the + timeout callback's EVM call through `cachedCtx`, as the acknowledgement path already does. +- Resolve the ABI method at the top of every stateful precompile's `Run`, before `RunNativeAction` + is entered. The native-action preamble takes a cache context, snapshots the multi-store and + commits the state DB cache - replaying the caller's entire dirty account and storage set - and + only then does `SetupABI` get around to resolving the selector and failing. `RequiredGas` returns + 0 for an unresolvable selector, so none of that work was charged: a contract could dirty a large + storage set and then hand `bank`, `gov`, `staking`, `distribution`, `ics20`, `slashing`, `erc20` + or `werc20` four random bytes to have the whole set replayed for free, up to 20 times per + transaction. The check goes through the new `cmn.ResolveMethod`, which `SetupABI` now also uses, + so the fallback/receive special cases WERC20 depends on keep working and the revert is unchanged. +- Make `ConvertCoinNativeERC20` atomic. It escrowed the sender's coins onto the erc20 module + account before calling the EVM, and `CallEVMWithData` caches only the EVM execution, so a VM + failure discarded the EVM cache while leaving the bank escrow committed. On the IBC error-ACK / + timeout refund path, which deliberately swallows a conversion error so a failed re-wrap cannot + undo the refund, that stranded the sender's coins on the module account permanently - contradicting + the `OnAcknowledgementPacket` / `OnTimeoutPacket` comments promising the user keeps the bank token. + The escrow, the EVM transfer and the burn now all run on a branched context and commit together. +- Run the gov precompile's `getTallyResult` against a throwaway cache. The SDK's `TallyResult` query + delegates to `Keeper.Tally`, which deletes every vote it counts; that is safe on the SDK's own + query paths because a gRPC query runs against a context that is never committed, but precompiles + commit their state DB cache context unconditionally. A plain EVM call to `getTallyResult` on a + proposal in the voting period therefore deleted that proposal's votes permanently. The query's + store writes are now discarded, leaving the tally itself unchanged. +- Verify the Ethereum sender in `Keeper.EthereumTx` before applying the transaction. The EVM ante + handler only runs over a tx's top-level messages, so an `MsgEthereumTx` nested inside a + dispatching module (`x/authz`, `x/group`, `x/gov`, a CosmWasm stargate/`Any` message, ICA) reached + 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. +- Clamp the fee market's cumulative gas wanted instead of failing the block. `EndBlock` converted + both the block's cumulative `gasWanted` and its `gasUsed` to `int64` and returned an error when + either did not fit. `gasWanted` is an unchecked uint64 sum of per-transaction declarations, and + under `max_gas: -1` nothing bounds a single declaration, so two transactions declaring `MaxInt64` + sum into `(MaxInt64, MaxUint64]` with neither being individually rejectable. An error out of + `EndBlock` surfaces through `FinalizeBlock` after the block has been decided, leaving CometBFT at + height H and the application at H-1. Both values are now clamped to `MaxInt64` and logged. + `AddTransientGasWanted` also saturates instead of wrapping, so the total can never silently roll + over to a small number. +- Reject a `x/erc20` conversion whose token `transfer` emits an unexpected `Approval` event. Both + native-ERC20 convert directions escrow the token and mint (or burn) the paired coins, checking + only that the transfer succeeded and that the balance moved by the right amount. A registered + token that also grants a third party an allowance over the recipient satisfies both checks: the + escrow only drains later, when that allowance is spent, leaving the minted coins unbacked. The + `validateApprovalEventDoesNotExist` guard, already documented by both convert doc-comments but + never called, now runs on both paths. +- 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`. +- Reject a `gasLimit` that does not fit in a `uint64` in `DerivedEVMCallWithData` instead of + truncating it. `big.Int.Uint64` is undefined for values that do not fit and in practice returns + the low 64 bits, and node-side validation bounds the universal-payload gas limit to `uint256` — + so a payload declaring `2^64 + 5` was silently handed **5** gas and failed with a misleading + out-of-gas error rather than a clear rejection. The call now returns `ErrInvalidGasLimit` naming + the problem; `math.MaxUint64` still fits and is clamped to `config.DefaultGasCap` as before. - [\#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/precompiles/bank/bank.go b/precompiles/bank/bank.go index 376eb5357..82781cac6 100644 --- a/precompiles/bank/bank.go +++ b/precompiles/bank/bank.go @@ -107,6 +107,15 @@ func (p Precompile) RequiredGas(input []byte) uint64 { } func (p Precompile) Run(evm *vm.EVM, contract *vm.Contract, readonly bool) ([]byte, error) { + // Resolve the target method before RunNativeAction touches any state: the + // native-action preamble (cache context, multi-store snapshot, state DB + // commit) replays the caller's entire dirty set and is not paid for when the + // selector turns out to be unresolvable, because RequiredGas returns 0 for + // one. Resolving here keeps the revert identical while skipping that work. + if _, err := cmn.ResolveMethod(p.ABI, contract); err != nil { + return cmn.ReturnRevertError(evm, err) + } + return p.RunNativeAction(evm, contract, func(ctx sdk.Context) ([]byte, error) { return p.Execute(ctx, contract, readonly) }) diff --git a/precompiles/bank/bank_selector_test.go b/precompiles/bank/bank_selector_test.go new file mode 100644 index 000000000..95d8a21b0 --- /dev/null +++ b/precompiles/bank/bank_selector_test.go @@ -0,0 +1,92 @@ +package bank_test + +import ( + "math/big" + "testing" + + "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/core/vm" + "github.com/ethereum/go-ethereum/params" + "github.com/holiman/uint256" + "github.com/stretchr/testify/require" + + "github.com/cosmos/evm/precompiles/bank" + cmn "github.com/cosmos/evm/precompiles/common" +) + +// runPrecompile drives bank.Precompile.Run with a nil StateDB and returns the +// ABI-encoded revert reason. +// +// The early selector guard never looks at evm.StateDB, whereas the first thing +// RunNativeAction does is type-assert on it. A revert reason carrying +// cmn.ErrNotRunInEvm therefore proves the call reached the native-action +// preamble, and any other reason proves it was rejected before it - which is +// what lets these tests tell the two apart without standing up a full app. +func runPrecompile(t *testing.T, input []byte, value *uint256.Int) string { + t.Helper() + + evm := vm.NewEVM(vm.BlockContext{BlockNumber: big.NewInt(1)}, nil, params.TestChainConfig, vm.Config{}) + contract := vm.NewContract(common.Address{}, common.Address{}, value, 100_000, nil) + contract.Input = input + + bz, err := bank.NewPrecompile(nil, nil).Run(evm, contract, false) + require.ErrorIs(t, err, vm.ErrExecutionReverted, "precompile errors are always surfaced as a revert") + + return string(bz) +} + +// TestRunResolvesSelectorBeforeNativeAction covers F-2026-18786: calldata that +// cannot be dispatched used to run the whole native-action preamble - cache +// context, multi-store snapshot and a state DB commit that replays the caller's +// entire dirty set - before SetupABI got around to failing on the selector, and +// RequiredGas returns 0 for an unresolvable selector so none of it was charged. +func TestRunResolvesSelectorBeforeNativeAction(t *testing.T) { + testCases := []struct { + name string + // input is the calldata handed to the precompile. + input []byte + // reachesNativeAction is true when the calldata is dispatchable and so + // must still be handed to RunNativeAction. + reachesNativeAction bool + // expReason, when set, must appear in the revert reason. + expReason string + }{ + { + name: "unknown selector is rejected before the native action", + input: []byte{0xde, 0xad, 0xbe, 0xef}, + expReason: "no method with id", + }, + { + name: "calldata shorter than a selector is rejected before the native action", + input: []byte{0x01, 0x02, 0x03}, + }, + { + name: "empty calldata is rejected before the native action", + input: nil, + }, + { + name: "a valid selector is still dispatched", + input: bank.ABI.Methods[bank.BalancesMethod].ID, + reachesNativeAction: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + reason := runPrecompile(t, tc.input, uint256.NewInt(0)) + + if tc.reachesNativeAction { + require.Contains(t, reason, cmn.ErrNotRunInEvm, + "a valid selector must still reach RunNativeAction") + return + } + + require.NotContains(t, reason, cmn.ErrNotRunInEvm, + "undispatchable calldata must not reach the native-action preamble (F-2026-18786)") + + if tc.expReason != "" { + require.Contains(t, reason, tc.expReason) + } + }) + } +} diff --git a/precompiles/common/balance_handler_test.go b/precompiles/common/balance_handler_test.go index 83ac6e6b2..a38714f92 100644 --- a/precompiles/common/balance_handler_test.go +++ b/precompiles/common/balance_handler_test.go @@ -231,6 +231,51 @@ func TestAfterBalanceChange(t *testing.T) { require.Equal(t, "3", stateDB.GetBalance(receiver).String()) } +// TestAfterBalanceChangeSpendMoreThanBalancePanics reproduces the F-2026-18201 +// attack path at the precompile boundary: the EVM state view only tracks +// SPENDABLE balance, but a vesting account can delegate LOCKED coins through the +// staking precompile. That emits a coin_spent event for more than the StateDB +// holds. Before the underflow guard the subtraction wrapped to ~2^256, and +// x/vm/keeper/statedb.go reconciled the bogus view back to bank by MINTING the +// difference. It must now hard-fail instead. +func TestAfterBalanceChangeSpendMoreThanBalancePanics(t *testing.T) { + setupBalanceHandlerTest(t) + + storeKey := storetypes.NewKVStoreKey("test") + tKey := storetypes.NewTransientStoreKey("test_t") + ctx := sdktestutil.DefaultContext(storeKey, tKey) + + stateDB := statedb.New(ctx, mocks.NewEVMKeeper(), statedb.NewEmptyTxConfig()) + + _, addrs, err := testutil.GeneratePrivKeyAddressPairs(1) + require.NoError(t, err) + spenderAcc := addrs[0] + spender := common.BytesToAddress(spenderAcc) + + // Spendable balance as seen by the EVM. + stateDB.AddBalance(spender, uint256.NewInt(5), tracing.BalanceChangeUnspecified) + + bankKeeper := cmnmocks.NewBankKeeper(t) + bankKeeper.Mock.On("BlockedAddr", mock.AnythingOfType("types.AccAddress")).Return(false) + + bh := cmn.NewBalanceHandlerFactory(bankKeeper).NewBalanceHandler() + bh.BeforeBalanceChange(ctx) + + // Delegating locked coins spends more than the spendable balance. + ctx.EventManager().EmitEvents(sdk.Events{ + banktypes.NewCoinSpentEvent(spenderAcc, sdk.NewCoins(sdk.NewInt64Coin(evmtypes.GetEVMCoinDenom(), 10))), + }) + + require.PanicsWithValue( + t, + "state balance underflow for "+spender.Hex()+": have=5 sub=10", + func() { _ = bh.AfterBalanceChange(ctx, stateDB) }, + ) + + // The balance must not have wrapped around to ~2^256. + require.Equal(t, "5", stateDB.GetBalance(spender).String()) +} + func TestAfterBalanceChangeErrors(t *testing.T) { setupBalanceHandlerTest(t) diff --git a/precompiles/common/precompile.go b/precompiles/common/precompile.go index 488ef9379..7daf845b8 100644 --- a/precompiles/common/precompile.go +++ b/precompiles/common/precompile.go @@ -127,14 +127,19 @@ func (p Precompile) runNativeAction(evm *vm.EVM, contract *vm.Contract, action N return bz, nil } -// SetupABI runs the initial setup required to run a transaction or a query. -// It returns the ABI method, initial gas and calling arguments. -func SetupABI( - api abi.ABI, - contract *vm.Contract, - readOnly bool, - isTransaction func(name *abi.Method) bool, -) (method *abi.Method, args []interface{}, err error) { +// ResolveMethod resolves the ABI method targeted by the contract calldata. +// +// It is the selector-resolution half of SetupABI, exported so that a concrete +// precompile can reject calldata it cannot dispatch at the very top of its Run, +// *before* RunNativeAction sets up the native context, snapshots the multi-store +// and commits the state DB cache. That preamble replays the caller's whole dirty +// set and is not charged for an unresolvable selector, since RequiredGas also +// returns 0 for one. +// +// SetupABI delegates to this function so the early check and the in-action check +// can never disagree - notably on the fallback/receive special cases, which the +// werc20 precompile relies on for empty, short and unknown calldata. +func ResolveMethod(api abi.ABI, contract *vm.Contract) (method *abi.Method, err error) { // NOTE: This is a special case where the calling transaction does not specify a function name. // In this case we default to a `fallback` or `receive` function on the contract. @@ -146,17 +151,29 @@ func SetupABI( switch { // Case 1: Calldata is empty case isEmptyCallData: - method, err = emptyCallData(api, contract) + return emptyCallData(api, contract) // Case 2: calldata is non-empty but less than 4 bytes needed for a method case isShortCallData: - method, err = methodIDCallData(api) + return methodIDCallData(api) // Case 3: calldata is non-empty and contains the minimum 4 bytes needed for a method case isStandardCallData: - method, err = standardCallData(api, contract) + return standardCallData(api, contract) } + return nil, vm.ErrExecutionReverted +} + +// SetupABI runs the initial setup required to run a transaction or a query. +// It returns the ABI method, initial gas and calling arguments. +func SetupABI( + api abi.ABI, + contract *vm.Contract, + readOnly bool, + isTransaction func(name *abi.Method) bool, +) (method *abi.Method, args []interface{}, err error) { + method, err = ResolveMethod(api, contract) if err != nil { return nil, nil, err } diff --git a/precompiles/distribution/distribution.go b/precompiles/distribution/distribution.go index f363fa7c9..d6c0faf17 100644 --- a/precompiles/distribution/distribution.go +++ b/precompiles/distribution/distribution.go @@ -95,6 +95,15 @@ func (p Precompile) RequiredGas(input []byte) uint64 { } func (p Precompile) Run(evm *vm.EVM, contract *vm.Contract, readonly bool) ([]byte, error) { + // Resolve the target method before RunNativeAction touches any state: the + // native-action preamble (cache context, multi-store snapshot, state DB + // commit) replays the caller's entire dirty set and is not paid for when the + // selector turns out to be unresolvable, because RequiredGas returns 0 for + // one. Resolving here keeps the revert identical while skipping that work. + if _, err := cmn.ResolveMethod(p.ABI, contract); err != nil { + return cmn.ReturnRevertError(evm, err) + } + return p.RunNativeAction(evm, contract, func(ctx sdk.Context) ([]byte, error) { return p.Execute(ctx, evm.StateDB, contract, readonly) }) diff --git a/precompiles/distribution/distribution_selector_test.go b/precompiles/distribution/distribution_selector_test.go new file mode 100644 index 000000000..9383c75ea --- /dev/null +++ b/precompiles/distribution/distribution_selector_test.go @@ -0,0 +1,92 @@ +package distribution_test + +import ( + "math/big" + "testing" + + "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/core/vm" + "github.com/ethereum/go-ethereum/params" + "github.com/holiman/uint256" + "github.com/stretchr/testify/require" + + cmn "github.com/cosmos/evm/precompiles/common" + "github.com/cosmos/evm/precompiles/distribution" +) + +// runPrecompile drives distribution.Precompile.Run with a nil StateDB and returns the +// ABI-encoded revert reason. +// +// The early selector guard never looks at evm.StateDB, whereas the first thing +// RunNativeAction does is type-assert on it. A revert reason carrying +// cmn.ErrNotRunInEvm therefore proves the call reached the native-action +// preamble, and any other reason proves it was rejected before it - which is +// what lets these tests tell the two apart without standing up a full app. +func runPrecompile(t *testing.T, input []byte, value *uint256.Int) string { + t.Helper() + + evm := vm.NewEVM(vm.BlockContext{BlockNumber: big.NewInt(1)}, nil, params.TestChainConfig, vm.Config{}) + contract := vm.NewContract(common.Address{}, common.Address{}, value, 100_000, nil) + contract.Input = input + + bz, err := distribution.NewPrecompile(nil, nil, nil, nil, nil, nil).Run(evm, contract, false) + require.ErrorIs(t, err, vm.ErrExecutionReverted, "precompile errors are always surfaced as a revert") + + return string(bz) +} + +// TestRunResolvesSelectorBeforeNativeAction covers F-2026-18786: calldata that +// cannot be dispatched used to run the whole native-action preamble - cache +// context, multi-store snapshot and a state DB commit that replays the caller's +// entire dirty set - before SetupABI got around to failing on the selector, and +// RequiredGas returns 0 for an unresolvable selector so none of it was charged. +func TestRunResolvesSelectorBeforeNativeAction(t *testing.T) { + testCases := []struct { + name string + // input is the calldata handed to the precompile. + input []byte + // reachesNativeAction is true when the calldata is dispatchable and so + // must still be handed to RunNativeAction. + reachesNativeAction bool + // expReason, when set, must appear in the revert reason. + expReason string + }{ + { + name: "unknown selector is rejected before the native action", + input: []byte{0xde, 0xad, 0xbe, 0xef}, + expReason: "no method with id", + }, + { + name: "calldata shorter than a selector is rejected before the native action", + input: []byte{0x01, 0x02, 0x03}, + }, + { + name: "empty calldata is rejected before the native action", + input: nil, + }, + { + name: "a valid selector is still dispatched", + input: distribution.ABI.Methods[distribution.ValidatorCommissionMethod].ID, + reachesNativeAction: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + reason := runPrecompile(t, tc.input, uint256.NewInt(0)) + + if tc.reachesNativeAction { + require.Contains(t, reason, cmn.ErrNotRunInEvm, + "a valid selector must still reach RunNativeAction") + return + } + + require.NotContains(t, reason, cmn.ErrNotRunInEvm, + "undispatchable calldata must not reach the native-action preamble (F-2026-18786)") + + if tc.expReason != "" { + require.Contains(t, reason, tc.expReason) + } + }) + } +} diff --git a/precompiles/erc20/erc20.go b/precompiles/erc20/erc20.go index 12121b7c4..3d90afcaf 100644 --- a/precompiles/erc20/erc20.go +++ b/precompiles/erc20/erc20.go @@ -140,6 +140,15 @@ func (p Precompile) RequiredGas(input []byte) uint64 { } func (p Precompile) Run(evm *vm.EVM, contract *vm.Contract, readonly bool) ([]byte, error) { + // Resolve the target method before RunNativeAction touches any state: the + // native-action preamble (cache context, multi-store snapshot, state DB + // commit) replays the caller's entire dirty set and is not paid for when the + // selector turns out to be unresolvable, because RequiredGas returns 0 for + // one. Resolving here keeps the revert identical while skipping that work. + if _, err := cmn.ResolveMethod(p.ABI, contract); err != nil { + return cmn.ReturnRevertError(evm, err) + } + return p.RunNativeAction(evm, contract, func(ctx sdk.Context) ([]byte, error) { return p.Execute(ctx, evm.StateDB, contract, readonly) }) diff --git a/precompiles/erc20/erc20_selector_test.go b/precompiles/erc20/erc20_selector_test.go new file mode 100644 index 000000000..87b546e34 --- /dev/null +++ b/precompiles/erc20/erc20_selector_test.go @@ -0,0 +1,94 @@ +package erc20_test + +import ( + "math/big" + "testing" + + "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/core/vm" + "github.com/ethereum/go-ethereum/params" + "github.com/holiman/uint256" + "github.com/stretchr/testify/require" + + cmn "github.com/cosmos/evm/precompiles/common" + "github.com/cosmos/evm/precompiles/erc20" + + erc20types "github.com/cosmos/evm/x/erc20/types" +) + +// runPrecompile drives erc20.Precompile.Run with a nil StateDB and returns the +// ABI-encoded revert reason. +// +// The early selector guard never looks at evm.StateDB, whereas the first thing +// RunNativeAction does is type-assert on it. A revert reason carrying +// cmn.ErrNotRunInEvm therefore proves the call reached the native-action +// preamble, and any other reason proves it was rejected before it - which is +// what lets these tests tell the two apart without standing up a full app. +func runPrecompile(t *testing.T, input []byte, value *uint256.Int) string { + t.Helper() + + evm := vm.NewEVM(vm.BlockContext{BlockNumber: big.NewInt(1)}, nil, params.TestChainConfig, vm.Config{}) + contract := vm.NewContract(common.Address{}, common.Address{}, value, 100_000, nil) + contract.Input = input + + bz, err := erc20.NewPrecompile(erc20types.TokenPair{}, nil, nil, nil).Run(evm, contract, false) + require.ErrorIs(t, err, vm.ErrExecutionReverted, "precompile errors are always surfaced as a revert") + + return string(bz) +} + +// TestRunResolvesSelectorBeforeNativeAction covers F-2026-18786: calldata that +// cannot be dispatched used to run the whole native-action preamble - cache +// context, multi-store snapshot and a state DB commit that replays the caller's +// entire dirty set - before SetupABI got around to failing on the selector, and +// RequiredGas returns 0 for an unresolvable selector so none of it was charged. +func TestRunResolvesSelectorBeforeNativeAction(t *testing.T) { + testCases := []struct { + name string + // input is the calldata handed to the precompile. + input []byte + // reachesNativeAction is true when the calldata is dispatchable and so + // must still be handed to RunNativeAction. + reachesNativeAction bool + // expReason, when set, must appear in the revert reason. + expReason string + }{ + { + name: "unknown selector is rejected before the native action", + input: []byte{0xde, 0xad, 0xbe, 0xef}, + expReason: "no method with id", + }, + { + name: "calldata shorter than a selector is rejected before the native action", + input: []byte{0x01, 0x02, 0x03}, + }, + { + name: "empty calldata is rejected before the native action", + input: nil, + }, + { + name: "a valid selector is still dispatched", + input: erc20.ABI.Methods[erc20.NameMethod].ID, + reachesNativeAction: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + reason := runPrecompile(t, tc.input, uint256.NewInt(0)) + + if tc.reachesNativeAction { + require.Contains(t, reason, cmn.ErrNotRunInEvm, + "a valid selector must still reach RunNativeAction") + return + } + + require.NotContains(t, reason, cmn.ErrNotRunInEvm, + "undispatchable calldata must not reach the native-action preamble (F-2026-18786)") + + if tc.expReason != "" { + require.Contains(t, reason, tc.expReason) + } + }) + } +} diff --git a/precompiles/gov/gov.go b/precompiles/gov/gov.go index baf84d22b..c157874e6 100644 --- a/precompiles/gov/gov.go +++ b/precompiles/gov/gov.go @@ -90,6 +90,15 @@ func (p Precompile) RequiredGas(input []byte) uint64 { } func (p Precompile) Run(evm *vm.EVM, contract *vm.Contract, readonly bool) ([]byte, error) { + // Resolve the target method before RunNativeAction touches any state: the + // native-action preamble (cache context, multi-store snapshot, state DB + // commit) replays the caller's entire dirty set and is not paid for when the + // selector turns out to be unresolvable, because RequiredGas returns 0 for + // one. Resolving here keeps the revert identical while skipping that work. + if _, err := cmn.ResolveMethod(p.ABI, contract); err != nil { + return cmn.ReturnRevertError(evm, err) + } + return p.RunNativeAction(evm, contract, func(ctx sdk.Context) ([]byte, error) { return p.Execute(ctx, evm.StateDB, contract, readonly) }) diff --git a/precompiles/gov/gov_selector_test.go b/precompiles/gov/gov_selector_test.go new file mode 100644 index 000000000..70e7effcb --- /dev/null +++ b/precompiles/gov/gov_selector_test.go @@ -0,0 +1,92 @@ +package gov_test + +import ( + "math/big" + "testing" + + "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/core/vm" + "github.com/ethereum/go-ethereum/params" + "github.com/holiman/uint256" + "github.com/stretchr/testify/require" + + cmn "github.com/cosmos/evm/precompiles/common" + "github.com/cosmos/evm/precompiles/gov" +) + +// runPrecompile drives gov.Precompile.Run with a nil StateDB and returns the +// ABI-encoded revert reason. +// +// The early selector guard never looks at evm.StateDB, whereas the first thing +// RunNativeAction does is type-assert on it. A revert reason carrying +// cmn.ErrNotRunInEvm therefore proves the call reached the native-action +// preamble, and any other reason proves it was rejected before it - which is +// what lets these tests tell the two apart without standing up a full app. +func runPrecompile(t *testing.T, input []byte, value *uint256.Int) string { + t.Helper() + + evm := vm.NewEVM(vm.BlockContext{BlockNumber: big.NewInt(1)}, nil, params.TestChainConfig, vm.Config{}) + contract := vm.NewContract(common.Address{}, common.Address{}, value, 100_000, nil) + contract.Input = input + + bz, err := gov.NewPrecompile(nil, nil, nil, nil, nil).Run(evm, contract, false) + require.ErrorIs(t, err, vm.ErrExecutionReverted, "precompile errors are always surfaced as a revert") + + return string(bz) +} + +// TestRunResolvesSelectorBeforeNativeAction covers F-2026-18786: calldata that +// cannot be dispatched used to run the whole native-action preamble - cache +// context, multi-store snapshot and a state DB commit that replays the caller's +// entire dirty set - before SetupABI got around to failing on the selector, and +// RequiredGas returns 0 for an unresolvable selector so none of it was charged. +func TestRunResolvesSelectorBeforeNativeAction(t *testing.T) { + testCases := []struct { + name string + // input is the calldata handed to the precompile. + input []byte + // reachesNativeAction is true when the calldata is dispatchable and so + // must still be handed to RunNativeAction. + reachesNativeAction bool + // expReason, when set, must appear in the revert reason. + expReason string + }{ + { + name: "unknown selector is rejected before the native action", + input: []byte{0xde, 0xad, 0xbe, 0xef}, + expReason: "no method with id", + }, + { + name: "calldata shorter than a selector is rejected before the native action", + input: []byte{0x01, 0x02, 0x03}, + }, + { + name: "empty calldata is rejected before the native action", + input: nil, + }, + { + name: "a valid selector is still dispatched", + input: gov.ABI.Methods[gov.GetProposalMethod].ID, + reachesNativeAction: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + reason := runPrecompile(t, tc.input, uint256.NewInt(0)) + + if tc.reachesNativeAction { + require.Contains(t, reason, cmn.ErrNotRunInEvm, + "a valid selector must still reach RunNativeAction") + return + } + + require.NotContains(t, reason, cmn.ErrNotRunInEvm, + "undispatchable calldata must not reach the native-action preamble (F-2026-18786)") + + if tc.expReason != "" { + require.Contains(t, reason, tc.expReason) + } + }) + } +} diff --git a/precompiles/gov/query.go b/precompiles/gov/query.go index 0b389b9a0..ff7482f4b 100644 --- a/precompiles/gov/query.go +++ b/precompiles/gov/query.go @@ -4,6 +4,8 @@ import ( "github.com/ethereum/go-ethereum/accounts/abi" "github.com/ethereum/go-ethereum/core/vm" + vmstoretypes "github.com/cosmos/evm/x/vm/store/types" + sdk "github.com/cosmos/cosmos-sdk/types" ) @@ -136,7 +138,21 @@ func (p *Precompile) GetTallyResult( return nil, err } - res, err := p.govQuerier.TallyResult(ctx, queryTallyResultReq) + // The SDK's TallyResult query is *not* read-only for a proposal in the voting + // period: it delegates to the gov Keeper.Tally, which deletes every vote it + // counts (x/gov/keeper/tally.go). That is safe on the SDK's own query paths + // because a gRPC query runs against a context whose writes are never committed. + // + // Precompiles break that invariant: RunNativeAction commits the state DB cache + // context unconditionally, so a store write performed here would survive the + // enclosing EVM transaction and a plain CALL to getTallyResult would wipe the + // proposal's votes. Run the query against a throwaway cache instead, so the + // deletions are discarded while the tally itself -- delegations, weighted votes, + // validator defaults -- is still the one the SDK computes. + cacheCtx, discardWrites := newDiscardedCacheContext(ctx) + defer discardWrites() + + res, err := p.govQuerier.TallyResult(cacheCtx, queryTallyResultReq) if err != nil { return nil, err } @@ -145,6 +161,29 @@ func (p *Precompile) GetTallyResult( return method.Outputs.Pack(output.TallyResult) } +// newDiscardedCacheContext returns a context to run a query on, together with the +// function that throws away every store write the query performed. The caller is +// expected to always call it -- the writes are never meant to reach the parent +// store. +// +// sdk.Context.CacheContext alone is not enough inside a precompile. It defers to +// the underlying multi store's CacheMultiStore, and while the SDK's stores return +// a detached cache whose writes only reach the parent through the (here dropped) +// write function, the store the EVM runs precompiles on does not: +// x/vm/store/snapshotmulti pushes a snapshot layer and returns the very same +// store, so the layer stays live and is flushed along with everything else when +// the state DB commits. On that store the layer has to be popped explicitly, +// which is what the snapshot/revert pair below does. +func newDiscardedCacheContext(ctx sdk.Context) (sdk.Context, func()) { + if snapshotter, ok := ctx.MultiStore().(vmstoretypes.Snapshotter); ok { + snapshot := snapshotter.Snapshot() + return ctx, func() { snapshotter.RevertToSnapshot(snapshot) } + } + + cacheCtx, _ := ctx.CacheContext() // write function intentionally discarded + return cacheCtx, func() {} +} + // GetProposal implements the query logic for getting a proposal func (p *Precompile) GetProposal( ctx sdk.Context, diff --git a/precompiles/ics20/ics20.go b/precompiles/ics20/ics20.go index df90904c4..bd2702608 100644 --- a/precompiles/ics20/ics20.go +++ b/precompiles/ics20/ics20.go @@ -92,6 +92,15 @@ func (p Precompile) RequiredGas(input []byte) uint64 { } func (p Precompile) Run(evm *vm.EVM, contract *vm.Contract, readonly bool) ([]byte, error) { + // Resolve the target method before RunNativeAction touches any state: the + // native-action preamble (cache context, multi-store snapshot, state DB + // commit) replays the caller's entire dirty set and is not paid for when the + // selector turns out to be unresolvable, because RequiredGas returns 0 for + // one. Resolving here keeps the revert identical while skipping that work. + if _, err := cmn.ResolveMethod(p.ABI, contract); err != nil { + return cmn.ReturnRevertError(evm, err) + } + return p.RunNativeAction(evm, contract, func(ctx sdk.Context) ([]byte, error) { return p.Execute(ctx, evm.StateDB, contract, readonly) }) diff --git a/precompiles/ics20/ics20_selector_test.go b/precompiles/ics20/ics20_selector_test.go new file mode 100644 index 000000000..0fa854fe6 --- /dev/null +++ b/precompiles/ics20/ics20_selector_test.go @@ -0,0 +1,92 @@ +package ics20_test + +import ( + "math/big" + "testing" + + "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/core/vm" + "github.com/ethereum/go-ethereum/params" + "github.com/holiman/uint256" + "github.com/stretchr/testify/require" + + cmn "github.com/cosmos/evm/precompiles/common" + "github.com/cosmos/evm/precompiles/ics20" +) + +// runPrecompile drives ics20.Precompile.Run with a nil StateDB and returns the +// ABI-encoded revert reason. +// +// The early selector guard never looks at evm.StateDB, whereas the first thing +// RunNativeAction does is type-assert on it. A revert reason carrying +// cmn.ErrNotRunInEvm therefore proves the call reached the native-action +// preamble, and any other reason proves it was rejected before it - which is +// what lets these tests tell the two apart without standing up a full app. +func runPrecompile(t *testing.T, input []byte, value *uint256.Int) string { + t.Helper() + + evm := vm.NewEVM(vm.BlockContext{BlockNumber: big.NewInt(1)}, nil, params.TestChainConfig, vm.Config{}) + contract := vm.NewContract(common.Address{}, common.Address{}, value, 100_000, nil) + contract.Input = input + + bz, err := ics20.NewPrecompile(nil, nil, nil, nil, nil).Run(evm, contract, false) + require.ErrorIs(t, err, vm.ErrExecutionReverted, "precompile errors are always surfaced as a revert") + + return string(bz) +} + +// TestRunResolvesSelectorBeforeNativeAction covers F-2026-18786: calldata that +// cannot be dispatched used to run the whole native-action preamble - cache +// context, multi-store snapshot and a state DB commit that replays the caller's +// entire dirty set - before SetupABI got around to failing on the selector, and +// RequiredGas returns 0 for an unresolvable selector so none of it was charged. +func TestRunResolvesSelectorBeforeNativeAction(t *testing.T) { + testCases := []struct { + name string + // input is the calldata handed to the precompile. + input []byte + // reachesNativeAction is true when the calldata is dispatchable and so + // must still be handed to RunNativeAction. + reachesNativeAction bool + // expReason, when set, must appear in the revert reason. + expReason string + }{ + { + name: "unknown selector is rejected before the native action", + input: []byte{0xde, 0xad, 0xbe, 0xef}, + expReason: "no method with id", + }, + { + name: "calldata shorter than a selector is rejected before the native action", + input: []byte{0x01, 0x02, 0x03}, + }, + { + name: "empty calldata is rejected before the native action", + input: nil, + }, + { + name: "a valid selector is still dispatched", + input: ics20.ABI.Methods[ics20.DenomsMethod].ID, + reachesNativeAction: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + reason := runPrecompile(t, tc.input, uint256.NewInt(0)) + + if tc.reachesNativeAction { + require.Contains(t, reason, cmn.ErrNotRunInEvm, + "a valid selector must still reach RunNativeAction") + return + } + + require.NotContains(t, reason, cmn.ErrNotRunInEvm, + "undispatchable calldata must not reach the native-action preamble (F-2026-18786)") + + if tc.expReason != "" { + require.Contains(t, reason, tc.expReason) + } + }) + } +} diff --git a/precompiles/slashing/slashing.go b/precompiles/slashing/slashing.go index bc99e984e..47d2ad15c 100644 --- a/precompiles/slashing/slashing.go +++ b/precompiles/slashing/slashing.go @@ -90,6 +90,15 @@ func (p Precompile) RequiredGas(input []byte) uint64 { } func (p Precompile) Run(evm *vm.EVM, contract *vm.Contract, readonly bool) ([]byte, error) { + // Resolve the target method before RunNativeAction touches any state: the + // native-action preamble (cache context, multi-store snapshot, state DB + // commit) replays the caller's entire dirty set and is not paid for when the + // selector turns out to be unresolvable, because RequiredGas returns 0 for + // one. Resolving here keeps the revert identical while skipping that work. + if _, err := cmn.ResolveMethod(p.ABI, contract); err != nil { + return cmn.ReturnRevertError(evm, err) + } + return p.RunNativeAction(evm, contract, func(ctx sdk.Context) ([]byte, error) { return p.Execute(ctx, evm.StateDB, contract, readonly) }) diff --git a/precompiles/slashing/slashing_selector_test.go b/precompiles/slashing/slashing_selector_test.go new file mode 100644 index 000000000..4bc530ca9 --- /dev/null +++ b/precompiles/slashing/slashing_selector_test.go @@ -0,0 +1,92 @@ +package slashing_test + +import ( + "math/big" + "testing" + + "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/core/vm" + "github.com/ethereum/go-ethereum/params" + "github.com/holiman/uint256" + "github.com/stretchr/testify/require" + + cmn "github.com/cosmos/evm/precompiles/common" + "github.com/cosmos/evm/precompiles/slashing" +) + +// runPrecompile drives slashing.Precompile.Run with a nil StateDB and returns the +// ABI-encoded revert reason. +// +// The early selector guard never looks at evm.StateDB, whereas the first thing +// RunNativeAction does is type-assert on it. A revert reason carrying +// cmn.ErrNotRunInEvm therefore proves the call reached the native-action +// preamble, and any other reason proves it was rejected before it - which is +// what lets these tests tell the two apart without standing up a full app. +func runPrecompile(t *testing.T, input []byte, value *uint256.Int) string { + t.Helper() + + evm := vm.NewEVM(vm.BlockContext{BlockNumber: big.NewInt(1)}, nil, params.TestChainConfig, vm.Config{}) + contract := vm.NewContract(common.Address{}, common.Address{}, value, 100_000, nil) + contract.Input = input + + bz, err := slashing.NewPrecompile(nil, nil, nil, nil, nil).Run(evm, contract, false) + require.ErrorIs(t, err, vm.ErrExecutionReverted, "precompile errors are always surfaced as a revert") + + return string(bz) +} + +// TestRunResolvesSelectorBeforeNativeAction covers F-2026-18786: calldata that +// cannot be dispatched used to run the whole native-action preamble - cache +// context, multi-store snapshot and a state DB commit that replays the caller's +// entire dirty set - before SetupABI got around to failing on the selector, and +// RequiredGas returns 0 for an unresolvable selector so none of it was charged. +func TestRunResolvesSelectorBeforeNativeAction(t *testing.T) { + testCases := []struct { + name string + // input is the calldata handed to the precompile. + input []byte + // reachesNativeAction is true when the calldata is dispatchable and so + // must still be handed to RunNativeAction. + reachesNativeAction bool + // expReason, when set, must appear in the revert reason. + expReason string + }{ + { + name: "unknown selector is rejected before the native action", + input: []byte{0xde, 0xad, 0xbe, 0xef}, + expReason: "no method with id", + }, + { + name: "calldata shorter than a selector is rejected before the native action", + input: []byte{0x01, 0x02, 0x03}, + }, + { + name: "empty calldata is rejected before the native action", + input: nil, + }, + { + name: "a valid selector is still dispatched", + input: slashing.ABI.Methods[slashing.GetParamsMethod].ID, + reachesNativeAction: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + reason := runPrecompile(t, tc.input, uint256.NewInt(0)) + + if tc.reachesNativeAction { + require.Contains(t, reason, cmn.ErrNotRunInEvm, + "a valid selector must still reach RunNativeAction") + return + } + + require.NotContains(t, reason, cmn.ErrNotRunInEvm, + "undispatchable calldata must not reach the native-action preamble (F-2026-18786)") + + if tc.expReason != "" { + require.Contains(t, reason, tc.expReason) + } + }) + } +} diff --git a/precompiles/staking/staking.go b/precompiles/staking/staking.go index 9dd71a6a4..649954b0d 100644 --- a/precompiles/staking/staking.go +++ b/precompiles/staking/staking.go @@ -91,6 +91,15 @@ func (p Precompile) RequiredGas(input []byte) uint64 { } func (p Precompile) Run(evm *vm.EVM, contract *vm.Contract, readonly bool) ([]byte, error) { + // Resolve the target method before RunNativeAction touches any state: the + // native-action preamble (cache context, multi-store snapshot, state DB + // commit) replays the caller's entire dirty set and is not paid for when the + // selector turns out to be unresolvable, because RequiredGas returns 0 for + // one. Resolving here keeps the revert identical while skipping that work. + if _, err := cmn.ResolveMethod(p.ABI, contract); err != nil { + return cmn.ReturnRevertError(evm, err) + } + return p.RunNativeAction(evm, contract, func(ctx sdk.Context) ([]byte, error) { return p.Execute(ctx, evm.StateDB, contract, readonly) }) diff --git a/precompiles/staking/staking_selector_test.go b/precompiles/staking/staking_selector_test.go new file mode 100644 index 000000000..9f1f5496f --- /dev/null +++ b/precompiles/staking/staking_selector_test.go @@ -0,0 +1,92 @@ +package staking_test + +import ( + "math/big" + "testing" + + "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/core/vm" + "github.com/ethereum/go-ethereum/params" + "github.com/holiman/uint256" + "github.com/stretchr/testify/require" + + cmn "github.com/cosmos/evm/precompiles/common" + "github.com/cosmos/evm/precompiles/staking" +) + +// runPrecompile drives staking.Precompile.Run with a nil StateDB and returns the +// ABI-encoded revert reason. +// +// The early selector guard never looks at evm.StateDB, whereas the first thing +// RunNativeAction does is type-assert on it. A revert reason carrying +// cmn.ErrNotRunInEvm therefore proves the call reached the native-action +// preamble, and any other reason proves it was rejected before it - which is +// what lets these tests tell the two apart without standing up a full app. +func runPrecompile(t *testing.T, input []byte, value *uint256.Int) string { + t.Helper() + + evm := vm.NewEVM(vm.BlockContext{BlockNumber: big.NewInt(1)}, nil, params.TestChainConfig, vm.Config{}) + contract := vm.NewContract(common.Address{}, common.Address{}, value, 100_000, nil) + contract.Input = input + + bz, err := staking.NewPrecompile(nil, nil, nil, nil, nil).Run(evm, contract, false) + require.ErrorIs(t, err, vm.ErrExecutionReverted, "precompile errors are always surfaced as a revert") + + return string(bz) +} + +// TestRunResolvesSelectorBeforeNativeAction covers F-2026-18786: calldata that +// cannot be dispatched used to run the whole native-action preamble - cache +// context, multi-store snapshot and a state DB commit that replays the caller's +// entire dirty set - before SetupABI got around to failing on the selector, and +// RequiredGas returns 0 for an unresolvable selector so none of it was charged. +func TestRunResolvesSelectorBeforeNativeAction(t *testing.T) { + testCases := []struct { + name string + // input is the calldata handed to the precompile. + input []byte + // reachesNativeAction is true when the calldata is dispatchable and so + // must still be handed to RunNativeAction. + reachesNativeAction bool + // expReason, when set, must appear in the revert reason. + expReason string + }{ + { + name: "unknown selector is rejected before the native action", + input: []byte{0xde, 0xad, 0xbe, 0xef}, + expReason: "no method with id", + }, + { + name: "calldata shorter than a selector is rejected before the native action", + input: []byte{0x01, 0x02, 0x03}, + }, + { + name: "empty calldata is rejected before the native action", + input: nil, + }, + { + name: "a valid selector is still dispatched", + input: staking.ABI.Methods[staking.DelegationMethod].ID, + reachesNativeAction: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + reason := runPrecompile(t, tc.input, uint256.NewInt(0)) + + if tc.reachesNativeAction { + require.Contains(t, reason, cmn.ErrNotRunInEvm, + "a valid selector must still reach RunNativeAction") + return + } + + require.NotContains(t, reason, cmn.ErrNotRunInEvm, + "undispatchable calldata must not reach the native-action preamble (F-2026-18786)") + + if tc.expReason != "" { + require.Contains(t, reason, tc.expReason) + } + }) + } +} diff --git a/precompiles/werc20/werc20.go b/precompiles/werc20/werc20.go index b9e67623e..616788fcf 100644 --- a/precompiles/werc20/werc20.go +++ b/precompiles/werc20/werc20.go @@ -95,6 +95,15 @@ func (p Precompile) RequiredGas(input []byte) uint64 { } func (p Precompile) Run(evm *vm.EVM, contract *vm.Contract, readonly bool) ([]byte, error) { + // Resolve the target method before RunNativeAction touches any state: the + // native-action preamble (cache context, multi-store snapshot, state DB + // commit) replays the caller's entire dirty set and is not paid for when the + // selector turns out to be unresolvable, because RequiredGas returns 0 for + // one. Resolving here keeps the revert identical while skipping that work. + if _, err := cmn.ResolveMethod(p.ABI, contract); err != nil { + return cmn.ReturnRevertError(evm, err) + } + return p.RunNativeAction(evm, contract, func(ctx sdk.Context) ([]byte, error) { return p.Execute(ctx, evm.StateDB, contract, readonly) }) diff --git a/precompiles/werc20/werc20_selector_test.go b/precompiles/werc20/werc20_selector_test.go new file mode 100644 index 000000000..51fa8a0f9 --- /dev/null +++ b/precompiles/werc20/werc20_selector_test.go @@ -0,0 +1,91 @@ +package werc20_test + +import ( + "math/big" + "testing" + + "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/core/vm" + "github.com/ethereum/go-ethereum/params" + "github.com/holiman/uint256" + "github.com/stretchr/testify/require" + + cmn "github.com/cosmos/evm/precompiles/common" + "github.com/cosmos/evm/precompiles/werc20" + erc20types "github.com/cosmos/evm/x/erc20/types" +) + +// runPrecompile drives werc20.Precompile.Run with a nil StateDB and returns the +// ABI-encoded revert reason. +// +// The early selector guard never looks at evm.StateDB, whereas the first thing +// RunNativeAction does is type-assert on it. A revert reason carrying +// cmn.ErrNotRunInEvm therefore proves the call reached the native-action +// preamble, and any other reason proves it was rejected before it - which is +// what lets these tests tell the two apart without standing up a full app. +func runPrecompile(t *testing.T, input []byte, value *uint256.Int) string { + t.Helper() + + evm := vm.NewEVM(vm.BlockContext{BlockNumber: big.NewInt(1)}, nil, params.TestChainConfig, vm.Config{}) + contract := vm.NewContract(common.Address{}, common.Address{}, value, 100_000, nil) + contract.Input = input + + bz, err := werc20.NewPrecompile(erc20types.TokenPair{}, nil, nil, nil).Run(evm, contract, false) + require.ErrorIs(t, err, vm.ErrExecutionReverted, "precompile errors are always surfaced as a revert") + + return string(bz) +} + +// TestRunPreservesFallbackAndReceive is the regression guard for F-2026-18786. +// +// WERC20 is the one affected precompile whose ABI declares a `fallback` and a +// `receive`, and it leans on all three of SetupABI's special cases: empty +// calldata, calldata shorter than a selector, and an unknown selector all +// resolve to fallback/receive and behave like `deposit`. A naive +// "len(input) < 4 || MethodById fails -> revert" guard at the top of Run would +// have broken every one of them, so the guard resolves the method through +// cmn.ResolveMethod - the same helper SetupABI itself uses - instead. +func TestRunPreservesFallbackAndReceive(t *testing.T) { + testCases := []struct { + name string + input []byte + value *uint256.Int + }{ + { + name: "empty calldata with value resolves to receive", + input: nil, + value: uint256.NewInt(1), + }, + { + name: "empty calldata without value resolves to fallback", + input: nil, + value: uint256.NewInt(0), + }, + { + name: "calldata shorter than a selector resolves to fallback", + input: []byte{0x01, 0x02, 0x03}, + value: uint256.NewInt(1), + }, + { + name: "unknown selector resolves to fallback", + input: []byte("nonExistingMethod"), + value: uint256.NewInt(1), + }, + { + name: "a valid selector is still dispatched", + input: werc20.ABI.Methods[werc20.DepositMethod].ID, + value: uint256.NewInt(1), + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + reason := runPrecompile(t, tc.input, tc.value) + + require.Contains(t, reason, cmn.ErrNotRunInEvm, + "WERC20 dispatches this calldata via fallback/receive, so it must still reach RunNativeAction") + require.NotContains(t, reason, "no method with id", + "the early selector guard must not reject calldata that fallback/receive would have handled") + }) + } +} diff --git a/tests/integration/precompiles/gov/test_gov.go b/tests/integration/precompiles/gov/test_gov.go index b2a2aee96..fca69a6c4 100644 --- a/tests/integration/precompiles/gov/test_gov.go +++ b/tests/integration/precompiles/gov/test_gov.go @@ -13,6 +13,11 @@ import ( "github.com/cosmos/evm/testutil" "github.com/cosmos/evm/x/vm/statedb" evmtypes "github.com/cosmos/evm/x/vm/types" + + "cosmossdk.io/collections" + + sdk "github.com/cosmos/cosmos-sdk/types" + govv1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1" ) func (s *PrecompileTestSuite) TestIsTransaction() { @@ -136,3 +141,162 @@ func (s *PrecompileTestSuite) TestRun() { }) } } + +// runPrecompile executes the gov precompile with the given calldata, mirroring +// what the EVM does for a delivered transaction: the state DB is committed after +// the call so that every store write performed by the precompile is flushed to +// the underlying context. +func (s *PrecompileTestSuite) runPrecompile(ctx sdk.Context, input []byte, readOnly bool) []byte { + baseFee := s.network.App.GetEVMKeeper().GetBaseFee(ctx) + + contract := vm.NewPrecompile(s.keyring.GetAddr(0), s.precompile.Address(), uint256.NewInt(0), uint64(1e6)) + contract.Input = input + + contractAddr := contract.Address() + // Build and sign the Ethereum transaction backing the call. + txArgs := evmtypes.EvmTxArgs{ + ChainID: evmtypes.GetEthChainConfig().ChainID, + Nonce: 0, + To: &contractAddr, + Amount: nil, + GasLimit: 100000, + GasPrice: testutil.ExampleMinGasPrices, + GasFeeCap: baseFee, + GasTipCap: big.NewInt(1), + Accesses: ðtypes.AccessList{}, + } + msg, err := s.factory.GenerateGethCoreMsg(s.keyring.GetPrivKey(0), txArgs) + s.Require().NoError(err) + + cfg, err := s.network.App.GetEVMKeeper().EVMConfig(ctx, ctx.BlockHeader().ProposerAddress) + s.Require().NoError(err, "failed to instantiate EVM config") + + stDB := statedb.New(ctx, s.network.App.GetEVMKeeper(), statedb.NewEmptyTxConfig()) + evm := s.network.App.GetEVMKeeper().NewEVM(ctx, *msg, cfg, nil, stDB) + + precompiles, found, err := s.network.App.GetEVMKeeper().GetPrecompileInstance(ctx, contractAddr) + s.Require().NoError(err, "failed to instantiate precompile") + s.Require().True(found, "not found precompile") + evm.WithPrecompiles(precompiles.Map) + + bz, err := s.precompile.Run(evm, contract, readOnly) + s.Require().NoError(err, "expected no error when running the precompile") + s.Require().NotNil(bz, "expected returned bytes not to be nil") + + // A delivered tx always commits the state DB, which flushes the precompile's + // cache context onto the underlying multi store. + s.Require().NoError(stDB.Commit(), "failed to commit the state DB") + + return bz +} + +// proposalVoters returns the voters that currently have a vote stored for the +// given proposal. +func (s *PrecompileTestSuite) proposalVoters(ctx sdk.Context, proposalID uint64) []string { + govKeeper := s.network.App.GetGovKeeper() + + voters := []string{} + err := govKeeper.Votes.Walk( + ctx, + collections.NewPrefixedPairRange[uint64, sdk.AccAddress](proposalID), + func(_ collections.Pair[uint64, sdk.AccAddress], vote govv1.Vote) (bool, error) { + voters = append(voters, vote.Voter) + return false, nil + }, + ) + s.Require().NoError(err, "failed to walk the proposal votes") + + return voters +} + +// TestGetTallyResultDoesNotDeleteVotes is the regression test for F-2026-18187. +// +// The SDK's gov TallyResult query delegates to Keeper.Tally, which deletes every +// vote it counts. That is safe on the SDK's own query paths because a gRPC query +// runs against a context whose writes are never committed. Precompiles break that +// invariant: cmn.Precompile.runNativeAction commits the state DB cache context +// unconditionally, so before the fix a plain committing EVM call to +// getTallyResult wiped the votes of any proposal in the voting period. +// +// Query classification does not help here: getTallyResult is absent from +// IsTransaction, but that only feeds the staticcall guard and the gas price, so +// both a CALL and a STATICCALL reach the mutating SDK code. Both are covered. +func (s *PrecompileTestSuite) TestGetTallyResultDoesNotDeleteVotes() { + const proposalID uint64 = 1 + + testcases := []struct { + name string + readOnly bool + }{ + { + name: "call", + readOnly: false, + }, + { + name: "staticcall", + readOnly: true, + }, + } + + for _, tc := range testcases { + s.Run(tc.name, func() { + s.SetupTest() + + ctx := s.network.GetContext() + govKeeper := s.network.App.GetGovKeeper() + + // Account 0 is the only delegator of the test network, so it holds all of + // the voting power (3e18). Split its vote across every option so that the + // positive control below exercises the whole tally result. + s.Require().NoError(govKeeper.AddVote(ctx, proposalID, s.keyring.GetAccAddr(0), govv1.WeightedVoteOptions{ + {Option: govv1.OptionYes, Weight: "0.4"}, + {Option: govv1.OptionAbstain, Weight: "0.3"}, + {Option: govv1.OptionNo, Weight: "0.2"}, + {Option: govv1.OptionNoWithVeto, Weight: "0.1"}, + }, "")) + // Accounts 1 and 2 hold no delegations: they add nothing to the tally but + // are stored votes all the same, and Keeper.Tally removes them too. + s.Require().NoError(govKeeper.AddVote(ctx, proposalID, s.keyring.GetAccAddr(1), govv1.NewNonSplitVoteOption(govv1.OptionNo), "")) + s.Require().NoError(govKeeper.AddVote(ctx, proposalID, s.keyring.GetAccAddr(2), govv1.NewNonSplitVoteOption(govv1.OptionYes), "")) + + expVoters := []string{ + s.keyring.GetAccAddr(0).String(), + s.keyring.GetAccAddr(1).String(), + s.keyring.GetAccAddr(2).String(), + } + s.Require().ElementsMatch(expVoters, s.proposalVoters(ctx, proposalID), "expected the votes to be stored before the query") + + proposal, err := govKeeper.Proposals.Get(ctx, proposalID) + s.Require().NoError(err) + s.Require().Equal(govv1.StatusVotingPeriod, proposal.Status, "the proposal must be in the voting period for the tally to mutate state") + + input, err := s.precompile.Pack(gov.GetTallyResultMethod, proposalID) + s.Require().NoError(err, "failed to pack input") + + bz := s.runPrecompile(ctx, input, tc.readOnly) + + // Regression assertion: the query must leave every vote in place. + s.Require().ElementsMatch(expVoters, s.proposalVoters(ctx, proposalID), "getTallyResult must not delete the proposal's votes") + + // Positive control: the tally must still be the one the SDK computes, so + // we do not trade a mutating query for a wrong one. + expTally := gov.TallyResultData{ + Yes: "1200000000000000000", // 40% of 3e18 + Abstain: "900000000000000000", // 30% of 3e18 + No: "600000000000000000", // 20% of 3e18 + NoWithVeto: "300000000000000000", // 10% of 3e18 + } + + var out gov.TallyResultOutput + s.Require().NoError(s.precompile.UnpackIntoInterface(&out, gov.GetTallyResultMethod, bz)) + s.Require().Equal(expTally, out.TallyResult, "unexpected tally result") + + // And the query must be repeatable: a second call sees the same votes and + // therefore returns the same numbers. + var secondOut gov.TallyResultOutput + s.Require().NoError(s.precompile.UnpackIntoInterface(&secondOut, gov.GetTallyResultMethod, s.runPrecompile(ctx, input, tc.readOnly))) + s.Require().Equal(expTally, secondOut.TallyResult, "the tally result must be stable across calls") + s.Require().ElementsMatch(expVoters, s.proposalVoters(ctx, proposalID), "the votes must survive repeated queries") + }) + } +} diff --git a/tests/integration/x/erc20/test_convert.go b/tests/integration/x/erc20/test_convert.go index a0d1599b6..c510c4cbe 100644 --- a/tests/integration/x/erc20/test_convert.go +++ b/tests/integration/x/erc20/test_convert.go @@ -102,13 +102,20 @@ func (s *KeeperTestSuite) TestConvertERC20IntoCoinsForNativeToken() { false, }, { - "pass - delayed malicious contract", + // The contract grants a third party an allowance over the escrow + // module on every `transfer`, which would leave the minted coins + // unbacked once that allowance is spent. The unexpected `Approval` + // event in the transfer logs must abort the conversion + // (F-2026-18822). Upstream v0.6.0 still expects this to pass because + // it moved this body into x/erc20/keeper/convert.go without the + // validateApprovalEventDoesNotExist guard. + "fail - delayed malicious contract", 10, 10, func(common.Address) {}, func() {}, contractMaliciousDelayed, - true, + false, false, }, { diff --git a/tests/integration/x/erc20/test_ibc_callback.go b/tests/integration/x/erc20/test_ibc_callback.go index 957a7e629..056d8f6ee 100644 --- a/tests/integration/x/erc20/test_ibc_callback.go +++ b/tests/integration/x/erc20/test_ibc_callback.go @@ -771,3 +771,130 @@ func (s *KeeperTestSuite) TestOnTimeoutPacket() { }) } } + +// setupUnbackedPairAndFundSender registers a native-ERC20 pair and funds the +// sender with the paired bank denom, while deliberately leaving the erc20 module +// account with NO token balance to unescrow from. +// +// That produces the exact failure shape F-2026-18819 needs. Walking +// convertCoinNativeERC20 in order: +// +// balanceOf(receiver) -> succeeds (a plain view) +// SendCoinsFromAccountToModule -> succeeds, coins now on the module +// CallEVM "transfer" -> REVERTS, module holds nothing to send +// +// The failure therefore lands strictly after the bank mutation, which is the +// only ordering that can strand the sender's coins. Pausing the token does not +// work here: it fails the balanceOf read instead, before any escrow happens, and +// the test would pass vacuously. +// +// It returns a context fetched *after* the deployment: the suite's contract +// deployment only becomes visible to EVM reads on a context taken afterwards, so +// reusing an earlier one makes balanceOf return nil and the conversion fail +// before it ever escrows. +func (s *KeeperTestSuite) setupUnbackedPairAndFundSender( + sender sdk.AccAddress, fund math.Int, +) (sdk.Context, types.TokenPair) { + contractAddr, err := s.setupRegisterERC20Pair(contractMinterBurner) + s.Require().NoError(err, "failed to register pair") + + ctx := s.network.GetContext() + + id := s.network.App.GetErc20Keeper().GetTokenPairID(ctx, contractAddr.String()) + pair, found := s.network.App.GetErc20Keeper().GetTokenPair(ctx, id) + s.Require().True(found) + + s.Require().NoError(testutil.FundAccount( + ctx, s.network.App.GetBankKeeper(), sender, + sdk.NewCoins(sdk.NewCoin(pair.Denom, fund)), + )) + + // Sanity: the module must hold no tokens, or the transfer would succeed and + // the test would prove nothing. + moduleTokens := s.network.App.GetErc20Keeper().BalanceOf( + ctx, contracts.ERC20MinterBurnerDecimalsContract.ABI, contractAddr, types.ModuleAddress, + ) + s.Require().NotNil(moduleTokens, "balanceOf must work - the failure has to come from transfer") + s.Require().Zero(moduleTokens.Sign(), "module must have nothing to unescrow") + + return ctx, pair +} + +// TestConvertCoinNativeERC20RollsBackEscrowOnEVMFailure is the direct guard for +// F-2026-18819. +// +// ConvertCoinNativeERC20 escrows the sender's coins onto the erc20 module +// account and only then calls the EVM. CallEVM caches solely the EVM execution, +// so before the fix a VM failure discarded the EVM cache but left the bank +// escrow committed - the sender's coins ended up stranded on a module account +// with no user-facing way to reclaim them. +// +// Asserting on the error alone would not catch that. The assertions that matter +// are the balances. +func (s *KeeperTestSuite) TestConvertCoinNativeERC20RollsBackEscrowOnEVMFailure() { + s.mintFeeCollector = true + defer func() { s.mintFeeCollector = false }() + s.SetupTest() + + bankKeeper := s.network.App.GetBankKeeper() + moduleAcc := authtypes.NewModuleAddress(types.ModuleName) + + senderPk := secp256k1.GenPrivKey() + sender := sdk.AccAddress(senderPk.PubKey().Address()) + + ctx, pair := s.setupUnbackedPairAndFundSender(sender, math.NewInt(100)) + + senderBefore := bankKeeper.GetBalance(ctx, sender, pair.Denom) + moduleBefore := bankKeeper.GetBalance(ctx, moduleAcc, pair.Denom) + supplyBefore := bankKeeper.GetSupply(ctx, pair.Denom) + s.Require().Equal(math.NewInt(100), senderBefore.Amount) + + err := s.network.App.GetErc20Keeper().ConvertCoinNativeERC20( + ctx, pair, math.NewInt(10), common.BytesToAddress(sender), sender, false, + ) + s.Require().Error(err, "a paused token must fail the unescrow transfer") + + s.Require().Equal(senderBefore, bankKeeper.GetBalance(ctx, sender, pair.Denom), + "sender must keep every coin - the escrow has to roll back with the EVM failure") + s.Require().Equal(moduleBefore, bankKeeper.GetBalance(ctx, moduleAcc, pair.Denom), + "no coins may be stranded on the erc20 module account") + s.Require().Equal(supplyBefore, bankKeeper.GetSupply(ctx, pair.Denom), + "a failed conversion must not burn supply") +} + +// TestConvertCoinToERC20FromPacketLeavesRefundWithSender covers the same defect +// one layer up, on the path that actually made it permanent. +// +// The IBC error-ACK / timeout wrapper deliberately swallows a conversion failure +// so a failed re-wrap cannot undo the refund itself, and the block commits. The +// doc comments on OnAcknowledgementPacket / OnTimeoutPacket promise that the +// user "receives the corresponding bank token from the TokenPair instead" - this +// pins that promise down. +func (s *KeeperTestSuite) TestConvertCoinToERC20FromPacketLeavesRefundWithSender() { + s.mintFeeCollector = true + defer func() { s.mintFeeCollector = false }() + s.SetupTest() + + bankKeeper := s.network.App.GetBankKeeper() + moduleAcc := authtypes.NewModuleAddress(types.ModuleName) + + senderPk := secp256k1.GenPrivKey() + sender := sdk.AccAddress(senderPk.PubKey().Address()) + + ctx, pair := s.setupUnbackedPairAndFundSender(sender, math.NewInt(100)) + + senderBefore := bankKeeper.GetBalance(ctx, sender, pair.Denom) + moduleBefore := bankKeeper.GetBalance(ctx, moduleAcc, pair.Denom) + + data := transfertypes.NewFungibleTokenPacketData(pair.Denom, "10", sender.String(), "", "") + + // The wrapper swallows the conversion error by design; the IBC lifecycle + // must continue. + err := s.network.App.GetErc20Keeper().ConvertCoinToERC20FromPacket(ctx, data) + s.Require().NoError(err, "the refund path must not surface the conversion failure") + + s.Require().Equal(senderBefore, bankKeeper.GetBalance(ctx, sender, pair.Denom), + "the refunded coins must still be with the sender, who can retry the conversion") + s.Require().Equal(moduleBefore, bankKeeper.GetBalance(ctx, moduleAcc, pair.Denom), + "the refund must not be stranded on the erc20 module account") +} diff --git a/tests/integration/x/erc20/test_msg_server.go b/tests/integration/x/erc20/test_msg_server.go index 18f5d010c..d09f2bd9b 100644 --- a/tests/integration/x/erc20/test_msg_server.go +++ b/tests/integration/x/erc20/test_msg_server.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/mock" "go.uber.org/mock/gomock" + "github.com/cosmos/evm/contracts" "github.com/cosmos/evm/testutil/integration/base/factory" "github.com/cosmos/evm/x/erc20/keeper" "github.com/cosmos/evm/x/erc20/types" @@ -140,6 +141,150 @@ func (s *KeeperTestSuite) TestConvertERC20() { } } +// TestConvertERC20MaliciousApprovalKeepsEscrowInvariant asserts the escrow-and-mint invariant +// for the ERC20 -> Coin direction when the registered token hides an allowance grant inside its +// `transfer`. The module's balance check alone is satisfied by such a token - the escrow only +// drains later, when the third party spends the allowance - so the conversion has to be rejected +// on the unexpected `Approval` event. Asserting the error is not enough: the point of the fix is +// that no coins get minted against an escrow that can be emptied afterwards. +func (s *KeeperTestSuite) TestConvertERC20MaliciousApprovalKeepsEscrowInvariant() { + s.mintFeeCollector = true + defer func() { + s.mintFeeCollector = false + }() + s.SetupTest() + + contractAddr, err := s.setupRegisterERC20Pair(contractMaliciousDelayed) + s.Require().NoError(err) + s.Require().NotEqual(common.Address{}, contractAddr) + + var ( + erc20ABI = contracts.ERC20MinterBurnerDecimalsContract.ABI + sender = s.keyring.GetAccAddr(0) + senderHex = s.keyring.GetAddr(0) + amount = math.NewInt(10) + denom = types.CreateDenom(contractAddr.String()) + ) + + _, err = s.MintERC20Token(contractAddr, senderHex, amount.BigInt()) + s.Require().NoError(err) + + ctx := s.network.GetContext() + erc20Keeper := s.network.App.GetErc20Keeper() + bankKeeper := s.network.App.GetBankKeeper() + + escrowBefore := erc20Keeper.BalanceOf(ctx, erc20ABI, contractAddr, types.ModuleAddress) + s.Require().NotNil(escrowBefore) + senderTokensBefore := erc20Keeper.BalanceOf(ctx, erc20ABI, contractAddr, senderHex) + s.Require().NotNil(senderTokensBefore) + supplyBefore := bankKeeper.GetSupply(ctx, denom) + coinsBefore := bankKeeper.GetBalance(ctx, sender, denom) + s.Require().True(supplyBefore.IsZero(), "no coins should exist for the pair yet") + + convertMsg := types.NewMsgConvertERC20(amount, sender, contractAddr, senderHex) + + // Direct keeper call on a throwaway branch, to pin the rejection reason. + cacheCtx, _ := ctx.CacheContext() + _, err = erc20Keeper.ConvertERC20(cacheCtx, convertMsg) + s.Require().Error(err) + s.Require().ErrorContains(err, "unexpected Approval event") + + // Same message through a real tx, so the failed message's writes are rolled back + // exactly as they would be on chain. The gas limit is set explicitly because the + // factory would otherwise simulate the tx, and the simulation fails for the same reason. + gasLimit := uint64(1_000_000) + res, err := s.factory.CommitCosmosTx(s.keyring.GetPrivKey(0), factory.CosmosTxArgs{ + Gas: &gasLimit, + Msgs: []sdk.Msg{convertMsg}, + }) + s.Require().NoError(err) + s.Require().NotEqual(uint32(0), res.Code, "expected the conversion tx to fail, got log: %s", res.Log) + s.Require().Contains(res.Log, "unexpected Approval event") + + // Invariant: nothing escrowed, nothing minted. + ctx = s.network.GetContext() + + escrowAfter := erc20Keeper.BalanceOf(ctx, erc20ABI, contractAddr, types.ModuleAddress) + s.Require().NotNil(escrowAfter) + s.Require().Zero(escrowBefore.Cmp(escrowAfter), "escrow moved: before %s, after %s", escrowBefore, escrowAfter) + + senderTokensAfter := erc20Keeper.BalanceOf(ctx, erc20ABI, contractAddr, senderHex) + s.Require().NotNil(senderTokensAfter) + s.Require().Zero(senderTokensBefore.Cmp(senderTokensAfter), "sender tokens moved: before %s, after %s", senderTokensBefore, senderTokensAfter) + + supplyAfter := bankKeeper.GetSupply(ctx, denom) + s.Require().True(supplyAfter.IsZero(), "unbacked coins were minted: %s", supplyAfter) + s.Require().Equal(supplyBefore, supplyAfter) + s.Require().Equal(coinsBefore, bankKeeper.GetBalance(ctx, sender, denom)) +} + +// TestConvertCoinMaliciousApprovalKeepsEscrowInvariant covers the Coin -> ERC20 direction of the +// same guard. Here the hidden allowance is granted over the receiver rather than the module, so +// the unescrow must be rejected before any token leaves escrow and before the coins are burned. +// +// The whole exercise runs on a throwaway branch of the context: the malicious pair can never +// reach this state through committed txs, because the ERC20 -> Coin leg that would normally mint +// the coins is itself rejected by the same guard. +func (s *KeeperTestSuite) TestConvertCoinMaliciousApprovalKeepsEscrowInvariant() { + s.mintFeeCollector = true + defer func() { + s.mintFeeCollector = false + }() + s.SetupTest() + + contractAddr, err := s.setupRegisterERC20Pair(contractMaliciousDelayed) + s.Require().NoError(err) + s.Require().NotEqual(common.Address{}, contractAddr) + + var ( + erc20ABI = contracts.ERC20MinterBurnerDecimalsContract.ABI + sender = s.keyring.GetAccAddr(0) + receiverHex = s.keyring.GetAddr(1) + amount = math.NewInt(10) + denom = types.CreateDenom(contractAddr.String()) + ) + + // Fund the escrow directly: `mint` does not route through the malicious `transfer`. + _, err = s.MintERC20Token(contractAddr, types.ModuleAddress, amount.BigInt()) + s.Require().NoError(err) + + erc20Keeper := s.network.App.GetErc20Keeper() + bankKeeper := s.network.App.GetBankKeeper() + + ctx, _ := s.network.GetContext().CacheContext() + + // Give the sender the matching coins, as a successful ERC20 -> Coin leg would have. + coins := sdk.Coins{sdk.Coin{Denom: denom, Amount: amount}} + s.Require().NoError(bankKeeper.MintCoins(ctx, types.ModuleName, coins)) + s.Require().NoError(bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, sender, coins)) + + escrowBefore := erc20Keeper.BalanceOf(ctx, erc20ABI, contractAddr, types.ModuleAddress) + s.Require().NotNil(escrowBefore) + s.Require().Zero(escrowBefore.Cmp(amount.BigInt()), "escrow should hold the minted tokens") + receiverTokensBefore := erc20Keeper.BalanceOf(ctx, erc20ABI, contractAddr, receiverHex) + s.Require().NotNil(receiverTokensBefore) + supplyBefore := bankKeeper.GetSupply(ctx, denom) + + // baseapp runs every message on its own branch and drops it when the message errors; + // mirror that here so the assertions below see what the chain would have kept. + msgCtx, _ := ctx.CacheContext() + convertMsg := types.NewMsgConvertCoin(coins[0], receiverHex, sender) + _, err = erc20Keeper.ConvertCoin(msgCtx, convertMsg) + s.Require().Error(err) + s.Require().ErrorContains(err, "unexpected Approval event") + + // Invariant: escrow untouched, receiver got nothing, coins not burned. + escrowAfter := erc20Keeper.BalanceOf(ctx, erc20ABI, contractAddr, types.ModuleAddress) + s.Require().NotNil(escrowAfter) + s.Require().Zero(escrowBefore.Cmp(escrowAfter), "escrow moved: before %s, after %s", escrowBefore, escrowAfter) + + receiverTokensAfter := erc20Keeper.BalanceOf(ctx, erc20ABI, contractAddr, receiverHex) + s.Require().NotNil(receiverTokensAfter) + s.Require().Zero(receiverTokensBefore.Cmp(receiverTokensAfter), "receiver tokens moved: before %s, after %s", receiverTokensBefore, receiverTokensAfter) + + s.Require().Equal(supplyBefore, bankKeeper.GetSupply(ctx, denom), "coins were burned without releasing tokens") +} + func (s *KeeperTestSuite) TestConvertNativeERC20ToEVMERC20() { var ( contractAddr common.Address diff --git a/tests/integration/x/feemarket/test_abci.go b/tests/integration/x/feemarket/test_abci.go index df7a0f5eb..1c15af3e3 100644 --- a/tests/integration/x/feemarket/test_abci.go +++ b/tests/integration/x/feemarket/test_abci.go @@ -1,6 +1,8 @@ package feemarket import ( + "math" + "github.com/cosmos/evm/testutil/integration/evm/network" storetypes "cosmossdk.io/store/types" @@ -59,3 +61,72 @@ func (s *KeeperTestSuite) TestEndBlock() { }) } } + +// TestEndBlockGasWantedOverflowIsClamped covers F-2026-18144. +// +// The cumulative gas wanted is a uint64 sum of per-transaction declarations. +// Under an unbounded block gas limit (max_gas: -1) nothing caps a single +// declaration, so two transactions declaring MaxInt64 sum into +// (MaxInt64, MaxUint64] - a perfectly valid uint64 that cannot be converted +// back to int64. EndBlock used to return an error for that, and an error out +// of EndBlock surfaces through FinalizeBlock after the block has been decided, +// stopping the chain. It must clamp instead. +func (s *KeeperTestSuite) TestEndBlockGasWantedOverflowIsClamped() { + testCases := []struct { + name string + gasWanted uint64 + expGasWanted uint64 + }{ + { + // 2 * MaxInt64, the two-transaction shape from the finding. + "two MaxInt64 declarations", + uint64(math.MaxInt64)*2 - 1, + // clamped to MaxInt64, then * MinGasMultiplier (0.5) + uint64(math.MaxInt64) / 2, + }, + { + "MaxUint64", + uint64(math.MaxUint64), + uint64(math.MaxInt64) / 2, + }, + { + // Exactly MaxInt64 is representable and must be left alone. + "MaxInt64 is not clamped", + uint64(math.MaxInt64), + uint64(math.MaxInt64) / 2, + }, + } + + for _, tc := range testCases { + s.Run(tc.name, func() { + nw := network.NewUnitTestNetwork(s.create, s.options...) + ctx := nw.GetContext().WithBlockGasMeter(storetypes.NewInfiniteGasMeter()) + + nw.App.GetFeeMarketKeeper().SetTransientBlockGasWanted(ctx, tc.gasWanted) + + err := nw.App.GetFeeMarketKeeper().EndBlock(ctx) + + gasWanted := nw.App.GetFeeMarketKeeper().GetBlockGasWanted(ctx) + s.Equal(tc.expGasWanted, gasWanted, tc.name) + s.NoError(err, "EndBlock must not fail the block on an oversized cumulative gas wanted") + }) + } +} + +// TestAddTransientGasWantedSaturates: the accumulation itself must not wrap. +// A wrapped total would silently under-report the block's gas wanted to the +// base fee calculation. +func (s *KeeperTestSuite) TestAddTransientGasWantedSaturates() { + nw := network.NewUnitTestNetwork(s.create, s.options...) + ctx := nw.GetContext() + k := nw.App.GetFeeMarketKeeper() + + total, err := k.AddTransientGasWanted(ctx, math.MaxUint64) + s.Require().NoError(err) + s.Require().Equal(uint64(math.MaxUint64), total) + + total, err = k.AddTransientGasWanted(ctx, 1) + s.Require().NoError(err) + s.Equal(uint64(math.MaxUint64), total, "the cumulative gas wanted must saturate, not wrap to 0") + s.Equal(uint64(math.MaxUint64), k.GetTransientGasWanted(ctx)) +} diff --git a/tests/integration/x/vm/test_call_evm.go b/tests/integration/x/vm/test_call_evm.go index 359d8d0dc..d4859a099 100644 --- a/tests/integration/x/vm/test_call_evm.go +++ b/tests/integration/x/vm/test_call_evm.go @@ -12,6 +12,8 @@ import ( "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" ) @@ -241,3 +243,62 @@ 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 config.DefaultGasCap, which a +// caller-supplied gasCap can only narrow (F-2026-18818), 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) + // v0.6.0: the caller supplies the StateDB. Build it after deployRawCode so it + // sees the burner's code. + stateDB := statedb.New(ctx, keeper, statedb.NewEmptyTxConfig()) + gasBefore := ctx.GasMeter().GasConsumed() + + res, err := keeper.CallEVMWithData(ctx, stateDB, from, &burner, nil, true, false, 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) + // v0.6.0: the caller supplies the StateDB. Build it after deployRawCode so it + // sees the burner's code. + stateDB := statedb.New(ctx, keeper, statedb.NewEmptyTxConfig()) + gasBefore := ctx.GasMeter().GasConsumed() + + res, err := keeper.CallEVMWithData(ctx, stateDB, from, &burner, nil, true, false, 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_call_evm_gascap.go b/tests/integration/x/vm/test_call_evm_gascap.go new file mode 100644 index 000000000..2141e8359 --- /dev/null +++ b/tests/integration/x/vm/test_call_evm_gascap.go @@ -0,0 +1,89 @@ +package vm + +import ( + "math/big" + + "github.com/ethereum/go-ethereum/common" + + "github.com/cosmos/evm/contracts" + "github.com/cosmos/evm/server/config" + testconstants "github.com/cosmos/evm/testutil/constants" + utiltx "github.com/cosmos/evm/testutil/tx" + "github.com/cosmos/evm/x/erc20/types" + "github.com/cosmos/evm/x/vm/statedb" +) + +// TestCallEVMWithDataHonoursGasCap is the guard for F-2026-18818. +// +// CallEVMWithData accepted a gasCap and then hardcoded GasLimit to +// config.DefaultGasCap, so the parameter did nothing. Callers passing one - the +// IBC callback keeper passes its remaining Cosmos gas, intending to sandbox the +// EVM work - got an EVM message that could burn up to 25M gas regardless. +// +// The assertion that catches this is the middle case: a cap below what the call +// costs has to actually stop the call. Asserting only that a large cap succeeds +// would pass just as happily against the ignored parameter. +func (s *KeeperTestSuite) TestCallEVMWithDataHonoursGasCap() { + erc20 := contracts.ERC20MinterBurnerDecimalsContract.ABI + contractAddr := common.HexToAddress(testconstants.WEVMOSContractMainnet) + + data, err := erc20.Pack("balanceOf", utiltx.GenerateAddress()) + s.Require().NoError(err) + + callWithCap := func(gasCap *big.Int) (uint64, error) { + s.SetupTest() + ctx := s.Network.GetContext() + // v0.6.0: the caller supplies the StateDB. + stateDB := statedb.New(ctx, s.Network.App.GetEVMKeeper(), statedb.NewEmptyTxConfig()) + res, err := s.Network.App.GetEVMKeeper().CallEVMWithData( + ctx, stateDB, types.ModuleAddress, &contractAddr, data, false, false, gasCap, + ) + if res == nil { + return 0, err + } + return res.GasUsed, err + } + + // Baseline: no cap. Establishes what the call actually costs, so the + // too-small case below is derived rather than guessed. + baseline, err := callWithCap(nil) + s.Require().NoError(err, "an uncapped call must still work") + s.Require().Positive(baseline) + + // A cap below the call's own cost must fail it. This is the case that fails + // when gasCap is ignored. + _, err = callWithCap(new(big.Int).SetUint64(baseline - 1)) + s.Require().Error(err, "a gasCap below the call's cost must stop the call") + + // A modest cap that comfortably covers the call succeeds, while staying two + // orders of magnitude below DefaultGasCap - so success here is the supplied + // cap being honoured, not the default ceiling doing the work. + // + // Note GasUsed is not itself a viable limit. It is the net, post-refund + // figure (~24k for this call), whereas the execution needs materially more + // than that to be *given* up front - a cap of 2x GasUsed still reverts. Do + // not "tighten" this to baseline or a small multiple of it. + room := new(big.Int).SetUint64(baseline * 10) + used, err := callWithCap(room) + s.Require().NoError(err, "a gasCap with headroom over the call's cost must succeed") + s.Require().Less(room.Uint64(), config.DefaultGasCap, + "the headroom cap must stay below DefaultGasCap or this proves nothing") + s.Require().Positive(used) + + // DefaultGasCap remains the ceiling: a caller may narrow the limit, never + // widen it. + huge := new(big.Int).Mul(new(big.Int).SetUint64(config.DefaultGasCap), big.NewInt(10)) + used, err = callWithCap(huge) + s.Require().NoError(err) + s.Require().LessOrEqual(used, config.DefaultGasCap, + "a gasCap above DefaultGasCap must not raise the EVM gas limit") + + // A non-positive or absurdly wide cap falls back to the default ceiling + // rather than truncating to something meaningless. + _, err = callWithCap(big.NewInt(0)) + s.Require().NoError(err, "a zero gasCap must fall back to DefaultGasCap, not to a zero limit") + + overUint64 := new(big.Int).Lsh(big.NewInt(1), 70) + _, err = callWithCap(overUint64) + s.Require().NoError(err, "a gasCap wider than uint64 must fall back to DefaultGasCap") +} diff --git a/tests/integration/x/vm/test_derived_call.go b/tests/integration/x/vm/test_derived_call.go index b1d73f80a..7580a54c4 100644 --- a/tests/integration/x/vm/test_derived_call.go +++ b/tests/integration/x/vm/test_derived_call.go @@ -1,18 +1,26 @@ package vm import ( + "math" "math/big" "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 +197,383 @@ 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} + +// stopCode is a single STOP opcode: the cheapest possible successful execution, +// costing exactly the 21,000 intrinsic gas of the call itself. It gives the gas +// estimator something it can predict exactly. +var stopCode = []byte{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") +} + +// --------------------------------------------------------------------------- +// F-2026-18182: a gas limit that does not fit in uint64 must be rejected, not +// silently truncated to its low 64 bits. +// --------------------------------------------------------------------------- + +// TestDerivedEVMCallWithDataRejectsNonUint64GasLimit is the regression test for +// F-2026-18182. big.Int.Uint64 is undefined for values that do not fit and in +// practice returns the low 64 bits, so before the IsUint64 guard a caller-supplied +// gasLimit at or above 2^64 was silently reinterpreted as a much smaller number. +// Node-side validation bounds UniversalPayload.GasLimit to uint256, so every value +// exercised here genuinely reaches this call. +// +// The third case is the one that makes the bug visible rather than merely wrong: +// 2^64 + 5_000_000 truncates to exactly 5_000_000 gas, which is enough for the +// burner to run to completion — so without the guard the call would report a clean +// success against a budget the caller never asked for. The first two truncate to 0 +// and 5, which fail intrinsic-gas checks and so surface as a confusing out-of-gas +// error instead of a clear rejection. Both shapes are wrong; asserting on the error +// text plus the untouched parent meter distinguishes the fix from either of them. +func (s *KeeperTestSuite) TestDerivedEVMCallWithDataRejectsNonUint64GasLimit() { + twoPow64 := new(big.Int).Lsh(big.NewInt(1), 64) + + testCases := []struct { + name string + gasLimit *big.Int + truncatesTo string + }{ + { + name: "2^64", + gasLimit: new(big.Int).Set(twoPow64), + truncatesTo: "0", + }, + { + name: "2^64 + 5", + gasLimit: new(big.Int).Add(twoPow64, big.NewInt(5)), + truncatesTo: "5", + }, + { + name: "2^64 + 5_000_000 (truncation would execute successfully)", + gasLimit: new(big.Int).Add(twoPow64, big.NewInt(5_000_000)), + truncatesTo: "5000000", + }, + { + name: "2^256 - 1 (the largest value node-side validation admits)", + gasLimit: new(big.Int).Sub(new(big.Int).Lsh(big.NewInt(1), 256), big.NewInt(1)), + truncatesTo: "18446744073709551615", + }, + } + + for _, tc := range testCases { + s.Run(tc.name, func() { + s.SetupTest() + + from := s.Keyring.GetAddr(0) + ctx := s.Network.GetContext().WithGasMeter(storetypes.NewGasMeter(parentGasMeterLimit)) + keeper := s.Network.App.GetEVMKeeper() + + // The burner needs ~2.2M gas: reachable under the truncated budget of + // the 5_000_000 case, unreachable under a correct rejection. + burner := s.deployRawCode(ctx, gasBurnerSuccessCode) + gasBefore := ctx.GasMeter().GasConsumed() + + res, err := keeper.DerivedEVMCallWithData( + ctx, from, &burner, nil, + true, // commit + false, // gasless + false, // isModuleSender + big.NewInt(0), tc.gasLimit, nil, + ) + + // Assert the *shape* of the outcome before the error text: under a + // truncation regression the 5_000_000 case returns (res, nil) and every + // error-flavoured assertion would be vacuous, so the no-execution checks + // have to come first to be the ones that actually fail. + charged := ctx.GasMeter().GasConsumed() - gasBefore + s.Require().Less(charged, uint64(100_000), + "a rejected gas limit must not run the EVM; truncating to %s would have burned ~2.2M", + tc.truncatesTo) + s.Require().Nil(res, + "a rejected gas limit must not produce an execution result") + + s.Require().Error(err, "a gas limit above uint64 must be rejected outright") + s.Require().ErrorContains(err, "does not fit in uint64", + "the rejection must name the problem rather than surface as out-of-gas") + s.Require().ErrorIs(err, evmtypes.ErrInvalidGasLimit) + }) + } +} + +// TestDerivedEVMCallWithDataAcceptsMaxUint64GasLimit pins the boundary: MaxUint64 +// exactly does fit in a uint64, so it must be accepted by the guard and then +// clamped to config.DefaultGasCap by the F-2026-18824 clamp — not rejected. The +// INVALID opcode burns the whole frame, so res.GasUsed reports the effective cap +// and shows the clamp took hold. +func (s *KeeperTestSuite) TestDerivedEVMCallWithDataAcceptsMaxUint64GasLimit() { + 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() + + maxUint64 := new(big.Int).SetUint64(math.MaxUint64) + + 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), maxUint64, nil, + ) + }, "MaxUint64 fits in a uint64 and must not panic the parent gas meter once clamped") + + s.Require().Error(err, "the halter halts exceptionally, so the call still surfaces an error") + s.Require().NotErrorIs(err, evmtypes.ErrInvalidGasLimit, + "MaxUint64 fits in uint64 and must not be rejected by the IsUint64 guard") + s.Require().NotNil(res) + s.Require().True(res.Failed()) + s.Require().Equal(config.DefaultGasCap, res.GasUsed, + "MaxUint64 must be clamped to DefaultGasCap, not accepted as-is") + + s.Require().LessOrEqual(ctx.GasMeter().GasConsumed()-gasBefore, config.DefaultGasCap+1_000_000, + "the parent meter must never be charged beyond the clamped cap") +} + +// TestDerivedEVMCallWithDataNormalGasLimitUnaffected pins ordinary traffic against +// the new guard. 610,477 is the gasUsed observed on donut for a real UEA payload +// execution, ~40x below the 25M DefaultGasCap; it fits in a uint64, so the guard +// must be invisible to it. +func (s *KeeperTestSuite) TestDerivedEVMCallWithDataNormalGasLimitUnaffected() { + s.SetupTest() + + erc20ABI := contracts.ERC20MinterBurnerDecimalsContract.ABI + wevmos := common.HexToAddress(testconstants.WEVMOSContractMainnet) + from := s.Keyring.GetAddr(0) + ctx := s.Network.GetContext().WithGasMeter(storetypes.NewGasMeter(parentGasMeterLimit)) + keeper := s.Network.App.GetEVMKeeper() + + data, err := erc20ABI.Pack("balanceOf", from) + s.Require().NoError(err) + + res, err := keeper.DerivedEVMCallWithData( + ctx, from, &wevmos, data, + true, false, false, + big.NewInt(0), big.NewInt(610_477), nil, + ) + s.Require().NoError(err, "a real-world gas limit must be unaffected by the uint64 guard") + s.Require().False(res.Failed()) + s.Require().Greater(res.GasUsed, uint64(0)) + s.Require().Less(res.GasUsed, uint64(610_477), + "the call must run inside the declared budget, not against a truncated one") +} + +// TestDerivedEVMCallWithDataNilGasLimitUsesEstimation pins the nil path: the guard +// lives inside the `gasLimit != nil` branch, so a nil limit must still fall through +// to EstimateGasInternal untouched — and must certainly not be dereferenced. +func (s *KeeperTestSuite) TestDerivedEVMCallWithDataNilGasLimitUsesEstimation() { + s.SetupTest() + + from := s.Keyring.GetAddr(0) + // Deliberately the plain context: gas estimation binary-searches the EVM and + // this test is about the nil branch, not about metering. + ctx := s.Network.GetContext() + keeper := s.Network.App.GetEVMKeeper() + + // A STOP-only contract is the one target whose cost the estimator can predict + // exactly. The estimator is documented to underpredict on richer targets — a + // pre-existing property of this path that the guard neither causes nor fixes. + target := s.deployRawCode(ctx, stopCode) + + var res *evmtypes.MsgEthereumTxResponse + var err error + s.Require().NotPanics(func() { + res, err = keeper.DerivedEVMCallWithData( + ctx, from, &target, nil, + true, // commit — the estimation branch only runs when committing + false, // gasless + false, // isModuleSender + big.NewInt(0), nil, nil, + ) + }, "a nil gasLimit must never reach the IsUint64 guard") + + s.Require().NoError(err) + s.Require().NotErrorIs(err, evmtypes.ErrInvalidGasLimit, + "the guard lives inside the gasLimit != nil branch and must not fire on nil") + s.Require().NotNil(res) + s.Require().False(res.Failed()) + s.Require().Greater(res.GasUsed, uint64(0), + "the estimation path must still produce a workable budget") +} diff --git a/tests/integration/x/vm/test_msg_server.go b/tests/integration/x/vm/test_msg_server.go index 8f776f9c3..b5f612b36 100644 --- a/tests/integration/x/vm/test_msg_server.go +++ b/tests/integration/x/vm/test_msg_server.go @@ -12,6 +12,7 @@ import ( sdkmath "cosmossdk.io/math" sdktypes "github.com/cosmos/cosmos-sdk/types" + errortypes "github.com/cosmos/cosmos-sdk/types/errors" authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" ) @@ -260,3 +261,104 @@ func (s *KeeperTestSuite) TestRegisterPreinstalls() { s.Require().NoError(err) } } + +// TestEthereumTxSenderVerification is the regression guard for F-2026-18197 +// (nested message dispatch bypasses the EVM ante for MsgEthereumTx). +// +// The EVM ante handler only runs over the top-level messages of a tx. A module +// that unpacks and re-dispatches an embedded sdk.Msg (x/authz MsgExec, x/group +// MsgSubmitProposal/MsgExec, x/gov proposals, a CosmWasm stargate/Any message, +// ICA) hands the nested message to this msg server with the ante already +// behind it, so the signature was never checked. Keeper.EthereumTx therefore +// verifies the sender itself: the declared From must be the ECDSA signer of the +// raw transaction, whatever route the message took to get here. +func (s *KeeperTestSuite) TestEthereumTxSenderVerification() { + testCases := []struct { + name string + mutate func(msg *types.MsgEthereumTx) + expErr bool + }{ + { + // The ante already ran VerifySender for this path, so the keeper + // check is a redundant no-op and normal traffic is unaffected. + name: "pass - untouched victim-signed tx (normal JSON-RPC path)", + mutate: func(*types.MsgEthereumTx) {}, + expErr: false, + }, + { + // The attack: take a victim-signed tx off the mempool and push it + // through a nested dispatcher, declaring the attacker (or a group + // policy / authz grantee account) as the sender so the outer + // permission check passes. Recovery still yields the victim. + name: "fail - From spoofed to a different account", + mutate: func(msg *types.MsgEthereumTx) { + msg.From = s.Keyring.GetAddr(1).Bytes() + }, + expErr: true, + }, + { + // A module account is derived from a name, never from a key pair, + // so no signature can ever recover to it. This is what makes it a + // design invariant that module-driven EVM calls go through + // ApplyMessage* directly and never through this msg server. + name: "fail - From spoofed to a module account", + mutate: func(msg *types.MsgEthereumTx) { + msg.From = authtypes.NewModuleAddress(govtypes.ModuleName).Bytes() + }, + expErr: true, + }, + { + name: "fail - From cleared", + mutate: func(msg *types.MsgEthereumTx) { + msg.From = nil + }, + expErr: true, + }, + } + + s.Run("fail - empty raw transaction", func() { + s.SetupTest() + // Raw unmarshals to a nil transaction when the field is empty, and + // ValidateBasic lives in the same ante path a nested message skips. + _, err := s.Network.App.GetEVMKeeper().EthereumTx(s.Network.GetContext(), &types.MsgEthereumTx{}) + s.Require().Error(err) + s.Require().ErrorIs(err, errortypes.ErrInvalidRequest) + }) + + for _, tc := range testCases { + s.Run(tc.name, func() { + s.SetupTest() + + victim := s.Keyring.GetKey(0) + recipient := s.Keyring.GetAddr(1) + tx, err := s.Factory.GenerateSignedEthTx(victim.Priv, types.EvmTxArgs{ + To: &recipient, + Amount: big.NewInt(1e10), + }) + s.Require().NoError(err) + + msg := tx.GetMsgs()[0].(*types.MsgEthereumTx) + s.Require().Equal(victim.Addr.Bytes(), msg.From, "sanity: tx must be signed by the victim") + tc.mutate(msg) + + ctx := s.Network.GetContext() + res, err := s.Network.App.GetEVMKeeper().EthereumTx(ctx, msg) + + if !tc.expErr { + s.Require().NoError(err) + s.Require().False(res.Failed()) + return + } + + s.Require().Error(err) + s.Require().ErrorIs(err, errortypes.ErrorInvalidSigner) + s.Require().Contains(err.Error(), "signature verification failed") + // The transaction must not have been applied at all. + s.Require().Nil(res) + s.Require().False( + utils.ContainsEventType(ctx.EventManager().Events().ToABCIEvents(), types.EventTypeEthereumTx), + "a rejected tx must not emit an ethereum_tx event", + ) + }) + } +} diff --git a/x/erc20/keeper/convert.go b/x/erc20/keeper/convert.go index aa06cf9e8..e3e234ade 100644 --- a/x/erc20/keeper/convert.go +++ b/x/erc20/keeper/convert.go @@ -90,6 +90,16 @@ func (k Keeper) ConvertERC20IntoCoinsForNativeToken(ctx sdk.Context, stateDB *st } } + // Check for unexpected `Approval` event in logs (F-2026-18822). + // v0.6.0 lifted this body out of msg_server.convertERC20IntoCoinsForNativeToken + // into this shared helper and dropped the guard on the way, even though the + // doc comment above still promises it. Re-applied here, in the same position + // it held before the move, so both the msg server and the ICS20 precompile + // path are covered. + if err := validateApprovalEventDoesNotExist(res.Logs); err != nil { + return nil, err + } + // Check expected escrow balance after transfer execution coins := sdk.Coins{sdk.Coin{Denom: pair.Denom, Amount: amount}} tokens := coins[0].Amount.BigInt() diff --git a/x/erc20/keeper/ibc_callbacks.go b/x/erc20/keeper/ibc_callbacks.go index 3b30a557b..af8429a2d 100644 --- a/x/erc20/keeper/ibc_callbacks.go +++ b/x/erc20/keeper/ibc_callbacks.go @@ -159,8 +159,12 @@ func (k Keeper) OnRecvPacket( // is refunded and then the IBC Coins are converted to ERC20. // If the ERC20 conversion fails for whatever reason, such as an attempt to call // a self-destructed ERC20 contract or an invalid function, OnAcknowledgementPacket -// still succeeds, but the user receives the corresponding bank token from the -// TokenPair instead. A user may then manually re-attempt the conversion. +// still succeeds and the user keeps the corresponding bank token from the +// TokenPair, so they may manually re-attempt the conversion. That holds only +// because ConvertCoinNativeERC20 is atomic: it escrows the coins onto the module +// account before touching the EVM, so without the rollback a VM failure would +// leave the refund stranded on the module account rather than with the sender +// (F-2026-18819). func (k Keeper) OnAcknowledgementPacket( ctx sdk.Context, _ channeltypes.Packet, data transfertypes.FungibleTokenPacketData, @@ -181,8 +185,9 @@ func (k Keeper) OnAcknowledgementPacket( // since the original packet sent was never received and has been timed out. // If the ERC20 conversion fails for whatever reason, such as an attempt to call // a self-destructed ERC20 contract or an invalid function, OnTimeoutPacket still -// succeeds, but the user receives the corresponding bank token from the TokenPair -// instead. A user may then manually re-attempt the conversion. +// succeeds and the user keeps the corresponding bank token from the TokenPair, +// so they may manually re-attempt the conversion. As with OnAcknowledgementPacket +// that depends on ConvertCoinNativeERC20 being atomic (F-2026-18819). func (k Keeper) OnTimeoutPacket(ctx sdk.Context, _ channeltypes.Packet, data transfertypes.FungibleTokenPacketData) error { return k.ConvertCoinToERC20FromPacket(ctx, data) } @@ -235,6 +240,12 @@ func (k Keeper) ConvertCoinToERC20FromPacket(ctx sdk.Context, data transfertypes // Convert from Coin to ERC20 if err := k.ConvertCoinNativeERC20(ctx, pair, coin.Amount, common.BytesToAddress(sender), sender, false); err != nil { + // Swallowing the error is deliberate: the refund itself already + // succeeded and must not be undone by a failure to re-wrap it into + // the ERC20 representation. This is only safe because + // ConvertCoinNativeERC20 rolls back on failure, so the sender still + // holds the bank coins here (F-2026-18819). + // // We want to record only the failed attempt to reconvert the coins during IBC. defer func() { telemetry.IncrCounter(1, types.ModuleName, "ibc", "error", "total") diff --git a/x/erc20/keeper/msg_server.go b/x/erc20/keeper/msg_server.go index 79e029260..8dc07170e 100644 --- a/x/erc20/keeper/msg_server.go +++ b/x/erc20/keeper/msg_server.go @@ -83,13 +83,68 @@ func (k Keeper) ConvertCoin( } // ConvertCoinNativeERC20 handles the coin conversion for a native ERC20 token -// pair: +// pair. It runs the conversion atomically: the bank escrow, the EVM transfer and +// the burn either all commit or none of them do. +// +// The atomicity is load-bearing, not cosmetic. The body escrows coins onto the +// module account before calling the EVM, and the EVM call isolates only the EVM +// execution — so on a VM failure the bank escrow used to survive, stranding the +// sender's coins on the erc20 module account with no user-facing way to reclaim +// them. The IBC refund path (see ConvertCoinToERC20FromPacket) swallows this +// error and commits the block, which made that stranding permanent +// (F-2026-18819). +// +// As of v0.6.0 this branch is the ONLY rollback boundary around the escrow. +// CallEVMWithData no longer wraps ApplyMessage in its own ctx.CacheContext(): the +// caller supplies the StateDB and ApplyMessageWithConfig commits it whenever +// commit is true, reverted frames included (it relies on the EVM's own +// RevertToSnapshot having already unwound them). Do not remove the CacheContext +// below on the grounds that "the EVM call rolls itself back" — the bank escrow is +// a plain SDK store write and was never part of what the EVM rolls back. +func (k Keeper) ConvertCoinNativeERC20( + ctx sdk.Context, + pair types.TokenPair, + amount math.Int, + receiver common.Address, + sender sdk.AccAddress, + callFromPrecompile bool, +) error { + // CacheContext branches the multistore and installs a fresh EventManager; + // writeCache replays those events onto the parent and commits the branch. + // Nothing is written unless the whole conversion succeeded. + // + // Note the gas meter is *shared* with the parent, so work done on a + // discarded branch is still charged. That is intended — a failed conversion + // should not be free. + cacheCtx, writeCache := ctx.CacheContext() + + if err := k.convertCoinNativeERC20(cacheCtx, pair, amount, receiver, sender, callFromPrecompile); err != nil { + // Branch discarded: the escrow, the EVM state and any events go with it. + return err + } + + writeCache() + + return nil +} + +// convertCoinNativeERC20 is the non-atomic body of ConvertCoinNativeERC20: // - escrow Coins on module account // - unescrow Tokens that have been previously escrowed with ConvertERC20 and send to receiver // - burn escrowed Coins // - check if token balance increased by amount // - check for unexpected `Approval` event in logs -func (k Keeper) ConvertCoinNativeERC20(ctx sdk.Context, pair types.TokenPair, amount math.Int, receiver common.Address, sender sdk.AccAddress, callFromPrecompile bool) error { +// +// It mutates ctx directly and must only be called on a branched context — see +// ConvertCoinNativeERC20. +func (k Keeper) convertCoinNativeERC20( + ctx sdk.Context, + pair types.TokenPair, + amount math.Int, + receiver common.Address, + sender sdk.AccAddress, + callFromPrecompile bool, +) error { if !amount.IsPositive() { return sdkerrors.Wrap(types.ErrNegativeToken, "converted coin amount must be positive") } @@ -131,6 +186,11 @@ func (k Keeper) ConvertCoinNativeERC20(ctx sdk.Context, pair types.TokenPair, am } } + // Check for unexpected `Approval` event in logs + if err := validateApprovalEventDoesNotExist(res.Logs); err != nil { + return err + } + // Check expected Receiver balance after transfer execution balanceTokenAfter := k.BalanceOf(ctx, erc20, contract, receiver) if balanceTokenAfter == nil { diff --git a/x/erc20/keeper/util.go b/x/erc20/keeper/util.go index 97e2fe81e..b25a5268b 100644 --- a/x/erc20/keeper/util.go +++ b/x/erc20/keeper/util.go @@ -13,6 +13,10 @@ import ( // an unexpected `Approval` event func validateApprovalEventDoesNotExist(logs []*types.Log) error { for _, log := range logs { + // anonymous events (LOG0) carry no topics + if len(log.Topics) == 0 { + continue + } if log.Topics[0] == logApprovalSigHash.Hex() { return errors.Wrapf( types2.ErrUnexpectedEvent, "unexpected Approval event", diff --git a/x/erc20/keeper/util_test.go b/x/erc20/keeper/util_test.go index d5f59ffc2..8262f7d73 100644 --- a/x/erc20/keeper/util_test.go +++ b/x/erc20/keeper/util_test.go @@ -33,6 +33,17 @@ func TestValidateApprovalEventDoesNotExist(t *testing.T) { }, expectError: false, }, + { + name: "anonymous log without topics", + res: &types.MsgEthereumTxResponse{ + Logs: []*types.Log{ + { + Topics: []string{}, + }, + }, + }, + expectError: false, + }, { name: "has approval event", res: &types.MsgEthereumTxResponse{ diff --git a/x/feemarket/keeper/abci.go b/x/feemarket/keeper/abci.go index e0ef4de93..01604966d 100644 --- a/x/feemarket/keeper/abci.go +++ b/x/feemarket/keeper/abci.go @@ -58,13 +58,20 @@ func (k *Keeper) EndBlock(ctx sdk.Context) error { gasWanted := math.NewIntFromUint64(k.GetTransientGasWanted(ctx)) gasUsed := math.NewIntFromUint64(ctx.BlockGasMeter().GasConsumedToLimit()) + // Clamp rather than error. Both quantities are uint64 sums that can exceed + // MaxInt64 without any single transaction being individually rejectable, + // and this runs in EndBlock: an error here surfaces through FinalizeBlock + // after the block has already been decided, which stops the chain. A base + // fee computed from a clamped total is a far better outcome than a halt. + // MinGasMultiplier is validated to be <= 1, so MaxInt64 stays convertible + // to uint64 in the calculation below. if !gasWanted.IsInt64() { - k.Logger(ctx).Error("gas wanted exceeds MaxInt64, clamping", "gas_wanted", gasWanted) + k.Logger(ctx).Error("gas wanted exceeds MaxInt64, clamping", "gas_wanted", gasWanted.String()) gasWanted = math.NewInt(gomath.MaxInt64) } if !gasUsed.IsInt64() { - k.Logger(ctx).Error("gas used exceeds MaxInt64, clamping", "gas_used", gasUsed) + k.Logger(ctx).Error("gas used exceeds MaxInt64, clamping", "gas_used", gasUsed.String()) gasUsed = math.NewInt(gomath.MaxInt64) } diff --git a/x/feemarket/keeper/keeper.go b/x/feemarket/keeper/keeper.go index b43b21c14..ee23bef54 100644 --- a/x/feemarket/keeper/keeper.go +++ b/x/feemarket/keeper/keeper.go @@ -1,6 +1,8 @@ package keeper import ( + gomath "math" + "github.com/cosmos/evm/x/feemarket/types" "cosmossdk.io/log" @@ -75,9 +77,15 @@ func (k Keeper) SetTransientBlockGasWanted(ctx sdk.Context, gasWanted uint64) { store.Set(types.KeyPrefixTransientBlockGasWanted, gasBz) } -// AddTransientGasWanted adds the cumulative gas wanted in the transient store +// AddTransientGasWanted adds the cumulative gas wanted in the transient store. +// The sum saturates instead of wrapping: a wrapped total would silently +// under-report the block's gas wanted to the base fee calculation. func (k Keeper) AddTransientGasWanted(ctx sdk.Context, gasWanted uint64) (uint64, error) { - result := k.GetTransientGasWanted(ctx) + gasWanted + current := k.GetTransientGasWanted(ctx) + result := current + gasWanted + if result < current { + result = gomath.MaxUint64 + } k.SetTransientBlockGasWanted(ctx, result) return result, nil } diff --git a/x/ibc/callbacks/keeper/keeper.go b/x/ibc/callbacks/keeper/keeper.go index eee3e5d3c..c9fe36353 100644 --- a/x/ibc/callbacks/keeper/keeper.go +++ b/x/ibc/callbacks/keeper/keeper.go @@ -425,14 +425,19 @@ func (k ContractKeeper) IBCOnTimeoutPacketCallback( return err } - res, err := k.evmKeeper.CallEVM(ctx, stateDB, *abi, sender, contractAddr, true, false, math.NewIntFromUint64(cachedCtx.GasMeter().GasRemaining()).BigInt(), "onPacketTimeout", + // NOTE: use the cached ctx for the EVM calls, as the acknowledgement path + // does. Running on the parent ctx committed the callback's EVM state + // regardless of the outcome and made writeFn() below a no-op, while the gas + // cap was still being read off cachedCtx (F-2026-18818). The StateDB above is + // likewise branched off cachedCtx, so the two must agree. + res, err := k.evmKeeper.CallEVM(cachedCtx, stateDB, *abi, sender, contractAddr, true, false, math.NewIntFromUint64(cachedCtx.GasMeter().GasRemaining()).BigInt(), "onPacketTimeout", packet.GetSourceChannel(), packet.GetSourcePort(), packet.GetSequence(), packet.GetData()) if err != nil { return errorsmod.Wrapf(types.ErrCallbackFailed, "EVM returned error: %s", err.Error()) } // Consume the actual gas used on the original callback context. - ctx.GasMeter().ConsumeGas(res.GasUsed, "callback onPacketAcknowledgement") + ctx.GasMeter().ConsumeGas(res.GasUsed, "callback onPacketTimeout") if ctx.GasMeter().IsOutOfGas() { return errorsmod.Wrapf(types.ErrCallbackFailed, "out of gas") } diff --git a/x/vm/keeper/call_evm.go b/x/vm/keeper/call_evm.go index b3de0c72e..d3cec2364 100644 --- a/x/vm/keeper/call_evm.go +++ b/x/vm/keeper/call_evm.go @@ -2,6 +2,7 @@ package keeper import ( "encoding/json" + "math" "math/big" "strconv" @@ -49,12 +50,30 @@ func (k Keeper) CallEVMWithData(ctx sdk.Context, stateDB *statedb.StateDB, from return nil, err } + // Honour the caller's gasCap. Callers that pass one (the IBC callback keeper + // passes the remaining Cosmos gas) are trying to sandbox the EVM work; before + // F-2026-18818 this parameter was accepted and silently ignored, so the EVM + // always ran to DefaultGasCap and could burn far more gas than the caller had + // budgeted. DefaultGasCap stays the ceiling, so a caller can only ever narrow + // the limit, never widen it. Mirrors DerivedEVMCallWithData, and matches the + // shape upstream cosmos/evm settled on. + gasLimit := config.DefaultGasCap + if gasCap != nil && gasCap.Sign() > 0 { + // A gasCap wider than uint64 cannot be a meaningful limit; fall back to + // the default ceiling rather than truncating it. + if gasCap.BitLen() <= 64 { + if provided := gasCap.Uint64(); provided < gasLimit { + gasLimit = provided + } + } + } + msg := core.Message{ From: from, To: contract, Nonce: nonce, Value: big.NewInt(0), - GasLimit: config.DefaultGasCap, + GasLimit: gasLimit, GasPrice: big.NewInt(0), GasTipCap: big.NewInt(0), GasFeeCap: big.NewInt(0), @@ -66,18 +85,32 @@ func (k Keeper) CallEVMWithData(ctx sdk.Context, stateDB *statedb.StateDB, from // StateDB (returns ErrNilStateDB). Pass it (and callFromPrecompile) straight // through so that contract — and the precompile snapshot/flush chain — is // preserved. + // + // This replaces the ctx.CacheContext() sandbox the pre-v0.6.0 code wrapped + // around ApplyMessage: state isolation on a revert is now the StateDB's + // journal/snapshot job, not a store branch. Gas accounting is unaffected — + // ApplyMessage never touches ctx.GasMeter(), so the explicit ConsumeGas calls + // below remain the only charge to the parent meter, exactly as before. res, err := k.ApplyMessage(ctx, stateDB, msg, nil, commit, callFromPrecompile, true) if err != nil { return nil, err } if res.Failed() { - // push-chain audit fix: unlike upstream we deliberately do NOT call - // k.ResetGasMeterAndConsumeGas(ctx, ctx.GasMeter().Limit()) here. Consuming - // the full gas limit on a revert can overflow the parent gas meter. We - // surface the VM error and leave the gas consumed during execution in - // place; the EVM has already rolled back the reverted call frame via its - // own snapshot, so no extra state isolation is required. + // A failed execution still burned real EVM work, so charge it to the parent + // Cosmos gas meter exactly like a successful one (F-2026-18824). Skipping + // this made a revert (or a deliberate out-of-gas) a free way to consume + // block compute. msg.GasLimit above is config.DefaultGasCap, which a + // caller-supplied gasCap can only narrow, so res.GasUsed — which equals the + // gas limit on out-of-gas — is bounded by that cap and cannot be inflated by + // the caller. + // + // Note we charge exactly res.GasUsed and, unlike upstream, deliberately do + // NOT call k.ResetGasMeterAndConsumeGas(ctx, ctx.GasMeter().Limit()): + // consuming the full gas limit on a revert can overflow the parent gas + // meter. The EVM has already rolled back the reverted call frame via its own + // snapshot, so no extra state isolation is required. + ctx.GasMeter().ConsumeGas(res.GasUsed, "apply evm message (failed)") return res, errorsmod.Wrap(types.ErrVMExecution, res.VmError) } @@ -119,7 +152,11 @@ func (k Keeper) DerivedEVMCall( resp, err := k.DerivedEVMCallWithData(ctx, from, &contract, data, commit, gasless, isModuleSender, value, gasLimit, manualNonce) if err != nil { - return nil, errorsmod.Wrapf(err, "contract call failed: method '%s', contract '%s'", method, contract) + // Propagate resp alongside the error. On a VM failure (revert / out of gas) + // DerivedEVMCallWithData still returns a response carrying GasUsed; dropping + // it here leaves callers that charge for the gas a call actually burned with + // nothing to charge against. CallEVM above propagates for the same reason. + return resp, errorsmod.Wrapf(err, "contract call failed: method '%s', contract '%s'", method, contract) } return resp, nil } @@ -185,22 +222,44 @@ func (k Keeper) DerivedEVMCallWithData( gasCap = gasRes.Gas } if gasLimit != nil { - gasCap = gasLimit.Uint64() + // Reject a limit that does not fit in uint64 instead of truncating it. + // big.Int.Uint64 is documented as undefined for values that do not fit and + // in practice returns the low 64 bits, so a payload declaring 2^64+5 would + // silently be handed 5 gas and fail with a misleading out-of-gas error. + // Node-side validation bounds UniversalPayload.GasLimit to uint256, so + // values at or above 2^64 do reach this call. IsUint64 also rejects a + // negative limit, for which Uint64 is equally undefined. + if !gasLimit.IsUint64() { + return nil, errorsmod.Wrapf( + types.ErrInvalidGasLimit, + "gas limit %s does not fit in uint64 (max %d)", + gasLimit, uint64(math.MaxUint64), + ) + } + // 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{ - From: from, - To: contract, - Nonce: nonce, - Value: value, - GasLimit: gasCap, - GasFeeCap: big.NewInt(0), - GasTipCap: big.NewInt(0), - GasPrice: big.NewInt(0), - Data: data, - AccessList: ethtypes.AccessList{}, - SkipNonceChecks: !commit, - SkipFromEOACheck: !commit, + From: from, + To: contract, + Nonce: nonce, + Value: value, + GasLimit: gasCap, + GasFeeCap: big.NewInt(0), + GasTipCap: big.NewInt(0), + GasPrice: big.NewInt(0), + Data: data, + AccessList: ethtypes.AccessList{}, + SkipNonceChecks: !commit, + SkipFromEOACheck: !commit, } tx := ethtypes.NewTx(ðtypes.DynamicFeeTx{ Nonce: msg.Nonce, @@ -321,6 +380,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) } diff --git a/x/vm/keeper/msg_server.go b/x/vm/keeper/msg_server.go index 27cebf57a..9cc27684c 100644 --- a/x/vm/keeper/msg_server.go +++ b/x/vm/keeper/msg_server.go @@ -4,10 +4,13 @@ import ( "context" "encoding/hex" "fmt" + "math/big" "strconv" "github.com/hashicorp/go-metrics" + ethtypes "github.com/ethereum/go-ethereum/core/types" + cmttypes "github.com/cometbft/cometbft/types" "github.com/cosmos/evm/x/vm/types" @@ -17,6 +20,7 @@ import ( "github.com/cosmos/cosmos-sdk/telemetry" sdk "github.com/cosmos/cosmos-sdk/types" + errortypes "github.com/cosmos/cosmos-sdk/types/errors" govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" ) @@ -30,6 +34,36 @@ func (k *Keeper) EthereumTx(goCtx context.Context, msg *types.MsgEthereumTx) (*t ctx := sdk.UnwrapSDKContext(goCtx) tx := msg.AsTransaction() + // MsgEthereumTx.Raw unmarshals to a nil transaction when the raw field is + // empty, and ValidateBasic is part of the same ante path that a nested + // message skips, so the executor cannot assume it ran. Fail cleanly instead + // of dereferencing nil below. + if tx == nil { + return nil, errorsmod.Wrap(errortypes.ErrInvalidRequest, "invalid transaction: raw ethereum transaction is empty") + } + + // Verify that msg.From really is the ECDSA signer of the transaction. + // + // This is normally established by the EVM ante handler + // (ante/evm/05_signature_verification.go), but the ante chain only runs over + // the *top-level* messages of a tx. Any module that unpacks an embedded + // sdk.Msg and re-dispatches it through the message router - x/authz MsgExec, + // x/group MsgSubmitProposal/MsgExec, x/gov proposals, a CosmWasm + // stargate/Any message, ICA - delivers the nested message here with the ante + // already behind it, so nothing has checked the signature. An attacker could + // then take any victim-signed transaction off the mempool and have it + // executed as the victim. Checking here means the executor never trusts an + // unverified From, whatever route the message took to reach it. + // + // Cost on the normal path is ~zero: msg.AsTransaction() hands back the same + // *ethtypes.Transaction the ante verified, and go-ethereum caches the + // recovered sender on it per signer, so this is a cache hit rather than a + // second ECDSA recovery. + signer := ethtypes.MakeSigner(types.GetEthChainConfig(), big.NewInt(ctx.BlockHeight()), uint64(ctx.BlockTime().Unix())) //#nosec G115 -- int overflow is not a concern here + if err := msg.VerifySender(signer); err != nil { + return nil, errorsmod.Wrapf(errortypes.ErrorInvalidSigner, "signature verification failed: %s", err.Error()) + } + txIndex := k.GetTxIndexTransient(ctx) labels := []metrics.Label{ diff --git a/x/vm/statedb/journal.go b/x/vm/statedb/journal.go index 7ac0106bb..6323f8cd5 100644 --- a/x/vm/statedb/journal.go +++ b/x/vm/statedb/journal.go @@ -31,15 +31,17 @@ import ( // overrideEventManagerEvents replaces, in place, the events held by em with the // provided events. It mirrors cosmos-sdk v0.53's EventManager.OverrideEvents. // -// NOTE (push-chain): upstream cosmos/evm v0.6.0 calls -// em.OverrideEvents(prevEvents) directly, but that method only exists on the -// cosmos-sdk v0.53 EventManager. push-chain pins cosmos-sdk to the v0.50.x line -// (the rest of v0.6.0 is compatible with it), which has no in-place events -// setter. The precompile-call revert path REQUIRES an in-place mutation — -// precompiles obtain the cacheCtx via GetCacheContext(), which shares the same -// EventManager pointer, so swapping in a fresh EventManager would diverge from -// those outstanding copies. We therefore set the unexported `events` field -// directly, which is exactly what OverrideEvents does on v0.53. +// NOTE (push-chain): upstream cosmos/evm v0.6.0 calls em.OverrideEvents(prevEvents) +// directly, but that method only exists on the cosmos-sdk v0.53 EventManager. +// push-chain-node pins cosmos-sdk to the v0.50.x line (the rest of v0.6.0 is +// compatible with it), which has no in-place events setter. The precompile-call +// revert path REQUIRES an in-place mutation — precompiles obtain the cacheCtx via +// GetCacheContext(), which shares the same EventManager pointer, so swapping in a +// fresh EventManager would diverge from those outstanding copies. We therefore set +// the unexported `events` field directly, which is exactly what OverrideEvents +// does on v0.53. Do NOT replace this with a direct OverrideEvents call until the +// node's cosmos-sdk replace directive moves off v0.50.x — it compiles here but +// breaks the node build. func overrideEventManagerEvents(em sdk.EventManagerI, events sdk.Events) { // The concrete type behind EventManagerI is *sdk.EventManager. f := reflect.ValueOf(em).Elem().FieldByName("events") @@ -203,9 +205,11 @@ func (pc precompileCallChange) Revert(s *StateDB) { // state stored in the snapshot s.RevertMultiStore(pc.snapshot) - // Restore events to the state before this precompile call. - // Equivalent to s.cacheCtx.EventManager().OverrideEvents(pc.prevEvents); see - // overrideEventManagerEvents for why the shim is used. + // Restore events to the state before this precompile call. OverrideEvents + // sets the manager's events in place (em.events = events), which is required + // here: precompiles hold the cacheCtx obtained from GetCacheContext() and + // share this EventManager pointer, so replacing the manager would diverge + // from those outstanding copies. overrideEventManagerEvents(s.cacheCtx.EventManager(), pc.prevEvents) // Restore processed events counter diff --git a/x/vm/statedb/statedb_test.go b/x/vm/statedb/statedb_test.go index 8dc409e20..6f5dde745 100644 --- a/x/vm/statedb/statedb_test.go +++ b/x/vm/statedb/statedb_test.go @@ -249,6 +249,16 @@ func (suite *StateDBTestSuite) TestDBError() { } } +func (suite *StateDBTestSuite) TestSubBalanceUnderflowPanics() { + db := statedb.New(sdk.Context{}.WithEventManager(sdk.NewEventManager()), mocks.NewEVMKeeper(), emptyTxConfig) + db.AddBalance(address, uint256.NewInt(1), tracing.BalanceChangeUnspecified) + + expectedPanic := fmt.Sprintf("state balance underflow for %s: have=%s sub=%s", address.Hex(), "1", "2") + suite.Require().PanicsWithValue(expectedPanic, func() { + db.SubBalance(address, uint256.NewInt(2), tracing.BalanceChangeUnspecified) + }) +} + func (suite *StateDBTestSuite) TestBalance() { testCases := []struct { name string @@ -288,16 +298,6 @@ func (suite *StateDBTestSuite) TestBalance() { } } -func (suite *StateDBTestSuite) TestSubBalanceUnderflowPanics() { - db := statedb.New(sdk.Context{}.WithEventManager(sdk.NewEventManager()), mocks.NewEVMKeeper(), emptyTxConfig) - db.AddBalance(address, uint256.NewInt(1), tracing.BalanceChangeUnspecified) - - expectedPanic := fmt.Sprintf("state balance underflow for %s: have=%s sub=%s", address.Hex(), "1", "2") - suite.Require().PanicsWithValue(expectedPanic, func() { - db.SubBalance(address, uint256.NewInt(2), tracing.BalanceChangeUnspecified) - }) -} - func (suite *StateDBTestSuite) TestState() { key1 := common.BigToHash(big.NewInt(1)) value1 := common.BigToHash(big.NewInt(1))