!feat(evm): Reject not replay-protected tx to prevent replay attack (#…

…1124)

* Reject not replay-protected tx to prevent replay attack

Closes: #1122

- reject such txs in ante handler

* add reject unprotected parameter

* Update CHANGELOG.md

* Apply suggestions from code review

Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>

* pr suggestions

* add unit test case

* Reject not replay-protected tx to prevent replay attack

Closes: #1122

- reject such txs in ante handler

add reject unprotected parameter

Update CHANGELOG.md

Apply suggestions from code review

Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>

pr suggestions

add unit test case

use var

* add migrations

* Apply suggestions from code review

Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>

* rename

* update comments

Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>

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
+}