Merge pull request from GHSA-mx3r-7hpq-fr4g

* reject invalid `MsgEthereumTx` wrapping tx

Update CHANGELOG.md

* added a unit test

* reject invalid `MsgEthereumTx` wrapping tx in a non-breaking way

Update CHANGELOG.md

* delete code and state on suicide

* fix suicide tests

* update changelog

* update changelog

* delete code hash on suicide

* simplifies delete code

* Apply suggestions from code review

* Update app/ante/ante.go

Co-authored-by: Tomas Tauber <2410580+tomtau@users.noreply.github.com>
Co-authored-by: Freddy Caceres <freddy.caceres@crypto.com>
Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>

CHANGELOG.md

@@ -43,6 +43,8 @@ Ref: https://keepachangelog.com/en/1.0.0/
 - (feemarket) [tharsis#822](https://github.com/tharsis/ethermint/pull/822) Update EIP1559 base fee in `BeginBlock`.
 - (evm) [tharsis#817](https://github.com/tharsis/ethermint/pull/817) Use `effectiveGasPrice` in ante handler, add `effectiveGasPrice` to tx receipt.
 - (evm) [tharsis#808](https://github.com/tharsis/ethermint/issues/808) increase nonce in ante handler for contract creation transaction.
+- (evm) [tharsis#N/A]() reject invalid `MsgEthereumTx` wrapping tx
+- (evm) [tharsis#N/A]() Fix SelfDestruct opcode by deleting account code and state
 
 ### Improvements
 

app/ante/ante.go

@@ -76,6 +76,17 @@ func NewAnteHandler(
 			}
 		}
 
+		// Reject messages that requires specific authentication here.
+		// For example `MsgEthereumTx` requires fee to be deducted in the antehandler in order to perform the refund.
+		for _, msg := range tx.GetMsgs() {
+			if _, ok := msg.(*evmtypes.MsgEthereumTx); ok {
+				return ctx, sdkerrors.Wrapf(
+					sdkerrors.ErrInvalidType,
+					"MsgEthereumTx needs to be contained within a tx with ExtensionOptionsEthereumTx option",
+				)
+			}
+		}
+
 		// handle as totally normal Cosmos SDK tx
 
 		switch tx.(type) {

app/ante/ante_test.go

@@ -206,6 +206,16 @@ func (suite AnteTestSuite) TestAnteHandler() {
 				return txBuilder.GetTx()
 			}, true, false, false,
 		},
+		{
+			"fail - CheckTx (ExtensionOptionsEthereumTx not set)",
+			func() sdk.Tx {
+				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, false, true)
+				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
 		{

app/ante/utils_test.go

@@ -84,6 +84,7 @@ func TestAnteTestSuite(t *testing.T) {
 // CreateTestTx is a helper function to create a tx given multiple inputs.
 func (suite *AnteTestSuite) CreateTestTx(
 	msg *evmtypes.MsgEthereumTx, priv cryptotypes.PrivKey, accNum uint64, signCosmosTx bool,
+	unsetExtensionOptions ...bool,
 ) authsigning.Tx {
 	return suite.CreateTestTxBuilder(msg, priv, accNum, signCosmosTx).GetTx()
 }
@@ -91,15 +92,22 @@ func (suite *AnteTestSuite) CreateTestTx(
 // CreateTestTxBuilder is a helper function to create a tx builder given multiple inputs.
 func (suite *AnteTestSuite) CreateTestTxBuilder(
 	msg *evmtypes.MsgEthereumTx, priv cryptotypes.PrivKey, accNum uint64, signCosmosTx bool,
+	unsetExtensionOptions ...bool,
 ) client.TxBuilder {
-	option, err := codectypes.NewAnyWithValue(&evmtypes.ExtensionOptionsEthereumTx{})
-	suite.Require().NoError(err)
+	var option *codectypes.Any
+	var err error
+	if len(unsetExtensionOptions) == 0 {
+		option, err = codectypes.NewAnyWithValue(&evmtypes.ExtensionOptionsEthereumTx{})
+		suite.Require().NoError(err)
+	}
 
 	txBuilder := suite.clientCtx.TxConfig.NewTxBuilder()
 	builder, ok := txBuilder.(authtx.ExtensionOptionsTxBuilder)
 	suite.Require().True(ok)
 
-	builder.SetExtensionOptions(option)
+	if len(unsetExtensionOptions) == 0 {
+		builder.SetExtensionOptions(option)
+	}
 
 	err = msg.Sign(suite.ethSigner, tests.NewSigner(priv))
 	suite.Require().NoError(err)

x/evm/keeper/keeper.go

@@ -1,7 +1,6 @@
 package keeper
 
 import (
-	"bytes"
 	"math/big"
 
 	"github.com/cosmos/cosmos-sdk/codec"
@@ -322,15 +321,9 @@ func (k Keeper) DeleteAccountStorage(addr common.Address) {
 }
 
 // DeleteCode removes the contract code byte array from the store associated with
-// the given address.
+// the given address and empties CodeHash on account.
 func (k Keeper) DeleteCode(addr common.Address) {
-	hash := k.GetCodeHash(addr)
-	if bytes.Equal(hash.Bytes(), common.BytesToHash(types.EmptyCodeHash).Bytes()) {
-		return
-	}
-
-	store := prefix.NewStore(k.Ctx().KVStore(k.storeKey), types.KeyPrefixCode)
-	store.Delete(hash.Bytes())
+	k.SetCode(addr, nil)
 }
 
 // ClearBalance subtracts the EVM all the balance denomination from the address

x/evm/keeper/statedb.go

@@ -543,9 +543,11 @@ func (k *Keeper) Suicide(addr common.Address) bool {
 		return false
 	}
 
-	// TODO: (@fedekunze) do we also need to delete the storage state and the code?
 	k.setSuicided(ctx, addr)
 
+	// delete account code and state
+	k.ResetAccount(addr)
+
 	k.Logger(ctx).Debug(
 		"account suicided",
 		"ethereum-address", addr.Hex(),

x/evm/keeper/statedb_test.go

@@ -403,20 +403,33 @@ func (suite *KeeperTestSuite) TestCommittedState() {
 }
 
 func (suite *KeeperTestSuite) TestSuicide() {
-	testCases := []struct {
-		name     string
-		suicided bool
-	}{
-		{"success, first time suicided", true},
-		{"success, already suicided", true},
+	code := []byte("code")
+	// Add code to account
+	suite.app.EvmKeeper.SetCode(suite.address, code)
+	suite.Require().Equal(code, suite.app.EvmKeeper.GetCode(suite.address))
+	// Add state to account
+	for i := 0; i < 5; i++ {
+		suite.app.EvmKeeper.SetState(suite.address, common.BytesToHash([]byte(fmt.Sprintf("key%d", i))), common.BytesToHash([]byte(fmt.Sprintf("value%d", i))))
 	}
 
-	for _, tc := range testCases {
-		suite.Run(tc.name, func() {
-			suite.Require().Equal(tc.suicided, suite.app.EvmKeeper.Suicide(suite.address))
-			suite.Require().Equal(tc.suicided, suite.app.EvmKeeper.HasSuicided(suite.address))
-		})
-	}
+	// Call Suicide
+	suite.Require().Equal(true, suite.app.EvmKeeper.Suicide(suite.address))
+
+	// Check suicided is marked
+	suite.Require().Equal(true, suite.app.EvmKeeper.HasSuicided(suite.address))
+	// Check code is deleted
+	suite.Require().Nil(suite.app.EvmKeeper.GetCode(suite.address))
+	// Check state is deleted
+	var storage types.Storage
+	err := suite.app.EvmKeeper.ForEachStorage(suite.address, func(key, value common.Hash) bool {
+		storage = append(storage, types.NewState(key, value))
+		return true
+	})
+	suite.Require().NoError(err)
+	suite.Require().Equal(0, len(storage))
+
+	// Check CodeHash is emptied
+	suite.Require().Equal(common.BytesToHash(types.EmptyCodeHash).Bytes(), suite.app.EvmKeeper.GetCodeHash(suite.address).Bytes())
 }
 
 func (suite *KeeperTestSuite) TestExist() {