From e536252f991efddaae81d68498122058d7a13f79 Mon Sep 17 00:00:00 2001 From: HuangYi Date: Mon, 25 Oct 2021 17:17:43 +0800 Subject: [PATCH] Problem: Some Web3 RPC Handlers could panic Closes: #701 Solution: - return error rather than panic when decoding invalid tx --- encoding/config.go | 5 +- x/evm/types/access_list_tx.go | 14 ++++-- x/evm/types/dynamic_fee_tx.go | 19 +++++-- x/evm/types/dynamic_fee_tx_test.go | 3 +- x/evm/types/legacy_tx.go | 15 ++++-- x/evm/types/legacy_tx_test.go | 3 +- x/evm/types/msg.go | 13 +++-- x/evm/types/msg_test.go | 79 ++++++++++++++++++++++++++++++ x/evm/types/tx_data.go | 14 ++++-- x/evm/types/utils.go | 12 +++++ 10 files changed, 150 insertions(+), 27 deletions(-) diff --git a/encoding/config.go b/encoding/config.go index 6c43b2e007..b1fc48e209 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 a4758c6248..5a37805a58 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 diff --git a/x/evm/types/dynamic_fee_tx.go b/x/evm/types/dynamic_fee_tx.go index 2461723072..acdc6e8274 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 diff --git a/x/evm/types/dynamic_fee_tx_test.go b/x/evm/types/dynamic_fee_tx_test.go index a9f8c11e32..1fbee58e20 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/legacy_tx.go b/x/evm/types/legacy_tx.go index bcc8f24c3c..0726e9f55e 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 diff --git a/x/evm/types/legacy_tx_test.go b/x/evm/types/legacy_tx_test.go index 6e298a30b6..6ebc82a494 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 aa61cf077f..12ecb87ff0 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 a1f4a06aa9..3383d0368d 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" @@ -183,3 +185,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 4e72209cda..5ec2fa6688 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 285bf51f94..e44547f7d1 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,11 @@ 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 i.BitLen() > maxBitLen { + return sdk.NewInt(0), errors.New("SafeNewIntFromBigInt() out of bound") // nolint + } + return sdk.NewIntFromBigInt(i), nil +}