Skip to content
This repository has been archived by the owner on Apr 4, 2024. It is now read-only.

fix: Some fields in transaction are not authenticated by signature #703

Merged
merged 1 commit into from
Nov 8, 2021
Merged

fix: Some fields in transaction are not authenticated by signature #703

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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.


2 changes: 1 addition & 1 deletion app/ante/ante.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ func NewAnteHandler(
authante.NewMempoolFeeDecorator(),
authante.NewTxTimeoutHeightDecorator(),
authante.NewValidateMemoDecorator(ak),
NewEthValidateBasicDecorator(),
NewEthValidateBasicDecorator(evmKeeper),
NewEthSigVerificationDecorator(evmKeeper),
NewEthAccountVerificationDecorator(ak, bankKeeper, evmKeeper),
NewEthNonceVerificationDecorator(ak),
Expand Down
104 changes: 90 additions & 14 deletions app/ante/ante_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
},
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
},
Expand Down Expand Up @@ -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,
Expand Down
96 changes: 93 additions & 3 deletions app/ante/eth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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)
}

Expand Down
Loading