From c1af76106328a3eabe5c3b4752a1eda90761c043 Mon Sep 17 00:00:00 2001 From: mmsqe Date: Wed, 17 Jul 2024 09:41:30 +0800 Subject: [PATCH] Problem: no overflow check when add gasLimit to gasWanted --- CHANGELOG.md | 2 +- app/ante/eth.go | 8 ++++--- app/ante/eth_test.go | 51 ++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 55 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 543b02b336..8a5d495bab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,7 +46,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (rpc) [#443](https://github.com/crypto-org-chain/ethermint/pull/443) Keep behavior of random opcode as before. * (app) [#451](https://github.com/crypto-org-chain/ethermint/pull/451) Disable block gas meter, it's not compatible with parallel tx execution. It's safe to do as long as we checks total gas-wanted against block gas limit in process proposal, which we do in default handler. -* (ante) [#493](https://github.com/crypto-org-chain/ethermint/pull/493) Align gasWanted for process proposal mode. +* (ante) [#493](https://github.com/crypto-org-chain/ethermint/pull/493) Align gasWanted for process proposal mode, [#500](https://github.com/crypto-org-chain/ethermint/pull/500) Check for overflow before adding gasLimit to gasWanted. ### Bug Fixes diff --git a/app/ante/eth.go b/app/ante/eth.go index a09caa7d8d..efc8c93e8d 100644 --- a/app/ante/eth.go +++ b/app/ante/eth.go @@ -126,10 +126,12 @@ func CheckEthGasConsume( // We can't trust the tx gas limit, because we'll refund the unused gas. gasLimit := msgEthTx.GetGas() if maxGasWanted != 0 { - gasWanted += min(gasLimit, maxGasWanted) - } else { - gasWanted += gasLimit + gasLimit = min(gasLimit, maxGasWanted) } + if gasWanted > math.MaxInt64-gasLimit { + return ctx, fmt.Errorf("gasWanted(%d) + gasLimit(%d) overflow", gasWanted, gasLimit) + } + gasWanted += gasLimit // user balance is already checked during CheckTx so there's no need to // verify it again during ReCheckTx if ctx.IsReCheckTx() { diff --git a/app/ante/eth_test.go b/app/ante/eth_test.go index 5919dcb76a..8f83d2f9d1 100644 --- a/app/ante/eth_test.go +++ b/app/ante/eth_test.go @@ -1,10 +1,13 @@ package ante_test import ( + "errors" + "fmt" "math" "math/big" sdk "github.com/cosmos/cosmos-sdk/types" + "google.golang.org/protobuf/proto" storetypes "cosmossdk.io/store/types" ethtypes "github.com/ethereum/go-ethereum/core/types" @@ -155,6 +158,18 @@ func (suite *AnteTestSuite) TestEthNonceVerificationDecorator() { } } +type multiTx struct { + Msgs []sdk.Msg +} + +func (msg *multiTx) GetMsgs() []sdk.Msg { + return msg.Msgs +} + +func (msg *multiTx) GetMsgsV2() ([]proto.Message, error) { + return nil, errors.New("not implemented") +} + func (suite *AnteTestSuite) TestEthGasConsumeDecorator() { evmParams := suite.app.EvmKeeper.GetParams(suite.ctx) chainID := suite.app.EvmKeeper.ChainID() @@ -189,6 +204,9 @@ func (suite *AnteTestSuite) TestEthGasConsumeDecorator() { dynamicFeeTx.From = addr.Bytes() dynamicFeeTxPriority := int64(1) + maxGasLimitTx := evmtypes.NewTxContract(suite.app.EvmKeeper.ChainID(), 1, big.NewInt(10), math.MaxUint64, gasPrice, nil, nil, nil, ðtypes.AccessList{{Address: addr, StorageKeys: nil}}) + maxGasLimitTx.From = addr.Bytes() + var vmdb *statedb.StateDB testCases := []struct { @@ -199,8 +217,9 @@ func (suite *AnteTestSuite) TestEthGasConsumeDecorator() { expPass bool expPanic bool expPriority int64 + err error }{ - {"invalid transaction type", &invalidTx{}, math.MaxUint64, func() {}, false, false, 0}, + {"invalid transaction type", &invalidTx{}, math.MaxUint64, func() {}, false, false, 0, nil}, { "sender not found", evmtypes.NewTxContract(suite.app.EvmKeeper.ChainID(), 1, big.NewInt(10), 1000, big.NewInt(1), nil, nil, nil, nil), @@ -208,6 +227,7 @@ func (suite *AnteTestSuite) TestEthGasConsumeDecorator() { func() {}, false, false, 0, + nil, }, { "gas limit too low", @@ -216,6 +236,7 @@ func (suite *AnteTestSuite) TestEthGasConsumeDecorator() { func() {}, false, false, 0, + nil, }, { "gas limit above block gas limit", @@ -224,6 +245,7 @@ func (suite *AnteTestSuite) TestEthGasConsumeDecorator() { func() {}, false, false, 0, + nil, }, { "not enough balance for fees", @@ -232,6 +254,7 @@ func (suite *AnteTestSuite) TestEthGasConsumeDecorator() { func() {}, false, false, 0, + nil, }, { "not enough tx gas", @@ -242,6 +265,7 @@ func (suite *AnteTestSuite) TestEthGasConsumeDecorator() { }, false, true, 0, + nil, }, { "not enough block gas", @@ -253,6 +277,22 @@ func (suite *AnteTestSuite) TestEthGasConsumeDecorator() { }, false, true, 0, + nil, + }, + { + "gas limit overflow", + &multiTx{ + Msgs: []sdk.Msg{maxGasLimitTx, tx2}, + }, + math.MaxUint64, + func() { + limit := new(big.Int).SetUint64(math.MaxUint64) + balance := new(big.Int).Mul(limit, gasPrice) + vmdb.AddBalance(addr, balance) + }, + false, false, + 0, + fmt.Errorf("gasWanted(%d) + gasLimit(%d) overflow", maxGasLimitTx.GetGas(), tx2.GetGas()), }, { "success - legacy tx", @@ -264,6 +304,7 @@ func (suite *AnteTestSuite) TestEthGasConsumeDecorator() { }, true, false, tx2Priority, + nil, }, { "success - dynamic fee tx", @@ -275,6 +316,7 @@ func (suite *AnteTestSuite) TestEthGasConsumeDecorator() { }, true, false, dynamicFeeTxPriority, + nil, }, { "success - gas limit on gasMeter is set on ReCheckTx mode", @@ -286,6 +328,7 @@ func (suite *AnteTestSuite) TestEthGasConsumeDecorator() { }, true, false, 1, + nil, }, } @@ -313,7 +356,11 @@ func (suite *AnteTestSuite) TestEthGasConsumeDecorator() { suite.Require().NoError(err) suite.Require().Equal(tc.expPriority, ctx.Priority()) } else { - suite.Require().Error(err) + if tc.err != nil { + suite.Require().ErrorContains(err, tc.err.Error()) + } else { + suite.Require().Error(err) + } } suite.Require().Equal(tc.gasLimit, ctx.GasMeter().Limit()) })