From e04422b6ffa761f2722884aa8ba4ac80574d8060 Mon Sep 17 00:00:00 2001 From: Adu Date: Mon, 8 Nov 2021 21:00:35 +0800 Subject: [PATCH] fix: tx fields are not authenticated by signature (#703) Co-authored-by: Muggle-Du --- CHANGELOG.md | 3 ++ app/ante/ante.go | 2 +- app/ante/ante_test.go | 104 ++++++++++++++++++++++++++++++++++++------ app/ante/eth.go | 96 ++++++++++++++++++++++++++++++++++++-- app/ante/sigs_test.go | 45 ++++++++++++++++++ 5 files changed, 232 insertions(+), 18 deletions(-) create mode 100644 app/ante/sigs_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ab88f90..6bb1dc6e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (evm, ante) [tharsis#620](https://github.com/tharsis/ethermint/pull/620) Add fee market field to EVM `Keeper` and `AnteHandler`. * (all) [tharsis#231](https://github.com/tharsis/ethermint/pull/231) Bump go-ethereum version to [`v1.10.9`](https://github.com/ethereum/go-ethereum/releases/tag/v1.10.9) +* (ante) [tharsis#703](https://github.com/tharsis/ethermint/pull/703) Fix some fields in transaction are not authenticated by signature. ### Features @@ -397,3 +398,5 @@ corresponding Ethereum API namespace: * (`x/evm`) [\#319](https://github.com/cosmos/ethermint/pull/319) Fix `SetBlockHash` that was setting the incorrect height during `BeginBlock`. * (`x/evm`) [\#176](https://github.com/cosmos/ethermint/issues/176) Updated Web3 transaction hash from using RLP hash. Now all transaction hashes exposed are amino hashes: * Removes `Hash()` (RLP) function from `MsgEthereumTx` to avoid confusion or misuse in future. + + diff --git a/app/ante/ante.go b/app/ante/ante.go index 1adb7d6a..55c96866 100644 --- a/app/ante/ante.go +++ b/app/ante/ante.go @@ -59,7 +59,7 @@ func NewAnteHandler( authante.NewMempoolFeeDecorator(), authante.NewTxTimeoutHeightDecorator(), authante.NewValidateMemoDecorator(ak), - NewEthValidateBasicDecorator(), + NewEthValidateBasicDecorator(evmKeeper), NewEthSigVerificationDecorator(evmKeeper), NewEthAccountVerificationDecorator(ak, bankKeeper, evmKeeper), NewEthNonceVerificationDecorator(ak), diff --git a/app/ante/ante_test.go b/app/ante/ante_test.go index 0ae08bf9..ce55373d 100644 --- a/app/ante/ante_test.go +++ b/app/ante/ante_test.go @@ -38,7 +38,7 @@ func (suite AnteTestSuite) TestAnteHandler() { signedContractTx := evmtypes.NewTxContract(suite.app.EvmKeeper.ChainID(), 1, big.NewInt(10), 100000, big.NewInt(1), nil, nil, nil, nil) signedContractTx.From = addr.Hex() - tx := suite.CreateTestTx(signedContractTx, privKey, 1, true) + tx := suite.CreateTestTx(signedContractTx, privKey, 1, false) return tx }, false, false, true, @@ -49,7 +49,7 @@ func (suite AnteTestSuite) TestAnteHandler() { signedContractTx := evmtypes.NewTxContract(suite.app.EvmKeeper.ChainID(), 1, big.NewInt(10), 100000, big.NewInt(1), nil, nil, nil, nil) signedContractTx.From = addr.Hex() - tx := suite.CreateTestTx(signedContractTx, privKey, 1, true) + tx := suite.CreateTestTx(signedContractTx, privKey, 1, false) return tx }, true, false, true, @@ -60,7 +60,7 @@ func (suite AnteTestSuite) TestAnteHandler() { signedContractTx := evmtypes.NewTxContract(suite.app.EvmKeeper.ChainID(), 1, big.NewInt(10), 100000, big.NewInt(1), nil, nil, nil, nil) signedContractTx.From = addr.Hex() - tx := suite.CreateTestTx(signedContractTx, privKey, 1, true) + tx := suite.CreateTestTx(signedContractTx, privKey, 1, false) return tx }, false, true, true, @@ -71,7 +71,7 @@ func (suite AnteTestSuite) TestAnteHandler() { signedTx := evmtypes.NewTx(suite.app.EvmKeeper.ChainID(), 1, &to, big.NewInt(10), 100000, big.NewInt(1), nil, nil, nil, nil) signedTx.From = addr.Hex() - tx := suite.CreateTestTx(signedTx, privKey, 1, true) + tx := suite.CreateTestTx(signedTx, privKey, 1, false) return tx }, false, false, true, @@ -82,7 +82,7 @@ func (suite AnteTestSuite) TestAnteHandler() { signedTx := evmtypes.NewTx(suite.app.EvmKeeper.ChainID(), 2, &to, big.NewInt(10), 100000, big.NewInt(1), nil, nil, nil, nil) signedTx.From = addr.Hex() - tx := suite.CreateTestTx(signedTx, privKey, 1, true) + tx := suite.CreateTestTx(signedTx, privKey, 1, false) return tx }, true, false, true, @@ -93,7 +93,7 @@ func (suite AnteTestSuite) TestAnteHandler() { signedTx := evmtypes.NewTx(suite.app.EvmKeeper.ChainID(), 3, &to, big.NewInt(10), 100000, big.NewInt(1), nil, nil, nil, nil) signedTx.From = addr.Hex() - tx := suite.CreateTestTx(signedTx, privKey, 1, true) + tx := suite.CreateTestTx(signedTx, privKey, 1, false) return tx }, false, true, true, }, @@ -125,11 +125,87 @@ func (suite AnteTestSuite) TestAnteHandler() { signedTx := evmtypes.NewTx(suite.app.EvmKeeper.ChainID(), 5, &to, big.NewInt(10), 100000, big.NewInt(1), nil, nil, nil, nil) signedTx.From = addr.Hex() - txBuilder := suite.CreateTestTxBuilder(signedTx, privKey, 1, true) + txBuilder := suite.CreateTestTxBuilder(signedTx, privKey, 1, false) txBuilder.SetMemo(strings.Repeat("*", 257)) return txBuilder.GetTx() }, true, false, false, }, + // Based on EVMBackend.SendTransaction, for cosmos tx, forcing null for some fields except ExtensionOptions, Fee, MsgEthereumTx + // should be part of consensus + { + "fail - DeliverTx (cosmos tx signed)", + func() sdk.Tx { + nonce, err := suite.app.AccountKeeper.GetSequence(suite.ctx, acc.GetAddress()) + suite.Require().NoError(err) + signedTx := evmtypes.NewTx(suite.app.EvmKeeper.ChainID(), nonce, &to, big.NewInt(10), 100000, big.NewInt(1), nil, nil, nil, nil) + signedTx.From = addr.Hex() + + tx := suite.CreateTestTx(signedTx, privKey, 1, true) + return tx + }, false, false, false, + }, + { + "fail - DeliverTx (cosmos tx with memo)", + func() sdk.Tx { + nonce, err := suite.app.AccountKeeper.GetSequence(suite.ctx, acc.GetAddress()) + suite.Require().NoError(err) + signedTx := evmtypes.NewTx(suite.app.EvmKeeper.ChainID(), nonce, &to, big.NewInt(10), 100000, big.NewInt(1), nil, nil, nil, nil) + signedTx.From = addr.Hex() + + txBuilder := suite.CreateTestTxBuilder(signedTx, privKey, 1, false) + txBuilder.SetMemo("memo for cosmos tx not allowed") + return txBuilder.GetTx() + }, false, false, false, + }, + { + "fail - DeliverTx (cosmos tx with timeoutheight)", + func() sdk.Tx { + nonce, err := suite.app.AccountKeeper.GetSequence(suite.ctx, acc.GetAddress()) + suite.Require().NoError(err) + signedTx := evmtypes.NewTx(suite.app.EvmKeeper.ChainID(), nonce, &to, big.NewInt(10), 100000, big.NewInt(1), nil, nil, nil, nil) + signedTx.From = addr.Hex() + + txBuilder := suite.CreateTestTxBuilder(signedTx, privKey, 1, false) + txBuilder.SetTimeoutHeight(10) + return txBuilder.GetTx() + }, false, false, false, + }, + { + "fail - DeliverTx (invalid fee amount)", + func() sdk.Tx { + nonce, err := suite.app.AccountKeeper.GetSequence(suite.ctx, acc.GetAddress()) + suite.Require().NoError(err) + signedTx := evmtypes.NewTx(suite.app.EvmKeeper.ChainID(), nonce, &to, big.NewInt(10), 100000, big.NewInt(1), nil, nil, nil, nil) + signedTx.From = addr.Hex() + + txBuilder := suite.CreateTestTxBuilder(signedTx, privKey, 1, false) + + txData, err := evmtypes.UnpackTxData(signedTx.Data) + suite.Require().NoError(err) + + expFee := txData.Fee() + invalidFee := new(big.Int).Add(expFee, big.NewInt(1)) + invalidFeeAmount := sdk.Coins{sdk.NewCoin(evmtypes.DefaultEVMDenom, sdk.NewIntFromBigInt(invalidFee))} + txBuilder.SetFeeAmount(invalidFeeAmount) + return txBuilder.GetTx() + }, false, false, false, + }, + { + "fail - DeliverTx (invalid fee gaslimit)", + func() sdk.Tx { + nonce, err := suite.app.AccountKeeper.GetSequence(suite.ctx, acc.GetAddress()) + suite.Require().NoError(err) + signedTx := evmtypes.NewTx(suite.app.EvmKeeper.ChainID(), nonce, &to, big.NewInt(10), 100000, big.NewInt(1), nil, nil, nil, nil) + signedTx.From = addr.Hex() + + txBuilder := suite.CreateTestTxBuilder(signedTx, privKey, 1, false) + + expGasLimit := signedTx.GetGas() + invalidGasLimit := expGasLimit + 1 + txBuilder.SetGasLimit(invalidGasLimit) + return txBuilder.GetTx() + }, false, false, false, + }, } for _, tc := range testCases { @@ -188,7 +264,7 @@ func (suite AnteTestSuite) TestAnteHandlerWithDynamicTxFee() { ) signedContractTx.From = addr.Hex() - tx := suite.CreateTestTx(signedContractTx, privKey, 1, true) + tx := suite.CreateTestTx(signedContractTx, privKey, 1, false) return tx }, false, false, true, @@ -210,7 +286,7 @@ func (suite AnteTestSuite) TestAnteHandlerWithDynamicTxFee() { ) signedContractTx.From = addr.Hex() - tx := suite.CreateTestTx(signedContractTx, privKey, 1, true) + tx := suite.CreateTestTx(signedContractTx, privKey, 1, false) return tx }, true, false, true, @@ -232,7 +308,7 @@ func (suite AnteTestSuite) TestAnteHandlerWithDynamicTxFee() { ) signedContractTx.From = addr.Hex() - tx := suite.CreateTestTx(signedContractTx, privKey, 1, true) + tx := suite.CreateTestTx(signedContractTx, privKey, 1, false) return tx }, false, true, true, @@ -255,7 +331,7 @@ func (suite AnteTestSuite) TestAnteHandlerWithDynamicTxFee() { ) signedTx.From = addr.Hex() - tx := suite.CreateTestTx(signedTx, privKey, 1, true) + tx := suite.CreateTestTx(signedTx, privKey, 1, false) return tx }, false, false, true, @@ -278,7 +354,7 @@ func (suite AnteTestSuite) TestAnteHandlerWithDynamicTxFee() { ) signedTx.From = addr.Hex() - tx := suite.CreateTestTx(signedTx, privKey, 1, true) + tx := suite.CreateTestTx(signedTx, privKey, 1, false) return tx }, true, false, true, @@ -301,7 +377,7 @@ func (suite AnteTestSuite) TestAnteHandlerWithDynamicTxFee() { ) signedTx.From = addr.Hex() - tx := suite.CreateTestTx(signedTx, privKey, 1, true) + tx := suite.CreateTestTx(signedTx, privKey, 1, false) return tx }, false, true, true, }, @@ -369,7 +445,7 @@ func (suite AnteTestSuite) TestAnteHandlerWithDynamicTxFee() { ) signedTx.From = addr.Hex() - txBuilder := suite.CreateTestTxBuilder(signedTx, privKey, 1, true) + txBuilder := suite.CreateTestTxBuilder(signedTx, privKey, 1, false) txBuilder.SetMemo(strings.Repeat("*", 257)) return txBuilder.GetTx() }, true, false, false, diff --git a/app/ante/eth.go b/app/ante/eth.go index 76e36815..1220eb10 100644 --- a/app/ante/eth.go +++ b/app/ante/eth.go @@ -8,6 +8,7 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" + tx "github.com/cosmos/cosmos-sdk/types/tx" authante "github.com/cosmos/cosmos-sdk/x/auth/ante" ethermint "github.com/tharsis/ethermint/types" @@ -35,6 +36,10 @@ type EVMKeeper interface { ) (sdk.Coins, error) } +type protoTxProvider interface { + GetProtoTx() *tx.Tx +} + // EthSigVerificationDecorator validates an ethereum signatures type EthSigVerificationDecorator struct { evmKeeper EVMKeeper @@ -534,11 +539,15 @@ func (issd EthIncrementSenderSequenceDecorator) AnteHandle(ctx sdk.Context, tx s } // EthValidateBasicDecorator is adapted from ValidateBasicDecorator from cosmos-sdk, it ignores ErrNoSignatures -type EthValidateBasicDecorator struct{} +type EthValidateBasicDecorator struct { + evmKeeper EVMKeeper +} // NewEthValidateBasicDecorator creates a new EthValidateBasicDecorator -func NewEthValidateBasicDecorator() EthValidateBasicDecorator { - return EthValidateBasicDecorator{} +func NewEthValidateBasicDecorator(ek EVMKeeper) EthValidateBasicDecorator { + return EthValidateBasicDecorator{ + evmKeeper: ek, + } } // AnteHandle handles basic validation of tx @@ -554,6 +563,87 @@ func (vbd EthValidateBasicDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simu return ctx, stacktrace.Propagate(err, "tx basic validation failed") } + // For eth type cosmos tx, some fields should be veified as zero values, + // since we will only verify the signature against the hash of the MsgEthereumTx.Data + if wrapperTx, ok := tx.(protoTxProvider); ok { + protoTx := wrapperTx.GetProtoTx() + body := protoTx.Body + if body.Memo != "" || body.TimeoutHeight != uint64(0) || len(body.NonCriticalExtensionOptions) > 0 { + return ctx, stacktrace.Propagate( + sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, + "for eth tx body Memo TimeoutHeight NonCriticalExtensionOptions should be empty"), + "invalid data in tx", + ) + } + + if len(body.ExtensionOptions) != 1 { + return ctx, stacktrace.Propagate( + sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "for eth tx length of ExtensionOptions should be 1"), + "invalid data in tx", + ) + } + + if len(protoTx.GetMsgs()) != 1 { + return ctx, stacktrace.Propagate( + sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "only 1 ethereum msg supported per tx"), + "", + ) + } + msg := protoTx.GetMsgs()[0] + msgEthTx, ok := msg.(*evmtypes.MsgEthereumTx) + if !ok { + return ctx, stacktrace.Propagate( + sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "invalid transaction type %T, expected %T", tx, (*evmtypes.MsgEthereumTx)(nil)), + "failed to cast transaction", + ) + } + ethGasLimit := msgEthTx.GetGas() + + txData, err := evmtypes.UnpackTxData(msgEthTx.Data) + if err != nil { + return ctx, stacktrace.Propagate(err, "failed to unpack MsgEthereumTx Data") + } + params := vbd.evmKeeper.GetParams(ctx) + ethFeeAmount := sdk.Coins{sdk.NewCoin(params.EvmDenom, sdk.NewIntFromBigInt(txData.Fee()))} + + authInfo := protoTx.AuthInfo + if len(authInfo.SignerInfos) > 0 { + return ctx, stacktrace.Propagate( + sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "for eth tx AuthInfo SignerInfos should be empty"), + "invalid data in tx", + ) + } + + if authInfo.Fee.Payer != "" || authInfo.Fee.Granter != "" { + return ctx, stacktrace.Propagate( + sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "for eth tx AuthInfo Fee payer and granter should be empty"), + "invalid data in tx", + ) + } + + if !authInfo.Fee.Amount.IsEqual(ethFeeAmount) { + return ctx, stacktrace.Propagate( + sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "invalid eth tx AuthInfo Fee Amount"), + "invalid data in tx", + ) + } + + if authInfo.Fee.GasLimit != ethGasLimit { + return ctx, stacktrace.Propagate( + sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "invalid eth tx AuthInfo Fee GasLimit"), + "invalid data in tx", + ) + } + + sigs := protoTx.Signatures + if len(sigs) > 0 { + return ctx, stacktrace.Propagate( + sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "for eth tx Signatures should be empty"), + "invalid data in tx", + ) + } + } + return next(ctx, tx, simulate) } diff --git a/app/ante/sigs_test.go b/app/ante/sigs_test.go new file mode 100644 index 00000000..9c6f1c55 --- /dev/null +++ b/app/ante/sigs_test.go @@ -0,0 +1,45 @@ +package ante_test + +import ( + "math/big" + + "github.com/tharsis/ethermint/tests" + evmtypes "github.com/tharsis/ethermint/x/evm/types" +) + +func (suite AnteTestSuite) TestSignatures() { + suite.dynamicTxFee = false + suite.SetupTest() // reset + + addr, privKey := tests.NewAddrKey() + to := tests.GenerateAddress() + + acc := suite.app.AccountKeeper.NewAccountWithAddress(suite.ctx, addr.Bytes()) + suite.Require().NoError(acc.SetSequence(1)) + suite.app.AccountKeeper.SetAccount(suite.ctx, acc) + + suite.app.EvmKeeper.AddBalance(addr, big.NewInt(10000000000)) + msgEthereumTx := evmtypes.NewTx(suite.app.EvmKeeper.ChainID(), 1, &to, big.NewInt(10), 100000, big.NewInt(1), nil, nil, nil, nil) + msgEthereumTx.From = addr.Hex() + + // CreateTestTx will sign the msgEthereumTx but not sign the cosmos tx since we have signCosmosTx as false + tx := suite.CreateTestTx(msgEthereumTx, privKey, 1, false) + sigs, err := tx.GetSignaturesV2() + suite.Require().NoError(err) + + // signatures of cosmos tx should be empty + suite.Require().Equal(len(sigs), 0) + + txData, err := evmtypes.UnpackTxData(msgEthereumTx.Data) + suite.Require().NoError(err) + + msgV, msgR, msgS := txData.GetRawSignatureValues() + + ethTx := msgEthereumTx.AsTransaction() + ethV, ethR, ethS := ethTx.RawSignatureValues() + + // The signatures of MsgehtereumTx should be the same with the corresponding eth tx + suite.Require().Equal(msgV, ethV) + suite.Require().Equal(msgR, ethR) + suite.Require().Equal(msgS, ethS) +}