From 32f5e883b5c3010ebb19e1a090f049e4c4bc990e Mon Sep 17 00:00:00 2001 From: Nick DeLuca Date: Thu, 1 Aug 2024 02:44:56 +0000 Subject: [PATCH 1/4] feat!: Genesis Accounts can not contain empty code Genesis accounts currently serve no purpose for non-contract accounts. Currently, they can exist but code and storage will be empty and therefore has no effect besids ensuring the matching account is subject to similar validations as a contract. This change restrics valid genesis accounts to those with Code only. Storage can still be empty for contracts, as there is no requirement for values to be set in state for all contracts. --- x/evm/types/genesis.go | 17 +++++++++++++---- x/evm/types/genesis_test.go | 18 ++++++++++++++++-- 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/x/evm/types/genesis.go b/x/evm/types/genesis.go index e263ce695b..35f7bb3a66 100644 --- a/x/evm/types/genesis.go +++ b/x/evm/types/genesis.go @@ -21,7 +21,8 @@ import ( ethermint "github.com/evmos/ethermint/types" ) -// NewGenesisState creates a new genesis state. +// NewGenesisState creates a new genesis state with EVM configuration parameters +// and initial contract accounts. func NewGenesisState(params Params, accounts []GenesisAccount) *GenesisState { return &GenesisState{ Accounts: accounts, @@ -29,8 +30,8 @@ func NewGenesisState(params Params, accounts []GenesisAccount) *GenesisState { } } -// DefaultGenesisState sets default evm genesis state with empty accounts and default params and -// chain config values. +// DefaultGenesisState sets default evm genesis state with default parameters and +// no initial genesis accounts. func DefaultGenesisState() *GenesisState { return &GenesisState{ Accounts: []GenesisAccount{}, @@ -39,15 +40,23 @@ func DefaultGenesisState() *GenesisState { } // Validate performs a basic validation of a GenesisAccount fields. +// A valid genesis account has a valid address, non-empty code, and +// valid storage. func (ga GenesisAccount) Validate() error { if err := ethermint.ValidateAddress(ga.Address); err != nil { return err } + + if ga.Code == "" { + return fmt.Errorf("code can not be empty") + } + return ga.Storage.Validate() } // Validate performs basic genesis state validation returning an error upon any -// failure. +// failure. It ensures the params are valid and every genesis account is unique +// and valid. func (gs GenesisState) Validate() error { seenAccounts := make(map[string]struct{}) diff --git a/x/evm/types/genesis_test.go b/x/evm/types/genesis_test.go index 69c501f437..c078a32c71 100644 --- a/x/evm/types/genesis_test.go +++ b/x/evm/types/genesis_test.go @@ -79,7 +79,7 @@ func TestGenesisAccountValidate(t *testing.T) { expectedErr: "", }, { - name: "valid with empty code", + name: "invalid with empty code", getAccount: func() types.GenesisAccount { account := defaultGenesisAccount() @@ -87,7 +87,7 @@ func TestGenesisAccountValidate(t *testing.T) { return account }, - expectedErr: "", + expectedErr: "code can not be empty", }, } @@ -202,6 +202,20 @@ func TestGenesisStateValidate(t *testing.T) { }, expectedErr: "invalid address", }, + { + name: "genesis is invalid with empty code", + getState: func() *types.GenesisState { + state := types.DefaultGenesisState() + + account := defaultGenesisAccount() + account.Code = "" + + state.Accounts = append(state.Accounts, account) + + return state + }, + expectedErr: "code can not be empty", + }, { name: "genesis is invalid with an invalid genesis account storage key", getState: func() *types.GenesisState { From fff25e258ffcf4fcbf10e99b996854838f8df928 Mon Sep 17 00:00:00 2001 From: Nick DeLuca Date: Thu, 1 Aug 2024 03:10:13 +0000 Subject: [PATCH 2/4] refactor: Capitalize test case names Using capital letters for test case names improved readability when reading and parsing. --- x/evm/types/genesis_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/x/evm/types/genesis_test.go b/x/evm/types/genesis_test.go index c078a32c71..0aa4618a99 100644 --- a/x/evm/types/genesis_test.go +++ b/x/evm/types/genesis_test.go @@ -23,14 +23,14 @@ func TestGenesisAccountValidate(t *testing.T) { expectedErr string }{ { - name: "default is valid", + name: "Default is valid", getAccount: func() types.GenesisAccount { return defaultGenesisAccount() }, expectedErr: "", }, { - name: "invalid empty address", + name: "Invalid empty address", getAccount: func() types.GenesisAccount { account := defaultGenesisAccount() @@ -41,7 +41,7 @@ func TestGenesisAccountValidate(t *testing.T) { expectedErr: "invalid address", }, { - name: "invalid address length", + name: "Invalid address length", getAccount: func() types.GenesisAccount { account := defaultGenesisAccount() @@ -52,7 +52,7 @@ func TestGenesisAccountValidate(t *testing.T) { expectedErr: "invalid address", }, { - name: "invalid empty storage key", + name: "Invalid empty storage key", getAccount: func() types.GenesisAccount { account := defaultGenesisAccount() @@ -65,7 +65,7 @@ func TestGenesisAccountValidate(t *testing.T) { expectedErr: "state key hash cannot be blank", }, { - name: "valid with set storage state", + name: "Valid with set storage state", getAccount: func() types.GenesisAccount { account := defaultGenesisAccount() @@ -79,7 +79,7 @@ func TestGenesisAccountValidate(t *testing.T) { expectedErr: "", }, { - name: "invalid with empty code", + name: "Invalid with empty code", getAccount: func() types.GenesisAccount { account := defaultGenesisAccount() From f93633ddb3c365fd1df195f53fe4809034eea94d Mon Sep 17 00:00:00 2001 From: Nick DeLuca Date: Thu, 1 Aug 2024 03:21:38 +0000 Subject: [PATCH 3/4] fix!: Add back code hash check for empty code On some chains running ethermint, a self destruct bug that caused the code for other contract accounts to be deleted would result in exported genesis accounts with empty code. A patch was then added to genesis that allowed the hash check to be skipped in order for these genesis files to be re-imported successfully. This does not affect us contracts on Kava, so this check is being removed. --- x/evm/genesis.go | 3 +-- x/evm/genesis_test.go | 7 +++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/x/evm/genesis.go b/x/evm/genesis.go index 144d5a8ea4..bcf1af61a9 100644 --- a/x/evm/genesis.go +++ b/x/evm/genesis.go @@ -98,8 +98,7 @@ func InitGenesis( code := common.Hex2Bytes(account.Code) codeHash := crypto.Keccak256Hash(code) - // we ignore the empty Code hash checking, see ethermint PR#1234 - if len(account.Code) != 0 && !bytes.Equal(ethAcct.GetCodeHash().Bytes(), codeHash.Bytes()) { + if !bytes.Equal(ethAcct.GetCodeHash().Bytes(), codeHash.Bytes()) { s := "the evm state code doesn't match with the codehash\n" panic(fmt.Sprintf("%s account: %s , evm state codehash: %v, ethAccount codehash: %v, evm state code: %s\n", s, account.Address, codeHash, ethAcct.GetCodeHash(), account.Code)) diff --git a/x/evm/genesis_test.go b/x/evm/genesis_test.go index 15efae4da1..b82994c98f 100644 --- a/x/evm/genesis_test.go +++ b/x/evm/genesis_test.go @@ -242,7 +242,7 @@ func TestInitGenesis(t *testing.T) { }, }, { - name: "Does not panic when there is a code hash mismatch and matching genesis account contains no code", + name: "Panics when there is a code hash mismatch and matching genesis account contains no code", genFixture: func(t *testing.T, ctx sdk.Context, tApp *app.EthermintApp) testFixture { address := generateRandomAddress(t) @@ -261,12 +261,15 @@ func TestInitGenesis(t *testing.T) { } tApp.AccountKeeper.SetAccount(ctx, &acc) + s := "the evm state code doesn't match with the codehash\n" + expectedPanic := fmt.Sprintf("%s account: %s , evm state codehash: %v, ethAccount codehash: %v, evm state code: %s\n", s, address, common.BytesToHash(types.EmptyCodeHash), acc.GetCodeHash(), "") + return testFixture{ ctx: ctx, state: state, precompiles: nil, expectFunc: nil, - expectPanic: nil, + expectPanic: expectedPanic, } }, }, From b2308ed54c25166aea709040b0b64109dc273aab Mon Sep 17 00:00:00 2001 From: Nick DeLuca Date: Thu, 1 Aug 2024 15:26:49 +0000 Subject: [PATCH 4/4] feat!: Add strict contract account checks Since Genesis Accounts may only be contracts, we add the assertion that they must have a positive sequence (eip-155) and that they do not have a public key set. A contract can not be associated with a public key and must have a default nonce set. In addition, since enabled precompiles are contract accounts, we enforce that these have matching genesis accounts to share these validations and in addition, enforce a fixed code. --- x/evm/genesis.go | 18 ++++ x/evm/genesis_test.go | 173 ++++++++++++++++++++++++++++++++++++ x/evm/types/genesis.go | 6 ++ x/evm/types/genesis_test.go | 12 +++ 4 files changed, 209 insertions(+) diff --git a/x/evm/genesis.go b/x/evm/genesis.go index bcf1af61a9..a2ad2ff215 100644 --- a/x/evm/genesis.go +++ b/x/evm/genesis.go @@ -77,6 +77,11 @@ func InitGenesis( panic("the EVM module account has not been set") } + isEnabledPrecompile := make(map[string]struct{}) + for _, ep := range data.Params.EnabledPrecompiles { + isEnabledPrecompile[ep] = struct{}{} + } + for _, account := range data.Accounts { address := common.HexToAddress(account.Address) accAddress := sdk.AccAddress(address.Bytes()) @@ -95,9 +100,22 @@ func InitGenesis( ), ) } + + if ethAcct.GetSequence() == 0 { + panic(fmt.Errorf("account %s must have a positive nonce", account.Address)) + } + + if ethAcct.GetPubKey() != nil { + panic(fmt.Errorf("account %s must not have a public key set", account.Address)) + } + code := common.Hex2Bytes(account.Code) codeHash := crypto.Keccak256Hash(code) + if _, ok := isEnabledPrecompile[account.Address]; ok && !bytes.Equal(code, []byte{0x01}) { + panic(fmt.Errorf("enabled precompile %s must have code set to 0x01, got 0x%s", account.Address, account.Code)) + } + if !bytes.Equal(ethAcct.GetCodeHash().Bytes(), codeHash.Bytes()) { s := "the evm state code doesn't match with the codehash\n" panic(fmt.Sprintf("%s account: %s , evm state codehash: %v, ethAccount codehash: %v, evm state code: %s\n", diff --git a/x/evm/genesis_test.go b/x/evm/genesis_test.go index b82994c98f..58a986e348 100644 --- a/x/evm/genesis_test.go +++ b/x/evm/genesis_test.go @@ -8,6 +8,7 @@ import ( "cosmossdk.io/simapp" tmproto "github.com/cometbft/cometbft/proto/tendermint/types" + codectypes "github.com/cosmos/cosmos-sdk/codec/types" sdk "github.com/cosmos/cosmos-sdk/types" authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" "github.com/ethereum/go-ethereum/common" @@ -194,6 +195,7 @@ func TestInitGenesis(t *testing.T) { accAddr := sdk.AccAddress(common.HexToAddress(address).Bytes()) acc := authtypes.NewBaseAccountWithAddress(accAddr) + acc.Sequence = uint64(1) tApp.AccountKeeper.SetAccount(ctx, acc) return testFixture{ @@ -227,6 +229,7 @@ func TestInitGenesis(t *testing.T) { BaseAccount: authtypes.NewBaseAccountWithAddress(accAddr), CodeHash: incorrectCodeHash.String(), } + acc.Sequence = uint64(1) tApp.AccountKeeper.SetAccount(ctx, &acc) s := "the evm state code doesn't match with the codehash\n" @@ -259,6 +262,7 @@ func TestInitGenesis(t *testing.T) { BaseAccount: authtypes.NewBaseAccountWithAddress(accAddr), CodeHash: someCodeHash.String(), } + acc.Sequence = uint64(1) tApp.AccountKeeper.SetAccount(ctx, &acc) s := "the evm state code doesn't match with the codehash\n" @@ -293,6 +297,7 @@ func TestInitGenesis(t *testing.T) { BaseAccount: authtypes.NewBaseAccountWithAddress(accAddr), CodeHash: "", // we do not allow empty code hash when code is set } + acc.Sequence = uint64(1) tApp.AccountKeeper.SetAccount(ctx, &acc) s := "the evm state code doesn't match with the codehash\n" @@ -327,6 +332,7 @@ func TestInitGenesis(t *testing.T) { BaseAccount: authtypes.NewBaseAccountWithAddress(accAddr), CodeHash: codeHash.String(), } + acc.Sequence = uint64(1) tApp.AccountKeeper.SetAccount(ctx, &acc) expectFunc := func() { @@ -381,6 +387,7 @@ func TestInitGenesis(t *testing.T) { BaseAccount: authtypes.NewBaseAccountWithAddress(accAddr), CodeHash: codeHash.String(), } + acc.Sequence = uint64(1) tApp.AccountKeeper.SetAccount(ctx, &acc) expectFunc := func() { @@ -482,6 +489,36 @@ func TestInitGenesis(t *testing.T) { {Address: addr2}, } + code := []byte{0x01} + codeHash := crypto.Keccak256Hash(code) + codeHex := common.Bytes2Hex(code) + state.Accounts = append(state.Accounts, + types.GenesisAccount{ + Address: addr1.String(), + Code: codeHex, + }, + types.GenesisAccount{ + Address: addr2.String(), + Code: codeHex, + }, + ) + + accAddr1 := sdk.AccAddress(addr1.Bytes()) + acc1 := ethermint.EthAccount{ + BaseAccount: authtypes.NewBaseAccountWithAddress(accAddr1), + CodeHash: codeHash.String(), + } + acc1.Sequence = uint64(1) + tApp.AccountKeeper.SetAccount(ctx, &acc1) + + accAddr2 := sdk.AccAddress(addr2.Bytes()) + acc2 := ethermint.EthAccount{ + BaseAccount: authtypes.NewBaseAccountWithAddress(accAddr2), + CodeHash: codeHash.String(), + } + acc2.Sequence = uint64(1) + tApp.AccountKeeper.SetAccount(ctx, &acc2) + expectFunc := func() { assert.Equal(t, state.Params.EnabledPrecompiles, @@ -499,6 +536,142 @@ func TestInitGenesis(t *testing.T) { } }, }, + { + name: "Panics when genesis account has a nonce equal to zero", + genFixture: func(t *testing.T, ctx sdk.Context, tApp *app.EthermintApp) testFixture { + address := generateRandomAddress(t) + + code := []byte{0x01, 0x02, 0x03} + codeHash := crypto.Keccak256Hash(code) + codeHex := common.Bytes2Hex(code) + + state := types.DefaultGenesisState() + state.Accounts = append(state.Accounts, types.GenesisAccount{ + Address: address, + Code: codeHex, + }) + + accAddr := sdk.AccAddress(common.HexToAddress(address).Bytes()) + acc := ethermint.EthAccount{ + BaseAccount: authtypes.NewBaseAccountWithAddress(accAddr), + CodeHash: codeHash.String(), + } + acc.Sequence = uint64(0) // Not allowed for contracts + tApp.AccountKeeper.SetAccount(ctx, &acc) + + return testFixture{ + ctx: ctx, + state: state, + precompiles: nil, + expectFunc: nil, + expectPanic: fmt.Errorf("account %s must have a positive nonce", address), + } + }, + }, + { + name: "Allows a genesis account with a nonce greater than 1", + genFixture: func(t *testing.T, ctx sdk.Context, tApp *app.EthermintApp) testFixture { + address := generateRandomAddress(t) + + code := []byte{0x01, 0x02, 0x03} + codeHash := crypto.Keccak256Hash(code) + codeHex := common.Bytes2Hex(code) + + state := types.DefaultGenesisState() + state.Accounts = append(state.Accounts, types.GenesisAccount{ + Address: address, + Code: codeHex, + }) + + accAddr := sdk.AccAddress(common.HexToAddress(address).Bytes()) + acc := ethermint.EthAccount{ + BaseAccount: authtypes.NewBaseAccountWithAddress(accAddr), + CodeHash: codeHash.String(), + } + acc.Sequence = uint64(1000) + tApp.AccountKeeper.SetAccount(ctx, &acc) + + return testFixture{ + ctx: ctx, + state: state, + precompiles: nil, + expectFunc: nil, + expectPanic: nil, + } + }, + }, + { + name: "Panics when genesis account has a public key set", + genFixture: func(t *testing.T, ctx sdk.Context, tApp *app.EthermintApp) testFixture { + privkey, err := ethsecp256k1.GenerateKey() + require.NoError(t, err) + address := common.BytesToAddress(privkey.PubKey().Address()).String() + + code := []byte{0x01, 0x02, 0x03} + codeHash := crypto.Keccak256Hash(code) + codeHex := common.Bytes2Hex(code) + + state := types.DefaultGenesisState() + state.Accounts = append(state.Accounts, types.GenesisAccount{ + Address: address, + Code: codeHex, + }) + + accAddr := sdk.AccAddress(common.HexToAddress(address).Bytes()) + acc := ethermint.EthAccount{ + BaseAccount: authtypes.NewBaseAccountWithAddress(accAddr), + CodeHash: codeHash.String(), + } + acc.Sequence = uint64(1) + pubkey, err := codectypes.NewAnyWithValue(privkey.PubKey()) + require.NoError(t, err) + acc.PubKey = pubkey + tApp.AccountKeeper.SetAccount(ctx, &acc) + + return testFixture{ + ctx: ctx, + state: state, + precompiles: nil, + expectFunc: nil, + expectPanic: fmt.Errorf("account %s must not have a public key set", address), + } + }, + }, + { + name: "Panics when enabled precompile does not have a genesis account with code 0x01", + genFixture: func(t *testing.T, ctx sdk.Context, tApp *app.EthermintApp) testFixture { + address := generateRandomAddress(t) + + code := []byte{0x02} + codeHash := crypto.Keccak256Hash(code) + codeHex := common.Bytes2Hex(code) + + state := types.DefaultGenesisState() + state.Params.EnabledPrecompiles = []string{address} + state.Accounts = append(state.Accounts, types.GenesisAccount{ + Address: address, + Code: codeHex, + }) + + accAddr := sdk.AccAddress(common.HexToAddress(address).Bytes()) + acc := ethermint.EthAccount{ + BaseAccount: authtypes.NewBaseAccountWithAddress(accAddr), + CodeHash: codeHash.String(), + } + acc.Sequence = uint64(1) + tApp.AccountKeeper.SetAccount(ctx, &acc) + + registeredPrecompiles := []precompile_modules.Module{{Address: common.HexToAddress(address)}} + + return testFixture{ + ctx: ctx, + state: state, + precompiles: registeredPrecompiles, + expectFunc: nil, + expectPanic: fmt.Errorf("enabled precompile %s must have code set to 0x01, got 0x%s", address, codeHex), + } + }, + }, } for _, tc := range testCases { diff --git a/x/evm/types/genesis.go b/x/evm/types/genesis.go index 35f7bb3a66..3906cf0139 100644 --- a/x/evm/types/genesis.go +++ b/x/evm/types/genesis.go @@ -76,5 +76,11 @@ func (gs GenesisState) Validate() error { return fmt.Errorf("invalid params: %w", err) } + for _, ep := range gs.Params.EnabledPrecompiles { + if _, ok := seenAccounts[ep]; !ok { + return fmt.Errorf("enabled precompile %s must have a matching genesis account", ep) + } + } + return nil } diff --git a/x/evm/types/genesis_test.go b/x/evm/types/genesis_test.go index 0aa4618a99..9b1f466d74 100644 --- a/x/evm/types/genesis_test.go +++ b/x/evm/types/genesis_test.go @@ -309,6 +309,18 @@ func TestGenesisStateValidate(t *testing.T) { }, expectedErr: "invalid hex address", }, + { + name: "genesis is invalid if enabled precompile does not have a matching genesis account", + getState: func() *types.GenesisState { + state := types.DefaultGenesisState() + + account := defaultGenesisAccount() + state.Params.EnabledPrecompiles = append(state.Params.EnabledPrecompiles, account.Address) + + return state + }, + expectedErr: "must have a matching genesis account", + }, } for _, tc := range testCases {