fix: Web3 RPC handlers panic (#702)

* Problem: Some Web3 RPC Handlers could panic Closes: #701 Solution: - return error rather than panic when decoding invalid tx * add validation rules * changelog
evmos · Oct 26, 2021 · bc1d81c · bc1d81c
1 parent 23a3362
commit bc1d81c
Show file tree

Hide file tree

Showing 12 changed files with 196 additions and 27 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -53,6 +53,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (rpc) [tharsis#661](https://github.com/tharsis/ethermint/pull/661) Fix OOM bug when creating too many filters using JSON-RPC.
 * (evm) [tharsis#660](https://github.com/tharsis/ethermint/pull/660) Fix `nil` pointer panic in `ApplyNativeMessage`.
 * (evm, test) [tharsis#649](https://github.com/tharsis/ethermint/pull/649) Test DynamicFeeTx.
+* (evm) [tharsis#702](https://github.com/tharsis/ethermint/pull/702) Fix panic in web3 RPC handlers 
 
 ### Improvements
 

diff --git a/encoding/config.go b/encoding/config.go
@@ -68,7 +68,10 @@ func (g txConfig) TxDecoder() sdk.TxDecoder {
 		err := tx.UnmarshalBinary(txBytes)
 		if err == nil {
 			msg := &evmtypes.MsgEthereumTx{}
-			msg.FromEthereumTx(tx)
+			err := msg.FromEthereumTx(tx)
+			if err != nil {
+				return nil, err
+			}
 			return msg, nil
 		}
 

diff --git a/x/evm/types/access_list_tx.go b/x/evm/types/access_list_tx.go
@@ -10,7 +10,7 @@ import (
 	"github.com/tharsis/ethermint/types"
 )
 
-func newAccessListTx(tx *ethtypes.Transaction) *AccessListTx {
+func newAccessListTx(tx *ethtypes.Transaction) (*AccessListTx, error) {
 	txData := &AccessListTx{
 		Nonce:    tx.Nonce(),
 		Data:     tx.Data(),
@@ -23,12 +23,18 @@ func newAccessListTx(tx *ethtypes.Transaction) *AccessListTx {
 	}
 
 	if tx.Value() != nil {
-		amountInt := sdk.NewIntFromBigInt(tx.Value())
+		amountInt, err := SafeNewIntFromBigInt(tx.Value())
+		if err != nil {
+			return nil, err
+		}
 		txData.Amount = &amountInt
 	}
 
 	if tx.GasPrice() != nil {
-		gasPriceInt := sdk.NewIntFromBigInt(tx.GasPrice())
+		gasPriceInt, err := SafeNewIntFromBigInt(tx.GasPrice())
+		if err != nil {
+			return nil, err
+		}
 		txData.GasPrice = &gasPriceInt
 	}
 
@@ -38,7 +44,7 @@ func newAccessListTx(tx *ethtypes.Transaction) *AccessListTx {
 	}
 
 	txData.SetSignatureValues(tx.ChainId(), v, r, s)
-	return txData
+	return txData, nil
 }
 
 // TxType returns the tx type
@@ -177,6 +183,9 @@ func (tx AccessListTx) Validate() error {
 	if gasPrice == nil {
 		return sdkerrors.Wrap(ErrInvalidGasPrice, "cannot be nil")
 	}
+	if !IsValidInt256(gasPrice) {
+		return sdkerrors.Wrap(ErrInvalidGasPrice, "out of bound")
+	}
 
 	if gasPrice.Sign() == -1 {
 		return sdkerrors.Wrapf(ErrInvalidGasPrice, "gas price cannot be negative %s", gasPrice)
@@ -187,6 +196,13 @@ func (tx AccessListTx) Validate() error {
 	if amount != nil && amount.Sign() == -1 {
 		return sdkerrors.Wrapf(ErrInvalidAmount, "amount cannot be negative %s", amount)
 	}
+	if !IsValidInt256(amount) {
+		return sdkerrors.Wrap(ErrInvalidAmount, "out of bound")
+	}
+
+	if !IsValidInt256(tx.Fee()) {
+		return sdkerrors.Wrap(ErrInvalidGasFee, "out of bound")
+	}
 
 	if tx.To != "" {
 		if err := types.ValidateAddress(tx.To); err != nil {

diff --git a/x/evm/types/dynamic_fee_tx.go b/x/evm/types/dynamic_fee_tx.go
@@ -12,7 +12,7 @@ import (
 	"github.com/tharsis/ethermint/types"
 )
 
-func newDynamicFeeTx(tx *ethtypes.Transaction) *DynamicFeeTx {
+func newDynamicFeeTx(tx *ethtypes.Transaction) (*DynamicFeeTx, error) {
 	txData := &DynamicFeeTx{
 		Nonce:    tx.Nonce(),
 		Data:     tx.Data(),
@@ -25,17 +25,26 @@ func newDynamicFeeTx(tx *ethtypes.Transaction) *DynamicFeeTx {
 	}
 
 	if tx.Value() != nil {
-		amountInt := sdk.NewIntFromBigInt(tx.Value())
+		amountInt, err := SafeNewIntFromBigInt(tx.Value())
+		if err != nil {
+			return nil, err
+		}
 		txData.Amount = &amountInt
 	}
 
 	if tx.GasFeeCap() != nil {
-		gasFeeCapInt := sdk.NewIntFromBigInt(tx.GasFeeCap())
+		gasFeeCapInt, err := SafeNewIntFromBigInt(tx.GasFeeCap())
+		if err != nil {
+			return nil, err
+		}
 		txData.GasFeeCap = &gasFeeCapInt
 	}
 
 	if tx.GasTipCap() != nil {
-		gasTipCapInt := sdk.NewIntFromBigInt(tx.GasTipCap())
+		gasTipCapInt, err := SafeNewIntFromBigInt(tx.GasTipCap())
+		if err != nil {
+			return nil, err
+		}
 		txData.GasTipCap = &gasTipCapInt
 	}
 
@@ -45,7 +54,7 @@ func newDynamicFeeTx(tx *ethtypes.Transaction) *DynamicFeeTx {
 	}
 
 	txData.SetSignatureValues(tx.ChainId(), v, r, s)
-	return txData
+	return txData, nil
 }
 
 // TxType returns the tx type
@@ -201,18 +210,33 @@ func (tx DynamicFeeTx) Validate() error {
 		return sdkerrors.Wrapf(ErrInvalidGasCap, "gas fee cap cannot be negative %s", tx.GasFeeCap)
 	}
 
+	if !IsValidInt256(tx.GetGasTipCap()) {
+		return sdkerrors.Wrap(ErrInvalidGasCap, "out of bound")
+	}
+
+	if !IsValidInt256(tx.GetGasFeeCap()) {
+		return sdkerrors.Wrap(ErrInvalidGasCap, "out of bound")
+	}
+
 	if tx.GasFeeCap.LT(*tx.GasTipCap) {
 		return sdkerrors.Wrapf(
 			ErrInvalidGasCap, "max priority fee per gas higher than max fee per gas (%s > %s)",
 			tx.GasTipCap, tx.GasFeeCap,
 		)
 	}
 
+	if !IsValidInt256(tx.Fee()) {
+		return sdkerrors.Wrap(ErrInvalidGasFee, "out of bound")
+	}
+
 	amount := tx.GetValue()
 	// Amount can be 0
 	if amount != nil && amount.Sign() == -1 {
 		return sdkerrors.Wrapf(ErrInvalidAmount, "amount cannot be negative %s", amount)
 	}
+	if !IsValidInt256(amount) {
+		return sdkerrors.Wrap(ErrInvalidAmount, "out of bound")
+	}
 
 	if tx.To != "" {
 		if err := types.ValidateAddress(tx.To); err != nil {

diff --git a/x/evm/types/dynamic_fee_tx_test.go b/x/evm/types/dynamic_fee_tx_test.go
@@ -60,7 +60,8 @@ func (suite *TxDataTestSuite) TestNewDynamicFeeTx() {
 		},
 	}
 	for _, tc := range testCases {
-		tx := newDynamicFeeTx(tc.tx)
+		tx, err := newDynamicFeeTx(tc.tx)
+		suite.Require().NoError(err)
 
 		suite.Require().NotEmpty(tx)
 		suite.Require().Equal(uint8(2), tx.TxType())

diff --git a/x/evm/types/errors.go b/x/evm/types/errors.go
@@ -25,6 +25,7 @@ const (
 	codeErrCallDisabled
 	codeErrInvalidAmount
 	codeErrInvalidGasPrice
+	codeErrInvalidGasFee
 	codeErrVMExecution
 	codeErrInvalidRefund
 	codeErrInconsistentGas
@@ -72,6 +73,9 @@ var (
 	// ErrInvalidGasPrice returns an error if an invalid gas price is provided to the tx.
 	ErrInvalidGasPrice = sdkerrors.Register(ModuleName, codeErrInvalidGasPrice, "invalid gas price")
 
+	// ErrInvalidGasFee returns an error if the tx gas fee is out of bound.
+	ErrInvalidGasFee = sdkerrors.Register(ModuleName, codeErrInvalidGasFee, "invalid gas fee")
+
 	// ErrVMExecution returns an error resulting from an error in EVM execution.
 	ErrVMExecution = sdkerrors.Register(ModuleName, codeErrVMExecution, "evm transaction execution failed")
 

diff --git a/x/evm/types/legacy_tx.go b/x/evm/types/legacy_tx.go
@@ -3,14 +3,13 @@ package types
 import (
 	"math/big"
 
-	sdk "github.com/cosmos/cosmos-sdk/types"
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
 	"github.com/ethereum/go-ethereum/common"
 	ethtypes "github.com/ethereum/go-ethereum/core/types"
 	"github.com/tharsis/ethermint/types"
 )
 
-func newLegacyTx(tx *ethtypes.Transaction) *LegacyTx {
+func newLegacyTx(tx *ethtypes.Transaction) (*LegacyTx, error) {
 	txData := &LegacyTx{
 		Nonce:    tx.Nonce(),
 		Data:     tx.Data(),
@@ -23,17 +22,23 @@ func newLegacyTx(tx *ethtypes.Transaction) *LegacyTx {
 	}
 
 	if tx.Value() != nil {
-		amountInt := sdk.NewIntFromBigInt(tx.Value())
+		amountInt, err := SafeNewIntFromBigInt(tx.Value())
+		if err != nil {
+			return nil, err
+		}
 		txData.Amount = &amountInt
 	}
 
 	if tx.GasPrice() != nil {
-		gasPriceInt := sdk.NewIntFromBigInt(tx.GasPrice())
+		gasPriceInt, err := SafeNewIntFromBigInt(tx.GasPrice())
+		if err != nil {
+			return nil, err
+		}
 		txData.GasPrice = &gasPriceInt
 	}
 
 	txData.SetSignatureValues(tx.ChainId(), v, r, s)
-	return txData
+	return txData, nil
 }
 
 // TxType returns the tx type
@@ -161,12 +166,21 @@ func (tx LegacyTx) Validate() error {
 	if gasPrice.Sign() == -1 {
 		return sdkerrors.Wrapf(ErrInvalidGasPrice, "gas price cannot be negative %s", gasPrice)
 	}
+	if !IsValidInt256(gasPrice) {
+		return sdkerrors.Wrap(ErrInvalidGasPrice, "out of bound")
+	}
+	if !IsValidInt256(tx.Fee()) {
+		return sdkerrors.Wrap(ErrInvalidGasFee, "out of bound")
+	}
 
 	amount := tx.GetValue()
 	// Amount can be 0
 	if amount != nil && amount.Sign() == -1 {
 		return sdkerrors.Wrapf(ErrInvalidAmount, "amount cannot be negative %s", amount)
 	}
+	if !IsValidInt256(amount) {
+		return sdkerrors.Wrap(ErrInvalidAmount, "out of bound")
+	}
 
 	if tx.To != "" {
 		if err := types.ValidateAddress(tx.To); err != nil {

diff --git a/x/evm/types/legacy_tx_test.go b/x/evm/types/legacy_tx_test.go
@@ -29,7 +29,8 @@ func (suite *TxDataTestSuite) TestNewLegacyTx() {
 	}
 
 	for _, tc := range testCases {
-		tx := newLegacyTx(tc.tx)
+		tx, err := newLegacyTx(tc.tx)
+		suite.Require().NoError(err)
 
 		suite.Require().NotEmpty(tc.tx)
 		suite.Require().Equal(uint8(0), tx.TxType())

diff --git a/x/evm/types/msg.go b/x/evm/types/msg.go
@@ -130,17 +130,21 @@ func newMsgEthereumTx(
 }
 
 // fromEthereumTx populates the message fields from the given ethereum transaction
-func (msg *MsgEthereumTx) FromEthereumTx(tx *ethtypes.Transaction) {
-	txData := NewTxDataFromTx(tx)
+func (msg *MsgEthereumTx) FromEthereumTx(tx *ethtypes.Transaction) error {
+	txData, err := NewTxDataFromTx(tx)
+	if err != nil {
+		return err
+	}
 
 	anyTxData, err := PackTxData(txData)
 	if err != nil {
-		panic(err)
+		return err
 	}
 
 	msg.Data = anyTxData
 	msg.Size_ = float64(tx.Size())
 	msg.Hash = tx.Hash().Hex()
+	return nil
 }
 
 // Route returns the route value of an MsgEthereumTx.
@@ -225,8 +229,7 @@ func (msg *MsgEthereumTx) Sign(ethSigner ethtypes.Signer, keyringSigner keyring.
 		return err
 	}
 
-	msg.FromEthereumTx(tx)
-	return nil
+	return msg.FromEthereumTx(tx)
 }
 
 // GetGas implements the GasTx interface. It returns the GasLimit of the transaction.