fix: tx fields are not authenticated by signature (#703)

Co-authored-by: Muggle-Du <adudu@CNMAC0342.local>
This commit is contained in:
Adu 2021-11-08 21:00:35 +08:00 committed by GitHub
parent fb33f9b186
commit e04422b6ff
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 232 additions and 18 deletions

View File

@ -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`. * (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) * (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 ### 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`) [\#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: * (`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. * Removes `Hash()` (RLP) function from `MsgEthereumTx` to avoid confusion or misuse in future.

View File

@ -59,7 +59,7 @@ func NewAnteHandler(
authante.NewMempoolFeeDecorator(), authante.NewMempoolFeeDecorator(),
authante.NewTxTimeoutHeightDecorator(), authante.NewTxTimeoutHeightDecorator(),
authante.NewValidateMemoDecorator(ak), authante.NewValidateMemoDecorator(ak),
NewEthValidateBasicDecorator(), NewEthValidateBasicDecorator(evmKeeper),
NewEthSigVerificationDecorator(evmKeeper), NewEthSigVerificationDecorator(evmKeeper),
NewEthAccountVerificationDecorator(ak, bankKeeper, evmKeeper), NewEthAccountVerificationDecorator(ak, bankKeeper, evmKeeper),
NewEthNonceVerificationDecorator(ak), NewEthNonceVerificationDecorator(ak),

View File

@ -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 := evmtypes.NewTxContract(suite.app.EvmKeeper.ChainID(), 1, big.NewInt(10), 100000, big.NewInt(1), nil, nil, nil, nil)
signedContractTx.From = addr.Hex() signedContractTx.From = addr.Hex()
tx := suite.CreateTestTx(signedContractTx, privKey, 1, true) tx := suite.CreateTestTx(signedContractTx, privKey, 1, false)
return tx return tx
}, },
false, false, true, 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 := evmtypes.NewTxContract(suite.app.EvmKeeper.ChainID(), 1, big.NewInt(10), 100000, big.NewInt(1), nil, nil, nil, nil)
signedContractTx.From = addr.Hex() signedContractTx.From = addr.Hex()
tx := suite.CreateTestTx(signedContractTx, privKey, 1, true) tx := suite.CreateTestTx(signedContractTx, privKey, 1, false)
return tx return tx
}, },
true, false, true, 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 := evmtypes.NewTxContract(suite.app.EvmKeeper.ChainID(), 1, big.NewInt(10), 100000, big.NewInt(1), nil, nil, nil, nil)
signedContractTx.From = addr.Hex() signedContractTx.From = addr.Hex()
tx := suite.CreateTestTx(signedContractTx, privKey, 1, true) tx := suite.CreateTestTx(signedContractTx, privKey, 1, false)
return tx return tx
}, },
false, true, true, 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 := evmtypes.NewTx(suite.app.EvmKeeper.ChainID(), 1, &to, big.NewInt(10), 100000, big.NewInt(1), nil, nil, nil, nil)
signedTx.From = addr.Hex() signedTx.From = addr.Hex()
tx := suite.CreateTestTx(signedTx, privKey, 1, true) tx := suite.CreateTestTx(signedTx, privKey, 1, false)
return tx return tx
}, },
false, false, true, 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 := evmtypes.NewTx(suite.app.EvmKeeper.ChainID(), 2, &to, big.NewInt(10), 100000, big.NewInt(1), nil, nil, nil, nil)
signedTx.From = addr.Hex() signedTx.From = addr.Hex()
tx := suite.CreateTestTx(signedTx, privKey, 1, true) tx := suite.CreateTestTx(signedTx, privKey, 1, false)
return tx return tx
}, },
true, false, true, 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 := evmtypes.NewTx(suite.app.EvmKeeper.ChainID(), 3, &to, big.NewInt(10), 100000, big.NewInt(1), nil, nil, nil, nil)
signedTx.From = addr.Hex() signedTx.From = addr.Hex()
tx := suite.CreateTestTx(signedTx, privKey, 1, true) tx := suite.CreateTestTx(signedTx, privKey, 1, false)
return tx return tx
}, false, true, true, }, 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 := evmtypes.NewTx(suite.app.EvmKeeper.ChainID(), 5, &to, big.NewInt(10), 100000, big.NewInt(1), nil, nil, nil, nil)
signedTx.From = addr.Hex() signedTx.From = addr.Hex()
txBuilder := suite.CreateTestTxBuilder(signedTx, privKey, 1, true) txBuilder := suite.CreateTestTxBuilder(signedTx, privKey, 1, false)
txBuilder.SetMemo(strings.Repeat("*", 257)) txBuilder.SetMemo(strings.Repeat("*", 257))
return txBuilder.GetTx() return txBuilder.GetTx()
}, true, false, false, }, 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 { for _, tc := range testCases {
@ -188,7 +264,7 @@ func (suite AnteTestSuite) TestAnteHandlerWithDynamicTxFee() {
) )
signedContractTx.From = addr.Hex() signedContractTx.From = addr.Hex()
tx := suite.CreateTestTx(signedContractTx, privKey, 1, true) tx := suite.CreateTestTx(signedContractTx, privKey, 1, false)
return tx return tx
}, },
false, false, true, false, false, true,
@ -210,7 +286,7 @@ func (suite AnteTestSuite) TestAnteHandlerWithDynamicTxFee() {
) )
signedContractTx.From = addr.Hex() signedContractTx.From = addr.Hex()
tx := suite.CreateTestTx(signedContractTx, privKey, 1, true) tx := suite.CreateTestTx(signedContractTx, privKey, 1, false)
return tx return tx
}, },
true, false, true, true, false, true,
@ -232,7 +308,7 @@ func (suite AnteTestSuite) TestAnteHandlerWithDynamicTxFee() {
) )
signedContractTx.From = addr.Hex() signedContractTx.From = addr.Hex()
tx := suite.CreateTestTx(signedContractTx, privKey, 1, true) tx := suite.CreateTestTx(signedContractTx, privKey, 1, false)
return tx return tx
}, },
false, true, true, false, true, true,
@ -255,7 +331,7 @@ func (suite AnteTestSuite) TestAnteHandlerWithDynamicTxFee() {
) )
signedTx.From = addr.Hex() signedTx.From = addr.Hex()
tx := suite.CreateTestTx(signedTx, privKey, 1, true) tx := suite.CreateTestTx(signedTx, privKey, 1, false)
return tx return tx
}, },
false, false, true, false, false, true,
@ -278,7 +354,7 @@ func (suite AnteTestSuite) TestAnteHandlerWithDynamicTxFee() {
) )
signedTx.From = addr.Hex() signedTx.From = addr.Hex()
tx := suite.CreateTestTx(signedTx, privKey, 1, true) tx := suite.CreateTestTx(signedTx, privKey, 1, false)
return tx return tx
}, },
true, false, true, true, false, true,
@ -301,7 +377,7 @@ func (suite AnteTestSuite) TestAnteHandlerWithDynamicTxFee() {
) )
signedTx.From = addr.Hex() signedTx.From = addr.Hex()
tx := suite.CreateTestTx(signedTx, privKey, 1, true) tx := suite.CreateTestTx(signedTx, privKey, 1, false)
return tx return tx
}, false, true, true, }, false, true, true,
}, },
@ -369,7 +445,7 @@ func (suite AnteTestSuite) TestAnteHandlerWithDynamicTxFee() {
) )
signedTx.From = addr.Hex() signedTx.From = addr.Hex()
txBuilder := suite.CreateTestTxBuilder(signedTx, privKey, 1, true) txBuilder := suite.CreateTestTxBuilder(signedTx, privKey, 1, false)
txBuilder.SetMemo(strings.Repeat("*", 257)) txBuilder.SetMemo(strings.Repeat("*", 257))
return txBuilder.GetTx() return txBuilder.GetTx()
}, true, false, false, }, true, false, false,

