feat!: Reject not replay-protected tx to prevent replay attack

CHANGELOG.md

@@ -38,6 +38,10 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ## Unreleased
 
+### State Machine Breaking
+
+* (evm) [tharsis#1124](https://github.com/tharsis/ethermint/pull/1124) Reject non-replay-protected tx in ante handler to prevent replay attack
+
 ### Bug Fixes
 
 * (evm) [tharsis#1118](https://github.com/tharsis/ethermint/pull/1118) Fix `Type()` `Account` method `EmptyCodeHash` comparison

app/ante/eth.go

@@ -50,7 +50,12 @@ func (esvd EthSigVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, s
 			return ctx, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "invalid message type %T, expected %T", msg, (*evmtypes.MsgEthereumTx)(nil))
 		}
 
-		sender, err := signer.Sender(msgEthTx.AsTransaction())
+		ethTx := msgEthTx.AsTransaction()
+		if params.RejectUnprotectedTx && !ethTx.Protected() {
+			return ctx, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "eth tx is not replay-protected")
+		}
+
+		sender, err := signer.Sender(ethTx)
 		if err != nil {
 			return ctx, sdkerrors.Wrapf(
 				sdkerrors.ErrorInvalidSigner,

app/ante/eth_test.go

@@ -15,33 +15,46 @@ import (
 )
 
 func (suite AnteTestSuite) TestEthSigVerificationDecorator() {
-	dec := ante.NewEthSigVerificationDecorator(suite.app.EvmKeeper)
 	addr, privKey := tests.NewAddrKey()
 
 	signedTx := evmtypes.NewTxContract(suite.app.EvmKeeper.ChainID(), 1, big.NewInt(10), 1000, big.NewInt(1), nil, nil, nil, nil)
 	signedTx.From = addr.Hex()
 	err := signedTx.Sign(suite.ethSigner, tests.NewSigner(privKey))
 	suite.Require().NoError(err)
 
+	unprotectedTx := evmtypes.NewTxContract(nil, 1, big.NewInt(10), 1000, big.NewInt(1), nil, nil, nil, nil)
+	unprotectedTx.From = addr.Hex()
+	err = unprotectedTx.Sign(ethtypes.HomesteadSigner{}, tests.NewSigner(privKey))
+	suite.Require().NoError(err)
+
 	testCases := []struct {
-		name      string
-		tx        sdk.Tx
-		reCheckTx bool
-		expPass   bool
+		name                string
+		tx                  sdk.Tx
+		rejectUnprotectedTx bool
+		reCheckTx           bool
+		expPass             bool
 	}{
-		{"ReCheckTx", &invalidTx{}, true, false},
-		{"invalid transaction type", &invalidTx{}, false, false},
+		{"ReCheckTx", &invalidTx{}, true, true, false},
+		{"invalid transaction type", &invalidTx{}, true, false, false},
 		{
 			"invalid sender",
 			evmtypes.NewTx(suite.app.EvmKeeper.ChainID(), 1, &addr, big.NewInt(10), 1000, big.NewInt(1), nil, nil, nil, nil),
+			true,
 			false,
 			false,
 		},
-		{"successful signature verification", signedTx, false, true},
+		{"successful signature verification", signedTx, true, false, true},
+		{"invalid, not replay-protected", unprotectedTx, true, false, false},
+		{"successful, don't reject unprotected", unprotectedTx, false, false, true},
 	}
 
 	for _, tc := range testCases {
 		suite.Run(tc.name, func() {
+			suite.evmParamsOption = func(params *evmtypes.Params) {
+				params.RejectUnprotectedTx = tc.rejectUnprotectedTx
+			}
+			suite.SetupTest()
+			dec := ante.NewEthSigVerificationDecorator(suite.app.EvmKeeper)
 			_, err := dec.AnteHandle(suite.ctx.WithIsReCheckTx(tc.reCheckTx), tc.tx, false, NextFn)
 
 			if tc.expPass {
@@ -51,6 +64,7 @@ func (suite AnteTestSuite) TestEthSigVerificationDecorator() {
 			}
 		})
 	}
+	suite.evmParamsOption = nil
 }
 
 func (suite AnteTestSuite) TestNewEthAccountVerificationDecorator() {

app/ante/utils_test.go

@@ -76,6 +76,7 @@ func (suite *AnteTestSuite) SetupTest() {
 			genesis[feemarkettypes.ModuleName] = app.AppCodec().MustMarshalJSON(feemarketGenesis)
 		}
 		evmGenesis := evmtypes.DefaultGenesisState()
+		evmGenesis.Params.RejectUnprotectedTx = true
 		if !suite.enableLondonHF {
 			maxInt := sdk.NewInt(math.MaxInt64)
 			evmGenesis.Params.ChainConfig.LondonBlock = &maxInt

docs/api/proto-docs.md

@@ -227,6 +227,7 @@ Params defines the EVM module parameters
 | `enable_call` | [bool](#bool) |  | enable call toggles state transitions that use the vm.Call function |
 | `extra_eips` | [int64](#int64) | repeated | extra eips defines the additional EIPs for the vm.Config |
 | `chain_config` | [ChainConfig](#ethermint.evm.v1.ChainConfig) |  | chain config defines the EVM chain configuration parameters |
+| `reject_unprotected_tx` | [bool](#bool) |  | reject replay-unprotected transactions |
 
 
 
@@ -443,6 +444,7 @@ DynamicFeeTx is the data of EIP-1559 dinamic fee transactions.
 
 ### LegacyTx
 LegacyTx is the transaction data of regular Ethereum transactions.
+NOTE: All non-protected transactions (i.e non EIP155 signed) will fail if the RejectUnprotectedTx parameter is enabled.
 
 
 | Field | Type | Label | Description |

proto/ethermint/evm/v1/evm.proto

@@ -24,6 +24,8 @@ message Params {
     (gogoproto.moretags) = "yaml:\"chain_config\"",
     (gogoproto.nullable) = false
   ];
+  // reject replay-unprotected transactions
+  bool reject_unprotected_tx = 6;
 }
 
 // ChainConfig defines the Ethereum ChainConfig parameters using *sdk.Int values

proto/ethermint/evm/v1/tx.proto

@@ -36,6 +36,7 @@ message MsgEthereumTx {
 }
 
 // LegacyTx is the transaction data of regular Ethereum transactions.
+// NOTE: All non-protected transactions (i.e non EIP155 signed) will fail if the RejectUnprotectedTx parameter is enabled. 
 message LegacyTx {
   option (gogoproto.goproto_getters) = false;
   option (cosmos_proto.implements_interface) = "TxData";

x/evm/keeper/migrations.go

@@ -1,5 +1,10 @@
 package keeper
 
+import (
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	v2 "github.com/tharsis/ethermint/x/evm/migrations/v2"
+)
+
 // Migrator is a struct for handling in-place store migrations.
 type Migrator struct {
 	keeper Keeper
@@ -11,3 +16,8 @@ func NewMigrator(keeper Keeper) Migrator {
 		keeper: keeper,
 	}
 }
+
+// Migrate1to2 migrates the store from consensus version v1 to v2
+func (m Migrator) Migrate1to2(ctx sdk.Context) error {
+	return v2.MigrateStore(ctx, &m.keeper.paramSpace)
+}

x/evm/migrations/v2/migrate.go

@@ -0,0 +1,19 @@
+package v2
+
+import (
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	paramtypes "github.com/cosmos/cosmos-sdk/x/params/types"
+	"github.com/tharsis/ethermint/x/evm/types"
+)
+
+// MigrateStore add the default RejectUnprotected parameter.
+func MigrateStore(ctx sdk.Context, paramstore *paramtypes.Subspace) error {
+	if !paramstore.HasKeyTable() {
+		ps := paramstore.WithKeyTable(types.ParamKeyTable())
+		paramstore = &ps
+	}
+
+	// add RejectUnprotected
+	paramstore.Set(ctx, types.ParamStoreKeyRejectUnprotectedTx, types.DefaultParams().RejectUnprotectedTx)
+	return nil
+}

x/evm/migrations/v2/migrate_test.go

@@ -0,0 +1,47 @@
+package v2_test
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/cosmos/cosmos-sdk/testutil"
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	paramtypes "github.com/cosmos/cosmos-sdk/x/params/types"
+
+	"github.com/tharsis/ethermint/encoding"
+
+	"github.com/tharsis/ethermint/app"
+	v2 "github.com/tharsis/ethermint/x/evm/migrations/v2"
+	v2types "github.com/tharsis/ethermint/x/evm/migrations/v2/types"
+	"github.com/tharsis/ethermint/x/evm/types"
+)
+
+func TestMigrateStore(t *testing.T) {
+	encCfg := encoding.MakeConfig(app.ModuleBasics)
+	feemarketKey := sdk.NewKVStoreKey(types.StoreKey)
+	tFeeMarketKey := sdk.NewTransientStoreKey(fmt.Sprintf("%s_test", types.StoreKey))
+	ctx := testutil.DefaultContext(feemarketKey, tFeeMarketKey)
+	paramstore := paramtypes.NewSubspace(
+		encCfg.Marshaler, encCfg.Amino, feemarketKey, tFeeMarketKey, "evm",
+	).WithKeyTable(v2types.ParamKeyTable())
+
+	params := v2types.DefaultParams()
+	paramstore.SetParamSet(ctx, &params)
+
+	require.Panics(t, func() {
+		var result bool
+		paramstore.Get(ctx, types.ParamStoreKeyRejectUnprotectedTx, &result)
+	})
+
+	paramstore = paramtypes.NewSubspace(
+		encCfg.Marshaler, encCfg.Amino, feemarketKey, tFeeMarketKey, "evm",
+	).WithKeyTable(types.ParamKeyTable())
+	err := v2.MigrateStore(ctx, &paramstore)
+	require.NoError(t, err)
+
+	var result bool
+	paramstore.Get(ctx, types.ParamStoreKeyRejectUnprotectedTx, &result)
+	require.Equal(t, types.DefaultRejectUnprotectedTx, result)
+}

x/evm/migrations/v2/types/chain_config.go

@@ -0,0 +1,165 @@
+package types
+
+import (
+	"math/big"
+	"strings"
+
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
+
+	"github.com/ethereum/go-ethereum/common"
+	"github.com/ethereum/go-ethereum/params"
+
+	"github.com/tharsis/ethermint/x/evm/types"
+)
+
+// EthereumConfig returns an Ethereum ChainConfig for EVM state transitions.
+// All the negative or nil values are converted to nil
+func (cc ChainConfig) EthereumConfig(chainID *big.Int) *params.ChainConfig {
+	return &params.ChainConfig{
+		ChainID:                 chainID,
+		HomesteadBlock:          getBlockValue(cc.HomesteadBlock),
+		DAOForkBlock:            getBlockValue(cc.DAOForkBlock),
+		DAOForkSupport:          cc.DAOForkSupport,
+		EIP150Block:             getBlockValue(cc.EIP150Block),
+		EIP150Hash:              common.HexToHash(cc.EIP150Hash),
+		EIP155Block:             getBlockValue(cc.EIP155Block),
+		EIP158Block:             getBlockValue(cc.EIP158Block),
+		ByzantiumBlock:          getBlockValue(cc.ByzantiumBlock),
+		ConstantinopleBlock:     getBlockValue(cc.ConstantinopleBlock),
+		PetersburgBlock:         getBlockValue(cc.PetersburgBlock),
+		IstanbulBlock:           getBlockValue(cc.IstanbulBlock),
+		MuirGlacierBlock:        getBlockValue(cc.MuirGlacierBlock),
+		BerlinBlock:             getBlockValue(cc.BerlinBlock),
+		LondonBlock:             getBlockValue(cc.LondonBlock),
+		ArrowGlacierBlock:       getBlockValue(cc.ArrowGlacierBlock),
+		MergeForkBlock:          getBlockValue(cc.MergeForkBlock),
+		TerminalTotalDifficulty: nil,
+		Ethash:                  nil,
+		Clique:                  nil,
+	}
+}
+
+// DefaultChainConfig returns default evm parameters.
+func DefaultChainConfig() ChainConfig {
+	homesteadBlock := sdk.ZeroInt()
+	daoForkBlock := sdk.ZeroInt()
+	eip150Block := sdk.ZeroInt()
+	eip155Block := sdk.ZeroInt()
+	eip158Block := sdk.ZeroInt()
+	byzantiumBlock := sdk.ZeroInt()
+	constantinopleBlock := sdk.ZeroInt()
+	petersburgBlock := sdk.ZeroInt()
+	istanbulBlock := sdk.ZeroInt()
+	muirGlacierBlock := sdk.ZeroInt()
+	berlinBlock := sdk.ZeroInt()
+	londonBlock := sdk.ZeroInt()
+	arrowGlacierBlock := sdk.ZeroInt()
+	mergeForkBlock := sdk.ZeroInt()
+
+	return ChainConfig{
+		HomesteadBlock:      &homesteadBlock,
+		DAOForkBlock:        &daoForkBlock,
+		DAOForkSupport:      true,
+		EIP150Block:         &eip150Block,
+		EIP150Hash:          common.Hash{}.String(),
+		EIP155Block:         &eip155Block,
+		EIP158Block:         &eip158Block,
+		ByzantiumBlock:      &byzantiumBlock,
+		ConstantinopleBlock: &constantinopleBlock,
+		PetersburgBlock:     &petersburgBlock,
+		IstanbulBlock:       &istanbulBlock,
+		MuirGlacierBlock:    &muirGlacierBlock,
+		BerlinBlock:         &berlinBlock,
+		LondonBlock:         &londonBlock,
+		ArrowGlacierBlock:   &arrowGlacierBlock,
+		MergeForkBlock:      &mergeForkBlock,
+	}
+}
+
+func getBlockValue(block *sdk.Int) *big.Int {
+	if block == nil || block.IsNegative() {
+		return nil
+	}
+
+	return block.BigInt()
+}
+
+// Validate performs a basic validation of the ChainConfig params. The function will return an error
+// if any of the block values is uninitialized (i.e nil) or if the EIP150Hash is an invalid hash.
+func (cc ChainConfig) Validate() error {
+	if err := validateBlock(cc.HomesteadBlock); err != nil {
+		return sdkerrors.Wrap(err, "homesteadBlock")
+	}
+	if err := validateBlock(cc.DAOForkBlock); err != nil {
+		return sdkerrors.Wrap(err, "daoForkBlock")
+	}
+	if err := validateBlock(cc.EIP150Block); err != nil {
+		return sdkerrors.Wrap(err, "eip150Block")
+	}
+	if err := validateHash(cc.EIP150Hash); err != nil {
+		return err
+	}
+	if err := validateBlock(cc.EIP155Block); err != nil {
+		return sdkerrors.Wrap(err, "eip155Block")
+	}
+	if err := validateBlock(cc.EIP158Block); err != nil {
+		return sdkerrors.Wrap(err, "eip158Block")
+	}
+	if err := validateBlock(cc.ByzantiumBlock); err != nil {
+		return sdkerrors.Wrap(err, "byzantiumBlock")
+	}
+	if err := validateBlock(cc.ConstantinopleBlock); err != nil {
+		return sdkerrors.Wrap(err, "constantinopleBlock")
+	}
+	if err := validateBlock(cc.PetersburgBlock); err != nil {
+		return sdkerrors.Wrap(err, "petersburgBlock")
+	}
+	if err := validateBlock(cc.IstanbulBlock); err != nil {
+		return sdkerrors.Wrap(err, "istanbulBlock")
+	}
+	if err := validateBlock(cc.MuirGlacierBlock); err != nil {
+		return sdkerrors.Wrap(err, "muirGlacierBlock")
+	}
+	if err := validateBlock(cc.BerlinBlock); err != nil {
+		return sdkerrors.Wrap(err, "berlinBlock")
+	}
+	if err := validateBlock(cc.LondonBlock); err != nil {
+		return sdkerrors.Wrap(err, "londonBlock")
+	}
+	if err := validateBlock(cc.ArrowGlacierBlock); err != nil {
+		return sdkerrors.Wrap(err, "arrowGlacierBlock")
+	}
+	if err := validateBlock(cc.MergeForkBlock); err != nil {
+		return sdkerrors.Wrap(err, "mergeForkBlock")
+	}
+
+	// NOTE: chain ID is not needed to check config order
+	if err := cc.EthereumConfig(nil).CheckConfigForkOrder(); err != nil {
+		return sdkerrors.Wrap(err, "invalid config fork order")
+	}
+	return nil
+}
+
+func validateHash(hex string) error {
+	if hex != "" && strings.TrimSpace(hex) == "" {
+		return sdkerrors.Wrap(types.ErrInvalidChainConfig, "hash cannot be blank")
+	}
+
+	return nil
+}
+
+func validateBlock(block *sdk.Int) error {
+	// nil value means that the fork has not yet been applied
+	if block == nil {
+		return nil
+	}
+
+	if block.IsNegative() {
+		return sdkerrors.Wrapf(
+			types.ErrInvalidChainConfig, "block value cannot be negative: %s", block,
+		)
+	}
+
+	return nil
+}