From 1f4f7fb017db8f93d8e99665c709961111a33328 Mon Sep 17 00:00:00 2001
From: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
Date: Wed, 23 Sep 2020 17:49:20 +0300
Subject: [PATCH] ante: add message validation decorator (#525)

* ante: add message validation decorator

* changelog

* fix lint

* fix lint

* rename
---
 CHANGELOG.md     |  1 +
 app/ante/ante.go | 75 ++++++++++++++----------------------------------
 app/ante/eth.go  | 26 ++++++++++-------
 3 files changed, 39 insertions(+), 63 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index cdc18433..6749cfde 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -55,6 +55,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### Bug Fixes
 
+* (ante) [\#525](https://github.com/ChainSafe/ethermint/pull/525) Add message validation decorator to `AnteHandler` for `MsgEthereumTx`.
 * (types) [\#507](https://github.com/ChainSafe/ethermint/pull/507) Fix hardcoded `aphoton` on `EthAccount` balance getter and setter.
 * (`x/evm`) [\#496](https://github.com/ChainSafe/ethermint/pull/496) Fix bugs on `journal.revert` and `CommitStateDB.Copy`.
 * (types) [\#480](https://github.com/ChainSafe/ethermint/pull/480) Update [BIP44](https://github.com/bitcoin/bips/blob/master/bip-0044.mediawiki) coin type to `60` to satisfy [EIP84](https://github.com/ethereum/EIPs/issues/84).
diff --git a/app/ante/ante.go b/app/ante/ante.go
index 8015b152..340f5b61 100644
--- a/app/ante/ante.go
+++ b/app/ante/ante.go
@@ -5,6 +5,7 @@ import (
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
 	"github.com/cosmos/cosmos-sdk/x/auth"
 	authante "github.com/cosmos/cosmos-sdk/x/auth/ante"
+	"github.com/cosmos/cosmos-sdk/x/auth/keeper"
 	"github.com/cosmos/cosmos-sdk/x/auth/types"
 
 	"github.com/cosmos/ethermint/crypto"
@@ -54,6 +55,7 @@ func NewAnteHandler(ak auth.AccountKeeper, evmKeeper EVMKeeper, sk types.SupplyK
 			anteHandler = sdk.ChainAnteDecorators(
 				NewEthSetupContextDecorator(), // outermost AnteDecorator. EthSetUpContext must be called first
 				NewEthMempoolFeeDecorator(evmKeeper),
+				authante.NewValidateBasicDecorator(),
 				NewEthSigVerificationDecorator(),
 				NewAccountVerificationDecorator(ak, evmKeeper),
 				NewNonceVerificationDecorator(ak),
@@ -71,7 +73,7 @@ func NewAnteHandler(ak auth.AccountKeeper, evmKeeper EVMKeeper, sk types.SupplyK
 // sigGasConsumer overrides the DefaultSigVerificationGasConsumer from the x/auth
 // module on the SDK. It doesn't allow ed25519 nor multisig thresholds.
 func sigGasConsumer(
-	meter sdk.GasMeter, sig []byte, pubkey tmcrypto.PubKey, params types.Params,
+	meter sdk.GasMeter, _ []byte, pubkey tmcrypto.PubKey, _ types.Params,
 ) error {
 	switch pubkey.(type) {
 	case crypto.PubKeySecp256k1:
@@ -85,75 +87,42 @@ func sigGasConsumer(
 	}
 }
 
-// IncrementSequenceDecorator handles incrementing sequences of all signers.
-// Use the IncrementSequenceDecorator decorator to prevent replay attacks. Note,
-// there is no need to execute IncrementSequenceDecorator on RecheckTX since
-// CheckTx would already bump the sequence number.
-//
-// NOTE: Since CheckTx and DeliverTx state are managed separately, subsequent and
-// sequential txs orginating from the same account cannot be handled correctly in
-// a reliable way unless sequence numbers are managed and tracked manually by a
-// client. It is recommended to instead use multiple messages in a tx.
-type IncrementSequenceDecorator struct {
-	ak auth.AccountKeeper
-}
-
-func NewIncrementSequenceDecorator(ak auth.AccountKeeper) IncrementSequenceDecorator {
-	return IncrementSequenceDecorator{
-		ak: ak,
-	}
-}
-
-func (isd IncrementSequenceDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) {
-	// no need to increment sequence on RecheckTx
-	if ctx.IsReCheckTx() && !simulate {
-		return next(ctx, tx, simulate)
-	}
-
-	sigTx, ok := tx.(authante.SigVerifiableTx)
-	if !ok {
-		return ctx, sdkerrors.Wrap(sdkerrors.ErrTxDecode, "invalid transaction type")
-	}
-
-	// increment sequence of all signers
-	for _, addr := range sigTx.GetSigners() {
-		acc := isd.ak.GetAccount(ctx, addr)
-		if err := acc.SetSequence(acc.GetSequence() + 1); err != nil {
-			panic(err)
-		}
-
-		isd.ak.SetAccount(ctx, acc)
-	}
-
-	return next(ctx, tx, simulate)
-}
-
+// AccountSetupDecorator sets an account to state if it's not stored already. This only applies for MsgEthermint.
 type AccountSetupDecorator struct {
 	ak auth.AccountKeeper
 }
 
+// NewAccountSetupDecorator creates a new AccountSetupDecorator instance
 func NewAccountSetupDecorator(ak auth.AccountKeeper) AccountSetupDecorator {
 	return AccountSetupDecorator{
 		ak: ak,
 	}
 }
 
+// AnteHandle sets an account for MsgEthermint (evm) if the sender is registered.
+// NOTE: Since the account is set without any funds, the message execution will
+// fail if the validator requires a minimum fee > 0.
 func (asd AccountSetupDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) {
 	msgs := tx.GetMsgs()
 	if len(msgs) == 0 {
 		return ctx, sdkerrors.Wrap(sdkerrors.ErrUnknownRequest, "no messages included in transaction")
 	}
 
-	msg, ok := msgs[0].(evmtypes.MsgEthermint)
-	if !ok {
-		return next(ctx, tx, simulate)
-	}
-
-	acc := asd.ak.GetAccount(ctx, msg.From)
-	if acc == nil {
-		info := asd.ak.NewAccountWithAddress(ctx, msg.From)
-		asd.ak.SetAccount(ctx, info)
+	for _, msg := range msgs {
+		if msgEthermint, ok := msg.(evmtypes.MsgEthermint); ok {
+			setupAccount(asd.ak, ctx, msgEthermint.From)
+		}
 	}
 
 	return next(ctx, tx, simulate)
 }
+
+func setupAccount(ak keeper.AccountKeeper, ctx sdk.Context, addr sdk.AccAddress) {
+	acc := ak.GetAccount(ctx, addr)
+	if acc != nil {
+		return
+	}
+
+	acc = ak.NewAccountWithAddress(ctx, addr)
+	ak.SetAccount(ctx, acc)
+}
diff --git a/app/ante/eth.go b/app/ante/eth.go
index 424f4acf..bb16afbc 100644
--- a/app/ante/eth.go
+++ b/app/ante/eth.go
@@ -146,10 +146,10 @@ func (esvd EthSigVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, s
 		return ctx, sdkerrors.Wrap(emint.ErrInvalidChainID, ctx.ChainID())
 	}
 
-	// validate sender/signature
+	// validate sender/signature and cache the address
 	_, err = msgEthTx.VerifySig(chainID)
 	if err != nil {
-		return ctx, sdkerrors.Wrap(sdkerrors.ErrUnauthorized, fmt.Sprintf("signature verification failed: %s", err.Error()))
+		return ctx, sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "signature verification failed: %s", err.Error())
 	}
 
 	// NOTE: when signature verification succeeds, a non-empty signer address can be
@@ -217,7 +217,7 @@ func (avd AccountVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, s
 	return next(ctx, tx, simulate)
 }
 
-// NonceVerificationDecorator that the account nonce from the transaction matches
+// NonceVerificationDecorator checks that the account nonce from the transaction matches
 // the sender account sequence.
 type NonceVerificationDecorator struct {
 	ak auth.AccountKeeper
@@ -246,14 +246,17 @@ func (nvd NonceVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, sim
 
 	acc := nvd.ak.GetAccount(ctx, address)
 	if acc == nil {
-		return ctx, fmt.Errorf("account %s (%s) is nil", common.BytesToAddress(address.Bytes()), address)
+		return ctx, sdkerrors.Wrapf(
+			sdkerrors.ErrUnknownAddress,
+			"account %s (%s) is nil", common.BytesToAddress(address.Bytes()), address,
+		)
 	}
 
 	seq := acc.GetSequence()
 	if msgEthTx.Data.AccountNonce != seq {
-		return ctx, sdkerrors.Wrap(
+		return ctx, sdkerrors.Wrapf(
 			sdkerrors.ErrInvalidSequence,
-			fmt.Sprintf("invalid nonce; got %d, expected %d", msgEthTx.Data.AccountNonce, seq),
+			"invalid nonce; got %d, expected %d", msgEthTx.Data.AccountNonce, seq,
 		)
 	}
 
@@ -290,20 +293,23 @@ func (egcd EthGasConsumeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula
 		return ctx, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "invalid transaction type: %T", tx)
 	}
 
-	// sender address should be in the tx cache
+	// sender address should be in the tx cache from the previous AnteHandle call
 	address := msgEthTx.From()
 	if address.Empty() {
 		panic("sender address cannot be empty")
 	}
 
-	// Fetch sender account from signature
+	// fetch sender account from signature
 	senderAcc, err := auth.GetSignerAcc(ctx, egcd.ak, address)
 	if err != nil {
 		return ctx, err
 	}
 
 	if senderAcc == nil {
-		return ctx, fmt.Errorf("sender account %s (%s) is nil", common.BytesToAddress(address.Bytes()), address)
+		return ctx, sdkerrors.Wrapf(
+			sdkerrors.ErrUnknownAddress,
+			"sender account %s (%s) is nil", common.BytesToAddress(address.Bytes()), address,
+		)
 	}
 
 	gasLimit := msgEthTx.GetGas()
@@ -314,7 +320,7 @@ func (egcd EthGasConsumeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula
 
 	// intrinsic gas verification during CheckTx
 	if ctx.IsCheckTx() && gasLimit < gas {
-		return ctx, fmt.Errorf("intrinsic gas too low: %d < %d", gasLimit, gas)
+		return ctx, sdkerrors.Wrapf(sdkerrors.ErrOutOfGas, "intrinsic gas too low: %d < %d", gasLimit, gas)
 	}
 
 	// Charge sender for gas up to limit