diff --git a/CHANGELOG.md b/CHANGELOG.md index 8104fb4ae..754d0442b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -60,6 +60,12 @@ 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/tests/integration/x/vm/test_derived_call.go b/tests/integration/x/vm/test_derived_call.go index 5cacb679b..7580a54c4 100644 --- a/tests/integration/x/vm/test_derived_call.go +++ b/tests/integration/x/vm/test_derived_call.go @@ -1,6 +1,7 @@ package vm import ( + "math" "math/big" "strconv" @@ -219,6 +220,11 @@ var gasBurnerRevertCode = []byte{0x60, 0x00, 0x62, 0x10, 0x00, 0x00, 0x52, 0x60, // 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 @@ -377,3 +383,197 @@ func (s *KeeperTestSuite) TestDerivedEVMCallWithDataSuccessChargesGasUsed() { 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/x/vm/keeper/call_evm.go b/x/vm/keeper/call_evm.go index 2e000fac6..20f50d34b 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" @@ -215,6 +216,20 @@ func (k Keeper) DerivedEVMCallWithData( gasCap = gasRes.Gas } if gasLimit != nil { + // 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