Add FLIP for invalidating references to tranferred resources

[SupunS]

Description

When a reference is taken to a resource, that reference remains valid even if the resource was transferred. In other words, references stay alive forever. This could be a potential foot-gun, where one could gain/give/retain unintended access to resources through references.

This FLIP is to suggest introducing an invalidation of references, if the underlying resource is transferred from its original location where the reference was taken.

---

For contributor use:

• [x] Targeted PR against master branch
• [x] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• [x] Updated relevant documentation
• [x] Re-reviewed Files changed in the Github PR explorer
• [x] Added appropriate labels

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
flow-docs	✅ Ready (Inspect)	Visit Preview	Jul 8, 2022 at 9:16PM (UTC)

[turbolent]

@dsainati1

I like the general principle here, and agree that this removes a potential footgun, but I think that just having this be a runtime error is going to be a source of confusion, and if possible I think we should try to error statically.

Agreed, it would be great to improve this further and detect it also statically. I don't know how we could implement this easily though, as this gets into the territory of adding a borrow checker, which we do not have at the moment.

This seems like it would be possible because we already have the tools necessary to determine whether a reference is valid in a given scope; we know statically once a reference has been moved out of a variable so that we can invalidate uses of it later. It seems like we could use the same principle to statically invalidate any references to that variable once the underlying reference is moved. The properties resources already have should make this analysis fairly simple since we cannot have aliases or nested references.

Do you mean references in "once a reference has been moved" and "once the underlying reference is moved"? We do not currently track references statically.

The above I think should be sufficient to error statically for references created and invalidated within the scope of a single contract or transaction, but would not be able to handle the case where someone saves a reference to a resource to their account and then the resource is transferred in a different transaction. My question on this point then is whether or not there is in fact a legitimate use case for having a reference to a resource that lives beyond the scope of the transaction or contract where it is created? If not, perhaps we could simply prevent saving resource-kinded references at all?

References are only valid during the life-time of a single program execution (transaction/script) and they cannot be stored. Capabilities exist for this reason.

[dsainati1]

Do you mean references in "once a reference has been moved" and "once the underlying reference is moved"? We do not currently track references statically.

Oh yea I definitely meant resources here.

References are only valid during the life-time of a single program execution (transaction/script) and they cannot be stored. Capabilities exist for this reason.

Ah ok well then if this is the case we should definitely be able to handle this statically. I don't think this is quite as complex as something like Rust's borrow checker because our use case is significantly more specific. Assuming that 1) we can know statically what resource a reference is pointing to (which we can because there are no resource aliases) and 2) we can know statically when a resource is invalidated/moved (which obviously we can), it should be possible to implement a checker rule effectively stating that a reference to a resource cannot be used anywhere that the resource itself could not be used. E.g. in the example from the FLIP:

let r <-create R()

let ref = &r as &R

account.save(<-r, to: /storage/r) // after this point `r` is no longer "live", it has been invalidated after the move
  // so we can thus also prevent anyone from using "ref" since we know it is a reference to `r`
ref.id = 2

[SupunS]

I think statically validating all scenarios is not possible. For e.g: see the following example (from a test-case I added for the reference implementation): https://github.com/onflow/cadence/blob/fd35cb13d5318adf45e0d4128c4b829acf595d9a/runtime/tests/interpreter/resources_test.go#L2736-L2747

fun test(source: &[R], target: &[R]) {
    // Take reference while in the account_1
    let ref = &source[0] as &R
                
    // Move the resource out of the account_1 into the account_2
    target.append(<- source.remove(at: 0))

    // Update the reference
    ref.id = 2
}

At checking time, it is not possible to determine whether the source and target are from the same account. And the invalidation depends on whether the two source and target are from the same account or not. (i.e: if they are from the same account then no need to invalidate [?])

[SupunS]

let r <-create R()

let ref = &r as &R

account.save(<-r, to: /storage/r) // after this point `r` is no longer "live", it has been invalidated after the move
  // so we can thus also prevent anyone from using "ref" since we know it is a reference to `r`
ref.id = 2

This may be not always true. See the modified version of the example:

let r <-create R()

let ref = &r as &R

let r2 <- r  // after this point `r` is no longer "live", it has been invalidated after the move.
             // But IMO "ref" is still valid because it is referring to the same 'resource value',
             // (i.e: reference is to the value, not to the variable `r`)
ref.id = 2

[turbolent]

Yeah, we could detect some cases, which is better than none / only run-time checks, but we don't currently have enough static information to detect all cases.

