diff --git a/CHANGELOG.md b/CHANGELOG.md index 2294a0b7..769b4f51 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/app/ante/ante.go b/app/ante/ante.go index 9dba6cb5..8c274a04 100644 --- a/app/ante/ante.go +++ b/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) { diff --git a/app/ante/ante_test.go b/app/ante/ante_test.go index f4ef7b27..b9730ab6 100644 --- a/app/ante/ante_test.go +++ b/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 { diff --git a/app/ante/utils_test.go b/app/ante/utils_test.go index 81aa4db4..d8a7833f 100644 --- a/app/ante/utils_test.go +++ b/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) diff --git a/x/evm/keeper/keeper.go b/x/evm/keeper/keeper.go index 9c6945c0..2d44d0a4 100644 --- a/x/evm/keeper/keeper.go +++ b/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 diff --git a/x/evm/keeper/statedb.go b/x/evm/keeper/statedb.go index ac1cef66..4adb50ec 100644 --- a/x/evm/keeper/statedb.go +++ b/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(), diff --git a/x/evm/keeper/statedb_test.go b/x/evm/keeper/statedb_test.go index a3cb8caf..837234e8 100644 --- a/x/evm/keeper/statedb_test.go +++ b/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() {