Fix selfdestruct empty vs exists bug

Fantom-foundation · Oct 16, 2024 · 04fa7a9 · 04fa7a9
1 parent b9bd48f
commit 04fa7a9
Show file tree

Hide file tree

Showing 5 changed files with 121 additions and 28 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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()