View File

@ -8,6 +8,7 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types" sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" 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" authante "github.com/cosmos/cosmos-sdk/x/auth/ante"
ethermint "github.com/tharsis/ethermint/types" ethermint "github.com/tharsis/ethermint/types"
@ -35,6 +36,10 @@ type EVMKeeper interface {
) (sdk.Coins, error) ) (sdk.Coins, error)
} }
type protoTxProvider interface {
GetProtoTx() *tx.Tx
}
// EthSigVerificationDecorator validates an ethereum signatures // EthSigVerificationDecorator validates an ethereum signatures
type EthSigVerificationDecorator struct { type EthSigVerificationDecorator struct {
evmKeeper EVMKeeper 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 // EthValidateBasicDecorator is adapted from ValidateBasicDecorator from cosmos-sdk, it ignores ErrNoSignatures
type EthValidateBasicDecorator struct{} type EthValidateBasicDecorator struct {
evmKeeper EVMKeeper
}
// NewEthValidateBasicDecorator creates a new EthValidateBasicDecorator // NewEthValidateBasicDecorator creates a new EthValidateBasicDecorator
func NewEthValidateBasicDecorator() EthValidateBasicDecorator { func NewEthValidateBasicDecorator(ek EVMKeeper) EthValidateBasicDecorator {
return EthValidateBasicDecorator{} return EthValidateBasicDecorator{
evmKeeper: ek,
}
} }
// AnteHandle handles basic validation of tx // 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") 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) return next(ctx, tx, simulate)
} }

45
app/ante/sigs_test.go Normal file
View File

@ -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)
}