Merge pull request #2930 from onflow/supun/v0.42-port-151

[v0.42] Fix nested resource moves

runtime/interpreter/value.go

@@ -17175,6 +17175,13 @@ func (v *CompositeValue) Transfer(
 	preventTransfer map[atree.StorageID]struct{},
 ) Value {
 
+	config := interpreter.SharedState.Config
+
+	// Should be checked before accessing `v.dictionary`.
+	if config.InvalidatedResourceValidationEnabled {
+		v.checkInvalidatedResourceUse(locationRange)
+	}
+
 	baseUse, elementOverhead, dataUse, metaDataUse := common.NewCompositeMemoryUsages(v.dictionary.Count(), 0)
 	common.UseMemory(interpreter, baseUse)
 	common.UseMemory(interpreter, elementOverhead)
@@ -17183,12 +17190,6 @@ func (v *CompositeValue) Transfer(
 
 	interpreter.ReportComputation(common.ComputationKindTransferCompositeValue, 1)
 
-	config := interpreter.SharedState.Config
-
-	if config.InvalidatedResourceValidationEnabled {
-		v.checkInvalidatedResourceUse(locationRange)
-	}
-
 	if config.TracingEnabled {
 		startTime := time.Now()
 

runtime/runtime_test.go

@@ -9461,3 +9461,191 @@ func TestRuntimeInvalidWrappedPrivateCapability(t *testing.T) {
 	var argumentNotImportableErr *ArgumentNotImportableError
 	require.ErrorAs(t, err, &argumentNotImportableErr)
 }
+
+func TestNestedResourceMoveInDestructor(t *testing.T) {
+
+	t.Parallel()
+
+	runtime := newTestInterpreterRuntime()
+
+	signerAccount := common.MustBytesToAddress([]byte{0x1})
+
+	signers := []Address{signerAccount}
+
+	accountCodes := map[Location][]byte{}
+
+	runtimeInterface := &testRuntimeInterface{
+		getCode: func(location Location) (bytes []byte, err error) {
+			return accountCodes[location], nil
+		},
+		storage: newTestLedger(nil, nil),
+		getSigningAccounts: func() ([]Address, error) {
+			return signers, nil
+		},
+		resolveLocation: singleIdentifierLocationResolver(t),
+		getAccountContractCode: func(location common.AddressLocation) (code []byte, err error) {
+			return accountCodes[location], nil
+		},
+		updateAccountContractCode: func(location common.AddressLocation, code []byte) (err error) {
+			accountCodes[location] = code
+			return nil
+		},
+		emitEvent: func(event cadence.Event) error {
+			return nil
+		},
+		log: func(s string) {
+			fmt.Println(s)
+		},
+	}
+
+	nextTransactionLocation := newTransactionLocationGenerator()
+
+	attacker := []byte(fmt.Sprintf(`
+        import Bar from %[1]s
+
+        access(all) contract Foo {
+            pub var temp: @Bar.Vault?
+            init() {
+                self.temp <- nil
+            }
+
+            pub fun doubler(_ vault: @Bar.Vault): @Bar.Vault {
+                destroy  <- create R(<-vault)
+                var doubled <- self.temp <- nil
+                return <- doubled!
+            }
+
+            pub resource R {
+                pub var bounty: @Bar.Vault
+                pub var dummy: @Bar.Vault
+
+                init(_ v: @Bar.Vault) {
+                     self.bounty <- v
+                     self.dummy <- Bar.createEmptyVault()
+                }
+
+                pub fun swap() {
+                    self.bounty <-> self.dummy
+                }
+
+                destroy() {
+                    // Nested resource is moved here once
+                    var bounty <- self.bounty
+
+                    // Nested resource is again moved here. This one should fail.
+                    self.swap()
+
+                    var dummy <- self.dummy
+
+                    var r1 = &bounty as &Bar.Vault
+
+                    Foo.account.save(<-dummy, to: /storage/dummy)
+                    Foo.account.save(<-bounty, to: /storage/bounty)
+
+                    var dummy2 <- Foo.account.load<@Bar.Vault>(from: /storage/dummy)!
+                    var bounty2 <- Foo.account.load<@Bar.Vault>(from: /storage/bounty)!
+
+                    dummy2.deposit(from:<-bounty2)
+                    Foo.temp <-! dummy2
+                }
+            }
+        }`,
+		signerAccount.HexWithPrefix(),
+	))
+
+	bar := []byte(`
+        pub contract Bar {
+            pub resource Vault {
+
+                // Balance of a user's Vault
+                // we use unsigned fixed point numbers for balances
+                // because they can represent decimals and do not allow negative values
+                pub var balance: UFix64
+
+                init(balance: UFix64) {
+                    self.balance = balance
+                }
+
+                pub fun withdraw(amount: UFix64): @Vault {
+                    self.balance = self.balance - amount
+                    return <-create Vault(balance: amount)
+                }
+
+                pub fun deposit(from: @Vault) {
+                    self.balance = self.balance + from.balance
+                    destroy from
+                }
+            }
+
+            pub fun createEmptyVault(): @Bar.Vault {
+                return <- create Bar.Vault(balance: 0.0)
+            }
+
+            pub fun createVault(balance: UFix64): @Bar.Vault {
+                return <- create Bar.Vault(balance: balance)
+            }
+        }
+    `)
+
+	// Deploy Bar
+
+	deployVault := DeploymentTransaction("Bar", bar)
+	err := runtime.ExecuteTransaction(
+		Script{
+			Source: deployVault,
+		},
+		Context{
+			Interface: runtimeInterface,
+			Location:  nextTransactionLocation(),
+		},
+	)
+	require.NoError(t, err)
+
+	// Deploy Attacker
+
+	deployAttacker := DeploymentTransaction("Foo", attacker)
+
+	err = runtime.ExecuteTransaction(
+		Script{
+			Source: deployAttacker,
+		},
+		Context{
+			Interface: runtimeInterface,
+			Location:  nextTransactionLocation(),
+		},
+	)
+	require.NoError(t, err)
+
+	// Attack
+
+	attackTransaction := []byte(fmt.Sprintf(`
+        import Foo from %[1]s
+        import Bar from %[1]s
+
+        transaction {
+            prepare(acc: AuthAccount) {
+                acc.save(<- Bar.createVault(balance: 100.0), to: /storage/vault)!
+                var vault = acc.borrow<&Bar.Vault>(from: /storage/vault)!
+                var flow <- vault.withdraw(amount: 42.0)
+
+                var doubled <- Foo.doubler(<-flow)
+
+                destroy doubled
+            }
+        }`,
+		signerAccount.HexWithPrefix(),
+	))
+
+	err = runtime.ExecuteTransaction(
+		Script{
+			Source: attackTransaction,
+		},
+		Context{
+			Interface: runtimeInterface,
+			Location:  nextTransactionLocation(),
+		},
+	)
+
+	RequireError(t, err)
+	require.ErrorAs(t, err, &interpreter.UseBeforeInitializationError{})
+}

runtime/sema/check_variable_declaration.go

@@ -96,6 +96,14 @@ func (checker *Checker) visitVariableDeclaration(declaration *ast.VariableDeclar
 
 	var secondValueType Type
 
+	// The value transfer need to be marked as a resource move (when applicable),
+	// even if there is no second value, because in destructors,
+	// nested resource moves are allowed.
+	valueIsResource := valueType != nil && valueType.IsResourceType()
+	if valueIsResource {
+		checker.elaborateNestedResourceMoveExpression(declaration.Value)
+	}
+
 	if declaration.SecondTransfer == nil {
 		if declaration.SecondValue != nil {
 			panic(errors.NewUnreachableError())
@@ -132,8 +140,6 @@ func (checker *Checker) visitVariableDeclaration(declaration *ast.VariableDeclar
 			// so that second value types that are standalone not considered resource typed
 			// are still admitted if they are type compatible (e.g. `nil`).
 
-			valueIsResource := valueType != nil && valueType.IsResourceType()
-
 			if valueType != nil &&
 				!valueType.IsInvalidType() &&
 				!valueIsResource {
@@ -162,10 +168,6 @@ func (checker *Checker) visitVariableDeclaration(declaration *ast.VariableDeclar
 				declaration.SecondTransfer,
 				true,
 			)
-
-			if valueIsResource {
-				checker.elaborateNestedResourceMoveExpression(declaration.Value)
-			}
 		}
 	}
 

runtime/tests/interpreter/resources_test.go

@@ -2938,3 +2938,46 @@ func TestInterpretInnerResourceDestruction(t *testing.T) {
 	var destroyedResourceErr interpreter.DestroyedResourceError
 	require.ErrorAs(t, err, &destroyedResourceErr)
 }
+
+func TestInterpretInnerResourceMove(t *testing.T) {
+
+	t.Parallel()
+
+	inter := parseCheckAndInterpret(t, `
+        pub resource OuterResource {
+            pub var a: @InnerResource
+            pub var b: @InnerResource
+
+            init() {
+                 self.a <- create InnerResource()
+                 self.b <- create InnerResource()
+            }
+
+            pub fun swap() {
+                self.a <-> self.b
+            }
+
+            destroy() {
+                // Nested resource is moved here once
+                var a <- self.a
+
+                // Nested resource is again moved here. This one should fail.
+                self.swap()
+
+                destroy a
+                destroy self.b
+            }
+        }
+
+        pub resource InnerResource {}
+
+        pub fun main() {
+            let a <- create OuterResource()
+            destroy a
+        }`,
+	)
+
+	_, err := inter.Invoke("main")
+	RequireError(t, err)
+	require.ErrorAs(t, err, &interpreter.UseBeforeInitializationError{})
+}