Problem: no overflow check when add gasLimit to gasWanted

crypto-org-chain · Jul 17, 2024 · c1af761 · c1af761
1 parent 9c959a2
commit c1af761
Show file tree

Hide file tree

Showing 3 changed files with 55 additions and 6 deletions.
diff --git 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
@@ -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
@@ -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, &ethtypes.AccessList{{Address: addr, StorageKeys: nil}})
+	maxGasLimitTx.From = addr.Bytes()
+
 	var vmdb *statedb.StateDB
 
 	testCases := []struct {
@@ -199,15 +217,17 @@ 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),
 			math.MaxUint64,
 			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())
 		})