I guess we would have to include the "location" of the target of the reference inside the type, which would be impractical in real-world programs. For example, assuming &T would point to a value on the stack, and e.g. &0x1'T (straw-man syntax) for a reference to a value at address 0x1, then a concrete case of the test function above could be checked, e.g fun test(source: &0x1'R, target: &0x2'R), but in practice you would want to abstract over the addresses (as e.g. contract functions usually work with any address), e.g. fun test<A', B'>(source: &A'R, target: &B'R), ... which gets close to borrow life-times.

[bluesign]

This is very important and was a big security issue. But I think better to reach out people to fix implementations on mainnet before disclosing this ? I am sure I have seen more than one contract vulnerable to this.

Also this is not the best solution in my opinion. If we could just add immutability to cadence. [0] Then we could just make mutable reference immutable.

• which would make this change mostly non-breaking ( I mean breaking less code )
• only defend cases where it needs to defend ( without sacrificing usability )

[0] https://github.com/onflow/cadence/issues/1304

[turbolent]

@bluesign

This is very important and was a big security issue. But I think better to reach out people to fix implementations on mainnet before disclosing this ? I am sure I have seen more than one contract vulnerable to this.

We have not analyzed any contracts if they are vulnerable, because as far as I can see that is impossible to automate. Do you know of a way this could be statically analyzed? Feel free to DM me instead if you feel like that sharing this publicly would be problematic.

[turbolent]

@bluesign

If we could just add immutability to cadence. [0] Then we could just make mutable reference immutable.

• which would make this change mostly non-breaking ( I mean breaking less code )
• only defend cases where it needs to defend ( without sacrificing usability )

[0] https://github.com/onflow/cadence/issues/1304

Thank you for bringing this up, I had misses this issue. In general, I think it would be a good idea to add readonly/immutable references, as it would also help with FLIP 703.

Let's say we would add readonly/immutable references, could you describe how that would prevent the "rug pull" attack? (Is that what you are referring to above?)

[dsainati1]

I think the proposed run-time enforcement would solve the security footgun but would also probably make the language worse overall in the long run. Generally any kind of error that only has runtime enforcement is going to be more confusing and frustrating for users than something with static checks (e.g. null reference errors in languages without optional types) and I think this one is going to be particularly confusing, since the cause and effect of the problem are not necessarily going to be obvious; moving a resource in one part of the code may invalidate references in arbitrary other parts of the code.

If we can't statically invalidate references to moved resources, then I think we should instead try to prevent the situation where references need to be invalidated at all. I like the linked PR about immutable/read-only references, since the security footgun presented by resource references is not an issue when the reference is immutable. Similarly, as you pointed out @turbolent, the issue with https://github.com/onflow/flow/pull/703 is also solved when the reference is immutable.

However, as pointed out, we cannot simply require that references to resource be read-only, since we need to support the Vault use case wherein users borrow a reference to another Vault in order to deposit into it. However, we could require that you provide some kind of "proof" that you own the resource or are otherwise allowed to mutate it, in order to use the mutable features. It's not super fleshed out but I am imagining the proof being something you need statically before you can "downcast" the immutable reference into a mutable one, and being obtainable either by owning the resource or being a resource itself that can be created by the owner of the original resource that allows you to perform a one-time mutation on the resource.

[bluesign]

In my opinion Stack to Stack should be invalid. ( if I understood stack correct, owner=nil )

Though I think they can be valid in the same stack depth.

[dsainati1]

Now that https://github.com/onflow/flow/pull/1056 has been merged in a more limited form than originally proposed, discussion on this FLIP should resume imo. Based on the discussion on that FLIP, it seems as though any analysis powerful enough to make this problem solvable statically would be too complex/burdensome for the language, so a dynamic solution is perhaps the right way forward in this case.

[SupunS]

A draft implementation of this FLIP can be found at https://github.com/onflow/cadence/pull/1961

[SupunS]

In my opinion Stack to Stack should be invalid. ( if I understood stack correct, owner=nil )

Though I think they can be valid in the same stack depth.

I feel invalidating a reference on a resource move from stack to stack(with different depths) can be too restrictive. Do you see any potential issues that it can cause?

[bluesign]

@SupunS

Imagine simple purchase scenario, implemented naively like:

pub fun purchase(ftVault: @FT.Vault, receiver: Capability<&NFT.Receiver>){
   pre{
       ftVault.balance = self.sellprice 
   }
   var NFT <- mintNFT
   receiver.deposit(<-NFT) // exits to unsafe code
   self.vault.deposit(<- ftVault)
} 

in general complicates stuff to check, also makes pre conditions weaker.

[SupunS]

This particular problem with pre/post conditions would be solved with the purity/mutability analysis and restriction in function conditions

