From bc1d81c5e874109bd95edd99725ab764ce9f922b Mon Sep 17 00:00:00 2001 From: yihuang Date: Tue, 26 Oct 2021 19:13:27 +0800 Subject: [PATCH] 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 --- CHANGELOG.md | 1 + encoding/config.go | 5 +- x/evm/types/access_list_tx.go | 24 +++++++-- x/evm/types/dynamic_fee_tx.go | 34 +++++++++++-- x/evm/types/dynamic_fee_tx_test.go | 3 +- x/evm/types/errors.go | 4 ++ x/evm/types/legacy_tx.go | 24 +++++++-- x/evm/types/legacy_tx_test.go | 3 +- x/evm/types/msg.go | 13 +++-- x/evm/types/msg_test.go | 81 ++++++++++++++++++++++++++++++ x/evm/types/tx_data.go | 14 ++++-- x/evm/types/utils.go | 17 +++++++ 12 files changed, 196 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d8b9bd93..172efcc8 100644 --- 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 index 6c43b2e0..b1fc48e2 100644 --- 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 index a4758c62..b8187c21 100644 --- 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 index 24617230..1e1bfacd 100644 --- 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,6 +210,14 @@ 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)", @@ -208,11 +225,18 @@ func (tx DynamicFeeTx) Validate() error { ) } + 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 index a9f8c11e..1fbee58e 100644 --- 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 index a8cdad5f..811f506d 100644 --- 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 index bcc8f24c..6a1cc4cf 100644 --- 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 index 6e298a30..6ebc82a4 100644 --- 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 index aa61cf07..12ecb87f 100644 --- 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. diff --git a/x/evm/types/msg_test.go b/x/evm/types/msg_test.go index a1f4a06a..ca1dfbed 100644 --- a/x/evm/types/msg_test.go +++ b/x/evm/types/msg_test.go @@ -8,10 +8,12 @@ import ( "github.com/cosmos/cosmos-sdk/crypto/keyring" sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/tharsis/ethermint/crypto/ethsecp256k1" "github.com/tharsis/ethermint/tests" "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/core/types" + ethtypes "github.com/ethereum/go-ethereum/core/types" ) const invalidFromAddress = "0x0000" @@ -59,6 +61,7 @@ func (suite *MsgsTestSuite) TestMsgEthereumTx_ValidateBasic() { hundredInt := sdk.NewInt(100) zeroInt := sdk.ZeroInt() minusOneInt := sdk.NewInt(-1) + exp_2_255 := sdk.NewIntFromBigInt(new(big.Int).Exp(big.NewInt(2), big.NewInt(255), nil)) testCases := []struct { msg string @@ -80,6 +83,7 @@ func (suite *MsgsTestSuite) TestMsgEthereumTx_ValidateBasic() { {msg: "negative gas price - Legacy Tx", to: suite.to.Hex(), amount: &hundredInt, gasPrice: &minusOneInt, expectPass: false}, {msg: "zero gas price - Legacy Tx", to: suite.to.Hex(), amount: &hundredInt, gasPrice: &zeroInt, expectPass: true}, {msg: "invalid from address - Legacy Tx", to: suite.to.Hex(), amount: &hundredInt, gasPrice: &zeroInt, from: invalidFromAddress, expectPass: false}, + {msg: "out of bound gas fee - Legacy Tx", to: suite.to.Hex(), amount: &hundredInt, gasPrice: &exp_2_255, expectPass: false}, {msg: "nil amount - AccessListTx", to: suite.to.Hex(), amount: nil, gasPrice: &hundredInt, accessList: &types.AccessList{}, chainID: &hundredInt, expectPass: true}, {msg: "negative amount - AccessListTx", to: suite.to.Hex(), amount: &minusOneInt, gasPrice: &hundredInt, accessList: &types.AccessList{}, chainID: nil, expectPass: false}, {msg: "nil gas price - AccessListTx", to: suite.to.Hex(), amount: &hundredInt, gasPrice: nil, accessList: &types.AccessList{}, chainID: &hundredInt, expectPass: false}, @@ -183,3 +187,80 @@ func (suite *MsgsTestSuite) TestMsgEthereumTx_Sign() { } } } + +func (suite *MsgsTestSuite) TestFromEthereumTx() { + privkey, _ := ethsecp256k1.GenerateKey() + ethPriv, err := privkey.ToECDSA() + suite.Require().NoError(err) + + // 10^80 is more than 256 bits + exp_10_80 := new(big.Int).Mul(big.NewInt(1), new(big.Int).Exp(big.NewInt(10), big.NewInt(80), nil)) + + testCases := []struct { + msg string + expectPass bool + buildTx func() *ethtypes.Transaction + }{ + {"success, normal tx", true, func() *ethtypes.Transaction { + tx := ethtypes.NewTransaction( + 0, + common.BigToAddress(big.NewInt(1)), + big.NewInt(10), + 21000, big.NewInt(0), + nil, + ) + tx, err := ethtypes.SignTx(tx, types.NewEIP2930Signer(suite.chainID), ethPriv) + suite.Require().NoError(err) + return tx + }}, + {"fail, value bigger than 256bits", false, func() *ethtypes.Transaction { + tx := ethtypes.NewTransaction( + 0, + common.BigToAddress(big.NewInt(1)), + exp_10_80, + 21000, big.NewInt(0), + nil, + ) + tx, err := ethtypes.SignTx(tx, types.NewEIP2930Signer(suite.chainID), ethPriv) + suite.Require().NoError(err) + return tx + }}, + {"fail, gas price bigger than 256bits", false, func() *ethtypes.Transaction { + tx := ethtypes.NewTransaction( + 0, + common.BigToAddress(big.NewInt(1)), + big.NewInt(10), + 21000, exp_10_80, + nil, + ) + tx, err := ethtypes.SignTx(tx, types.NewEIP2930Signer(suite.chainID), ethPriv) + suite.Require().NoError(err) + return tx + }}, + } + + for _, tc := range testCases { + ethTx := tc.buildTx() + tx := &MsgEthereumTx{} + err := tx.FromEthereumTx(ethTx) + if tc.expectPass { + suite.Require().NoError(err) + + // round-trip test + suite.assertEthTxEqual(tx.AsTransaction(), ethTx) + } else { + suite.Require().Error(err) + } + } +} + +func (suite *MsgsTestSuite) assertEthTxEqual(tx1 *ethtypes.Transaction, tx2 *ethtypes.Transaction) { + suite.Require().Equal(tx1.Hash(), tx2.Hash()) + suite.Require().Equal(tx1.Size(), tx2.Size()) + + bin1, err := tx1.MarshalBinary() + suite.Require().NoError(err) + bin2, err := tx2.MarshalBinary() + suite.Require().NoError(err) + suite.Require().Equal(bin1, bin2) +} diff --git a/x/evm/types/tx_data.go b/x/evm/types/tx_data.go index 4e72209c..5ec2fa66 100644 --- a/x/evm/types/tx_data.go +++ b/x/evm/types/tx_data.go @@ -40,18 +40,22 @@ type TxData interface { Cost() *big.Int } -func NewTxDataFromTx(tx *ethtypes.Transaction) TxData { +func NewTxDataFromTx(tx *ethtypes.Transaction) (TxData, error) { var txData TxData + var err error switch tx.Type() { case ethtypes.DynamicFeeTxType: - txData = newDynamicFeeTx(tx) + txData, err = newDynamicFeeTx(tx) case ethtypes.AccessListTxType: - txData = newAccessListTx(tx) + txData, err = newAccessListTx(tx) default: - txData = newLegacyTx(tx) + txData, err = newLegacyTx(tx) + } + if err != nil { + return nil, err } - return txData + return txData, nil } // DeriveChainID derives the chain id from the given v parameter. diff --git a/x/evm/types/utils.go b/x/evm/types/utils.go index 285bf51f..0839c200 100644 --- a/x/evm/types/utils.go +++ b/x/evm/types/utils.go @@ -1,7 +1,9 @@ package types import ( + "errors" "fmt" + "math/big" "github.com/gogo/protobuf/proto" @@ -11,6 +13,8 @@ import ( "github.com/ethereum/go-ethereum/crypto" ) +const maxBitLen = 256 + var EmptyCodeHash = crypto.Keccak256(nil) // DecodeTxResponse decodes an protobuf-encoded byte slice into TxResponse @@ -86,3 +90,16 @@ func BinSearch(lo, hi uint64, executable func(uint64) (bool, *MsgEthereumTxRespo } return hi, nil } + +// SafeNewIntFromBigInt constructs Int from big.Int, return error if more than 256bits +func SafeNewIntFromBigInt(i *big.Int) (sdk.Int, error) { + if !IsValidInt256(i) { + return sdk.NewInt(0), errors.New("SafeNewIntFromBigInt() out of bound") // nolint + } + return sdk.NewIntFromBigInt(i), nil +} + +// IsValidInt256 check the bound of 256 bit number +func IsValidInt256(i *big.Int) bool { + return i == nil || i.BitLen() <= maxBitLen +}