From 04fa7a9e3b805d410e6be26995fe3492b9926104 Mon Sep 17 00:00:00 2001 From: Simon Date: Wed, 16 Oct 2024 12:51:58 +0200 Subject: [PATCH] Fix selfdestruct empty vs exists bug --- .../interpreter/dynamic_gas.go | 7 + .../interpreter/verify_gas_test.go | 5 +- go/interpreter/lfvm/instructions.go | 10 +- go/interpreter/lfvm/instructions_test.go | 126 +++++++++++++++--- go/interpreter/lfvm/interpreter_test.go | 1 + 5 files changed, 121 insertions(+), 28 deletions(-) diff --git a/go/integration_test/interpreter/dynamic_gas.go b/go/integration_test/interpreter/dynamic_gas.go index 270ce6bd2..68fad0aab 100644 --- a/go/integration_test/interpreter/dynamic_gas.go +++ b/go/integration_test/interpreter/dynamic_gas.go @@ -840,6 +840,13 @@ func gasDynamicSelfDestruct(revision Revision) []*DynGasTest { }) mock.EXPECT().AccountExists(targetAddress).AnyTimes().Return(!empty) + mock.EXPECT().GetBalance(targetAddress).AnyTimes().Return(tosca.Value{}) + mock.EXPECT().GetCode(targetAddress).AnyTimes().Return(nil) + if empty { + mock.EXPECT().GetNonce(targetAddress).AnyTimes().Return(uint64(0)) + } else { + mock.EXPECT().GetNonce(targetAddress).AnyTimes().Return(uint64(1)) + } mock.EXPECT().IsAddressInAccessList(targetAddress).AnyTimes().Return(inAcl) mock.EXPECT().AccessAccount(targetAddress).AnyTimes().Return(tosca.AccessStatus(inAcl)) } diff --git a/go/integration_test/interpreter/verify_gas_test.go b/go/integration_test/interpreter/verify_gas_test.go index 27aa069cb..3c6dff148 100644 --- a/go/integration_test/interpreter/verify_gas_test.go +++ b/go/integration_test/interpreter/verify_gas_test.go @@ -106,12 +106,13 @@ func TestDynamicGas(t *testing.T) { // World state interactions triggered by the EVM. mockStateDB.EXPECT().SetBalance(gomock.Any(), gomock.Any()).AnyTimes() - mockStateDB.EXPECT().GetNonce(gomock.Any()).AnyTimes() mockStateDB.EXPECT().SetNonce(gomock.Any(), gomock.Any()).AnyTimes() mockStateDB.EXPECT().SetCode(gomock.Any(), gomock.Any()).AnyTimes() - // SELFDESTRUCT gas computation is dependent on an account balance and sets its own expectations + // SELFDESTRUCT gas computation is dependent on an account balance and + // existence (handled by nonce in this test), it sets its own expectations if op != vm.SELFDESTRUCT { + mockStateDB.EXPECT().GetNonce(gomock.Any()).AnyTimes() mockStateDB.EXPECT().GetBalance(gomock.Any()).AnyTimes().Return(accountBalance) } diff --git a/go/interpreter/lfvm/instructions.go b/go/interpreter/lfvm/instructions.go index 4afbc7252..062b0dd4f 100644 --- a/go/interpreter/lfvm/instructions.go +++ b/go/interpreter/lfvm/instructions.go @@ -656,8 +656,10 @@ func opSelfdestruct(c *context) (status, error) { cost += getAccessCost(accessStatus) } } - cost += selfDestructNewAccountCost(c.context.AccountExists(beneficiary), - c.context.GetBalance(c.params.Recipient)) + empty := c.context.GetBalance(beneficiary).Cmp(tosca.Value{}) == 0 && + c.context.GetCode(beneficiary) == nil && + c.context.GetNonce(beneficiary) == 0 + cost += selfDestructNewAccountCost(empty, c.context.GetBalance(c.params.Recipient)) // even death is not for free if err := c.useGas(cost); err != nil { return statusStopped, err @@ -668,8 +670,8 @@ func opSelfdestruct(c *context) (status, error) { return statusSelfDestructed, nil } -func selfDestructNewAccountCost(accountExists bool, balance tosca.Value) tosca.Gas { - if !accountExists && balance != (tosca.Value{}) { +func selfDestructNewAccountCost(accountEmpty bool, balance tosca.Value) tosca.Gas { + if accountEmpty && balance != (tosca.Value{}) { // cost of creating an account defined in eip-150 (see https://eips.ethereum.org/EIPS/eip-150) // CreateBySelfdestructGas is used when the refunded account is one that does // not exist. This logic is similar to call. diff --git a/go/interpreter/lfvm/instructions_test.go b/go/interpreter/lfvm/instructions_test.go index 7ccf24315..3c5cd9486 100644 --- a/go/interpreter/lfvm/instructions_test.go +++ b/go/interpreter/lfvm/instructions_test.go @@ -854,35 +854,35 @@ func TestSelfDestruct_Refund(t *testing.T) { func TestSelfDestruct_NewAccountCost(t *testing.T) { tests := map[string]struct { - beneficiaryExists bool - balance tosca.Value - cost tosca.Gas + beneficiaryEmpty bool + balance tosca.Value + cost tosca.Gas }{ - "account exists no balance": { - beneficiaryExists: true, - balance: tosca.Value{}, - cost: 0, + "account empty no balance": { + beneficiaryEmpty: true, + balance: tosca.Value{}, + cost: 0, }, - "account exists with balance": { - beneficiaryExists: true, - balance: tosca.Value{1}, - cost: 0, + "account empty with balance": { + beneficiaryEmpty: true, + balance: tosca.Value{1}, + cost: 25_000, }, - "new account without balance": { - beneficiaryExists: false, - balance: tosca.Value{}, - cost: 0, + "account without balance": { + beneficiaryEmpty: false, + balance: tosca.Value{}, + cost: 0, }, - "new account with balance": { - beneficiaryExists: false, - balance: tosca.Value{1}, - cost: 25_000, + "account with balance": { + beneficiaryEmpty: false, + balance: tosca.Value{1}, + cost: 0, }, } for name, test := range tests { t.Run(name, func(t *testing.T) { - cost := selfDestructNewAccountCost(test.beneficiaryExists, test.balance) + cost := selfDestructNewAccountCost(test.beneficiaryEmpty, test.balance) if cost != test.cost { t.Errorf("unexpected gas, wanted %d, got %d", test.cost, cost) } @@ -901,7 +901,9 @@ func TestSelfDestruct_ExistingAccountToNewBeneficiary(t *testing.T) { ctrl := gomock.NewController(t) runContext := tosca.NewMockRunContext(ctrl) runContext.EXPECT().AccessAccount(beneficiaryAddress).Return(tosca.ColdAccess) - runContext.EXPECT().AccountExists(beneficiaryAddress).Return(false) + runContext.EXPECT().GetBalance(beneficiaryAddress).Return(tosca.Value{}) + runContext.EXPECT().GetCode(beneficiaryAddress).Return(nil) + runContext.EXPECT().GetNonce(beneficiaryAddress).Return(uint64(0)) runContext.EXPECT().GetBalance(selfAddress).Return(tosca.Value{1}) runContext.EXPECT().SelfDestruct(selfAddress, beneficiaryAddress).Return(true) @@ -932,6 +934,80 @@ func TestSelfDestruct_ExistingAccountToNewBeneficiary(t *testing.T) { } } +func TestSelfDestruct_AccountExistsButIsNotEmpty(t *testing.T) { + tests := map[string]struct { + beneficiaryNonce uint64 + beneficiaryBalance tosca.Value + beneficiarCode []byte + remainingGas tosca.Gas + }{ + "empty": { + 0, + tosca.Value{}, + nil, + tosca.Gas(0), + }, + "nonEmptyNonce": { + 1, + tosca.Value{}, + nil, + tosca.Gas(25_000), + }, + "nonEmptyBalance": { + 0, + tosca.Value{1}, + nil, + tosca.Gas(25_000), + }, + "nonEmptyCode": { + 0, + tosca.Value{}, + []byte{0x01}, + tosca.Gas(25_000), + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + address := tosca.Address{0x01} + beneficiary := tosca.Address{0x02} + ctrl := gomock.NewController(t) + runContext := tosca.NewMockRunContext(ctrl) + + runContext.EXPECT().AccessAccount(beneficiary).Return(tosca.ColdAccess) + runContext.EXPECT().GetBalance(beneficiary).Return(test.beneficiaryBalance) + runContext.EXPECT().GetCode(beneficiary).Return(test.beneficiarCode).AnyTimes() + runContext.EXPECT().GetNonce(beneficiary).Return(test.beneficiaryNonce).AnyTimes() + runContext.EXPECT().GetBalance(address).Return(tosca.Value{1}) + runContext.EXPECT().SelfDestruct(address, beneficiary).Return(true) + + ctxt := context{ + params: tosca.Parameters{ + BlockParameters: tosca.BlockParameters{ + Revision: tosca.R13_Cancun, + }, + Recipient: address, + }, + stack: NewStack(), + memory: NewMemory(), + context: runContext, + gas: 27_600, + } + ctxt.stack.push(new(uint256.Int).SetBytes(beneficiary[:])) + status, err := opSelfdestruct(&ctxt) + if err != nil { + t.Fatalf("unexpected error, got %v", err) + } + if want, got := statusSelfDestructed, status; want != got { + t.Fatalf("unexpected status, wanted %v, got %v", want, got) + } + if want, got := test.remainingGas, ctxt.gas; want != got { + t.Errorf("unexpected gas, wanted %d, got %d", want, got) + } + }) + } +} + func TestSelfDestruct_ProperlyReportsNotEnoughGas(t *testing.T) { for _, beneficiaryAccess := range []tosca.AccessStatus{tosca.WarmAccess, tosca.ColdAccess} { for _, accountExists := range []bool{true, false} { @@ -942,7 +1018,13 @@ func TestSelfDestruct_ProperlyReportsNotEnoughGas(t *testing.T) { ctrl := gomock.NewController(t) runContext := tosca.NewMockRunContext(ctrl) runContext.EXPECT().AccessAccount(beneficiaryAddress).Return(beneficiaryAccess) - runContext.EXPECT().AccountExists(beneficiaryAddress).Return(accountExists) + runContext.EXPECT().GetBalance(beneficiaryAddress).Return(tosca.Value{}) + runContext.EXPECT().GetCode(beneficiaryAddress).Return(nil) + if accountExists { + runContext.EXPECT().GetNonce(beneficiaryAddress).Return(uint64(1)) + } else { + runContext.EXPECT().GetNonce(beneficiaryAddress).Return(uint64(0)) + } runContext.EXPECT().GetBalance(selfAddress).Return(tosca.Value{1}) ctxt := context{ diff --git a/go/interpreter/lfvm/interpreter_test.go b/go/interpreter/lfvm/interpreter_test.go index da47554b9..21487feff 100644 --- a/go/interpreter/lfvm/interpreter_test.go +++ b/go/interpreter/lfvm/interpreter_test.go @@ -188,6 +188,7 @@ func TestInterpreter_CanDispatchExecutableInstructions(t *testing.T) { mock.EXPECT().GetBalance(gomock.Any()).AnyTimes() mock.EXPECT().GetCodeSize(gomock.Any()).AnyTimes() mock.EXPECT().GetCode(gomock.Any()).AnyTimes() + mock.EXPECT().GetNonce(gomock.Any()).AnyTimes() mock.EXPECT().AccountExists(gomock.Any()).AnyTimes() mock.EXPECT().AccessStorage(gomock.Any(), gomock.Any()).AnyTimes() mock.EXPECT().GetStorage(gomock.Any(), gomock.Any()).AnyTimes()