[bluesign]

it is not pre/post condition problem. I changed the code.

**myContract Some Receiver Resourcer**
self.savedftVaultReference = &Vault
someContract.purchase(<-vault, receiver : myReceiver ) // myReceiver = self
      **someContract**
      pub fun purchase(ftVault: @FT.Vault, receiver: Capability<&NFT.Receiver>){
         assert(ftVault.balance == self.sellprice)
         var NFT <- mintNFT
         receiver.deposit(<-NFT) // exits to unsafe code
                   ----- Capability<&NFT.Receiver> Context Start - myContract Some Receiver Resource --- 
                         self.stealerVault.deposit( <- savedftVaultReference.withdraw(amount: 42.0) )
                         self.collection.deposit( <- NFT ) 
                   ----- Capability<&NFT.Receiver> Context End - myContract Some Receiver Resource  --- 
         self.vault.deposit(<- ftVault)
         }
         ** someContract end ***
self.savedftVaultReference = nil

it is more like re-entry problem.

[SupunS]

oh, I see your point. Makes sense. Thanks for the example!

[SupunS]

When I initially wrote this proposal, I was under the idea that, a reference should be only invalidated if the current location of the underlying resource is different from the location of the resource when the reference was taken. e.g: If a reference rRef is taken to a resource r which is in account A, then if the resource is no longer in A when the rRef is accessed, then the reference is invalid. On the other hand, if r is still in account A, then the rRef is valid.

But this leaves a catch: In-between the creation of rRef and the actual use of rRef, the resource r can be moved across different accounts/stacks, and come back to A (its original location). Now according to the above rule, this reference is still valid. In terms of a security perspective, I think this should be OK. One of the pros of this is that, it allows moving a resource within an account (e.g: within a collection), and can still maintain the validity of the reference.

But the more I look into this, the more I start to feel like it may be better to make it more strict by immediately invalidating references upon a move of the underlying resource. So even if the resource is back at its original location/account, the reference would be invalid.

[joshuahannan]

My instinct would be to always make the reference invalid even if the resource is returned to the original spot. Just my two cents

[bluesign]

Now according to the above rule, this reference is still valid. In terms of a security perspective, I think this should be OK.

This one is tricky, I agree with @joshuahannan on this, too many edge cases to handle.

[SupunS]

Thanks for the feedback. Seems we are all on the same ground that transferring a resource should invalidate their references immediately, and should stay invalid, even if the resource is moved back to its original spot later on.

[SupunS]

The only thing remaining is to decide on whether to tighten up the restriction more by also invalidating the references if the resource is moved from stack to stack.

I am adding below a full code example of the foot-gun @bluesign posted above, so that we can go through it and hopefully finalize during the next language design meeting.

access(all) contract Buyer {

    pub fun buy()  {
        let receiver: &Receiver ...
        let vault: @Vault ...

        // keep a reference to the vault, and transfer the vault to purchase something
        receiver!.vaultRef = &vault as &Vault

        Seller.purchase(receiver, <-vault)
    }

    pub resource Receiver {
        priv let vaultRef: &Vault?
        priv let secretVault: @Vault

        init() {
            self.secretVault <- create Vault(0)
            self.vaultRef = nil
        }

        pub fun deposit(_ something: @AnyResource)  {
            self.steal()
            destroy something
        }

        pub fun steal()  {
            let stolen <- self.vaultRef!.withdraw(50)
            self.secretVault.deposit(<- stolen)
        }
    }

    pub resource Vault {
        pub var balance: Int

        init(_ balance: Int) {
            self.balance = balance
        }

        pub fun withdraw(_ amount: Int): @Vault  {
            self.balance = self.balance - amount
            return <- create Vault(amount)
        }

        pub fun deposit(_ from: @Vault)  {
            self.balance = self.balance + from.balance
            destroy from
        }
    }
}

access(all) contract Seller {

  pub fun purchase(_ receiver: &Receiver, _ vault: @Vault) {
    pre {
      vault.balance >= sellingPrice
    }

    receiver.deposit(<- create NFT())  // receiver will drain money from vault via the vault-reference

    self.vault.deposit(<-vault)
  }
}

[SupunS]

So the agreement as per the last language design meeting was to invalidate the references even if the move is from stack to stack. Updated the FLIP to reflect these changes: 98f88a7ceb4ee9adbfa9fa0d74c7546d640c2b31

[SupunS]

Thank you all for taking the time to review and provide feedback.

Based on the feedback, it seems there are no negative sentiments towards the proposal, and hence I'm gonna consider this as accepted.