From 2017e7b547ca577f7c8f1394d261e1dce6223002 Mon Sep 17 00:00:00 2001 From: Supun Setunga Date: Fri, 3 Nov 2023 10:31:39 -0700 Subject: [PATCH 1/2] Fix nested resource moves --- runtime/interpreter/value.go | 13 +- runtime/runtime_test.go | 188 +++++++++++++++++++++ runtime/sema/check_variable_declaration.go | 14 +- 3 files changed, 203 insertions(+), 12 deletions(-) diff --git a/runtime/interpreter/value.go b/runtime/interpreter/value.go index f9563c8d43..3418719755 100644 --- a/runtime/interpreter/value.go +++ b/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() diff --git a/runtime/runtime_test.go b/runtime/runtime_test.go index f19f19b04d..bded55499a 100644 --- a/runtime/runtime_test.go +++ b/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{}) +} diff --git a/runtime/sema/check_variable_declaration.go b/runtime/sema/check_variable_declaration.go index a34b42b262..f4eededdfe 100644 --- a/runtime/sema/check_variable_declaration.go +++ b/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) - } } } From fa4cf1b77cf37d6b69b53bda67a29ced8d26e875 Mon Sep 17 00:00:00 2001 From: Supun Setunga Date: Fri, 3 Nov 2023 11:14:17 -0700 Subject: [PATCH 2/2] Add interpreter test --- runtime/runtime_test.go | 10 ++--- runtime/tests/interpreter/resources_test.go | 43 +++++++++++++++++++++ 2 files changed, 48 insertions(+), 5 deletions(-) diff --git a/runtime/runtime_test.go b/runtime/runtime_test.go index bded55499a..3286727df6 100644 --- a/runtime/runtime_test.go +++ b/runtime/runtime_test.go @@ -9509,7 +9509,7 @@ func TestNestedResourceMoveInDestructor(t *testing.T) { self.temp <- nil } - pub fun doubler(_ vault: @Bar.Vault): @Bar.Vault{ + pub fun doubler(_ vault: @Bar.Vault): @Bar.Vault { destroy <- create R(<-vault) var doubled <- self.temp <- nil return <- doubled! @@ -9519,16 +9519,16 @@ func TestNestedResourceMoveInDestructor(t *testing.T) { pub var bounty: @Bar.Vault pub var dummy: @Bar.Vault - init(_ v: @Bar.Vault){ + init(_ v: @Bar.Vault) { self.bounty <- v self.dummy <- Bar.createEmptyVault() } - pub fun swap(){ + pub fun swap() { self.bounty <-> self.dummy } - destroy(){ + destroy() { // Nested resource is moved here once var bounty <- self.bounty @@ -9623,7 +9623,7 @@ func TestNestedResourceMoveInDestructor(t *testing.T) { import Bar from %[1]s transaction { - prepare(acc: AuthAccount){ + 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) diff --git a/runtime/tests/interpreter/resources_test.go b/runtime/tests/interpreter/resources_test.go index cd7f20e49d..4d810ef4e5 100644 --- a/runtime/tests/interpreter/resources_test.go +++ b/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{}) +}