FLIP 131: removal of custom destructors

[dsainati1]

This adds a FLIP proposing the removal of support for custom destructors as a solution to the attachments trolling attack. Specifically, it will no longer be possible to declare a destroy method in a resource. This would prevent the attachments attack that relies on preventing the destruction of an attachment in its destroy method.

[austinkline]

I think we need more detail about impact, here. I'd like to see this FLIP propose alternatives to the common cases so we can understand how this is actually going to impact what matters. Many of the impacted contracts out there are going to likely be boiler-plate which means that devs who need to correct things aren't necessarily going to know what they even are supposed to do to ensure the data they rely on remains accurate.

At a minimum, we should see how indexers are expected to track the destruction of an NFT and FungibleToken balances which are very important.

That, coupled with releasing the findings/analysis so that we can better understand the less common contracts means we can discuss more completely what will happen to contracts that rely on this

This also mean that apps which have no backend and rely only on Cadence scripts would become much more limited. This FLIP should be sure to call out the pain that will be caused to those apps so we can discuss impact and try to involve anyone who runs such a product.

[austinkline]

How is this an anti-pattern? My understanding of destruction and the use of conditions/panics it that it ensures that a resource is safe to be deleted in the first place.

For example, Flowty uses this to guarantee that no one can destroy a loan which has an NFT in escrow

[dsainati1]

Yeah, if the destructor is actually called, then the panic/assertion would ensure that the conditions are correct for it to be destroyed. However, if the resource is sent to an inaccessible account, then the destructor is never run. So, for example, someone could just send their loan away to a burner account, and it becomes inaccessible to everyone, effectively no longer existing, but the destructor is never called.

[austinkline]

However, if the resource is sent to an inaccessible account, then the destructor is never run. So, for example, someone could just send their loan away to a burner account, and it becomes inaccessible to everyone, effectively no longer existing, but the destructor is never called.

These really aren't the same, though. One is meant to prevent deletion of a resource accidentally. The other is someone getting rid of something explicitly. Just because someone can get around destruction doesn't mean it has no use. Ways to circumvent accidentally taking the wrong action doesn't mean safeguards aren't valuable

[dsainati1]

Yeah, this is a philosophical point that I think merits further discussion, maybe in a breakout session. But especially with the upcoming changes in Stable Cadence that will invalidate all references to a resource when the resource is moved, the extent to which a deleted and an inaccessible resource are functionally different is (imo) very minimal.

Ways to circumvent accidentally taking the wrong action doesn't mean safeguards aren't valuable

As for this, I think it cuts two ways. Yes, having safeguards can be valuable, but the existence of safeguards can also cause complacency in implementation, which is dangerous if those safeguards aren't 100% accurate. You see this all the time in type system design, where when something is 99% safe, users will assume that it works perfectly and won't take the proper precautions to handle the 1% of cases where it is not safe.

[austinkline]

You see this all the time in type system design, where when something is 99% safe, users will assume that it works perfectly and won't take the proper precautions to handle the 1% of cases where it is not safe.

What is unsafe, here, in this hypothetically? I get that in theory someone could get confused by the state of something being destroyed versus moved to an inaccessible address, but moving any resource anywhere is a core feature of cadence. If a developer isn't taking that into account, or doesn't understand that, then they are going to be running into all kinds of foot-guns in the future. Do you have any example of an application which will benefit from this, or which runs into this confusion?

[dsainati1]

Possibly I am misunderstanding the Flowty use case that you identified earlier, but if you use panicking destructors to "guarantee that no one can destroy a loan which has an NFT in escrow", then is it not potentially a safety issue that someone can effectively destroy a loan (even with an escrowed NFT) by sending it to a burner account? Very possibly I am wrong about this fwiw, just want to check my understanding here.

[austinkline]

I'm not sure I follow this. Yes you can always send items to a non-accessible address like we do on ethereum, but those addresses can be marked or determined to be inaccessible which means indexers are able to still correctly calculate the total supply of a token or an NFT Collection

Information like this is very important to have accurate, especially if defi is going to be focused on more on flow

[dsainati1]

This is only saying that you cannot rely on destructors executing to accurately compute this information, since there is no guarantee that the destructor is actually called if a resource is sent to an inaccessible account.

Indexers would still be able to compute this information accurately by looking at all the existing, accessible supply of a resource. In fact, indexing this way would probably be the suggested approach for this kind of thing, instead of relying on a destructor, when accuracy is important.

[austinkline]

Indexers would still be able to compute this information accurately by looking at all the existing, accessible supply of a resource

How would that actually work, though? A withdraw event for NFTs doesn't mean it's been destroyed. Nor does access or lackthereof now imply that it will always be inaccessible. Flowty escrows NFTs and FTs when loans are funded, and NFTs when a rental is filled. In both of those cases, there are resources you temporarily do not have access to, but the supply is not actually changed. If Flow is going to say that indexers can calculate this, then I'd like to see how it is actually possible which I'm pretty skeptical of right now

[austinkline]

The problem with this kind of analysis is that it doesn't take into account that boilerplate contracts are likely to overly represent the state of things. For instance, what if something in the NFTStorefrontV2 contract was impacted by this?

Everyone is supposed to use the same contract and so you would only see one count included in this analysis, despite that contract being a core piece of infrastructure. I'd like to see what contracts fall into these smaller counts, or at least publish all of these findings so the community can do it

[dsainati1]

onflow/cadence-tools#194

This PR has the linter that was used to compute this info, which you can run this way: https://github.com/onflow/cadence-tools/tree/master/lint#analyzing-contracts-in-a-directory

[austinkline]

Please run it for the community and post findings somewhere as part of this flip

[bluesign]

yeah stats here are a bit meaningless, 211 complex destructors are like >10% ( even without considering @austinkline 's point )

[dsainati1]

I added the output of the analysis to the PR

[joshuahannan]

I agree with Austin that the blast radius for a change like this is bigger than what this tool might indicate because of certain contracts

[austinkline]

Does this mean another alternative is to not release attachments with stable cadence?

[dsainati1]

That would be up for discussion. If we don't solve this problem, then we would need to decide whether or not it is feasible to release attachments with the existing security concerns. It is possible that we might decide it's not possible, and if a solution to the attack isn't found that doesn't rely on breaking changes, we may need to scrap the feature.

[austinkline]

It is possible that we might decide it's not possible, and if a solution to the attack isn't found that doesn't rely on breaking changes, we may need to scrap the feature.

To be clear, it could be scrapped until something like try/catch is done. In which case it could be released alongside whatever solution is implemented

[dsainati1]

The addition of try catch would not solve the problem unless the use of try catch is mandated in destructors, which would be a breaking change.

[bjartek]

I am against this FLIP. The impact of this change is just too huge. Anybody can write a transaction that withdraw and destroy something without your code beeing called. There needs to be some way to control what happens when something is destroyed or else so much existing logic will just fail.

[bluesign]

I feel like without attachments there is not a problem. ( security or else ) And there seems to be valid cases to prevent resource destruction with panic ( consider disallowing deletion of last admin resource, lost & found, floaty use cases etc ) and useful / required cases ( total supply tracking, emitting events etc )

In this case maybe we can focus on safely removing an attachment ( to be honest I didn't have time to play with attachments much yet )

I still believe try/catch ( not language feature but destruction feature ) is the solution here;
In the attachment removal context: Ask attachment kindly to clean up, if it panics for any reason ( in destructor ), you clean up the resource, continue from the next statement. ( by asking again kindly at first to sub resources inside if they fail, force clean them )

This guarantees, well behaved resources will always have their complex destructors executed as they except. Only broken destructors will be affected. It should be developers responsibility to clean up in case of a failure.

destroy(){
     destroy self.evilresource 
     self.i = i-1 #underflow
     destroy self.goodResource
}

in this case; destroy will always succeed. I will have events from goodResource. (even though underflow panics, it will be destroyed on clean up )

[bluesign]

I was under the impression that we compare Cadence with swift / go / java when considering language design changes, this feels a bit like step back to compare with Move.

[bluesign]

Also trolling attack can be prevented by updating storage fees logic. ( related discussion https://forum.onflow.org/t/storage-fees-improvements-and-few-random-ideas-on-the-way/5104 )

[joshuahannan]

I'm honestly not really convinced that the trolling attack is a big enough problem to warrant a change like this. I personally don't think that attachments are going to see much use and I think that the trolling attack is one of many ways people can take advantage of the current storage fee mechanisms to make life harder for a user. It doesn't allow them to steal something from a user or do something else that is equally as malicious, so I don't see it being as big of a problem. I like having custom destructors and think that they can do a lot of useful things. I also think exploring updates to storage fee logic could be worth doing, like bluesign says above

[dsainati1]

I have added a background context section to the FLIP in response to @austinkline's feedback in the LDM yesterday (thank you for that 🙏 !).

In particular, as a quick response to some of the comments on this FLIP regarding the size of the problem and whether this change is warranted:

During our previous discussions on this topic (and my bad for leaving this context out of the FLIP before, sorry), we came to the conclusion that a) this is not just a problem with attachments, b) we would like users to have full control over resources they own (including being able to delete them), and c) the storage size is not the only issue here, illegal or otherwise offensive content is also problematic. As such any solutions to this problem that either only address attachments, don't guarantee destructor success, or only approach the problem from the storage cost perspective, are not likely to be viable solutions.

[bluesign]

As I stated in the call, if there are good use cases for complex destructors, handling it with removing it totally is a bit of an extreme. Maybe we can find a solution not punishing the good developers but somehow prevent abuse of destructors. I am not a fan of complex destructors but some use cases ( flowty, total_supply, emitting events etc ) seems really useful.

Panic can be avoided ( in flowty case stuff can be send to lost & found etc ), so as a rule not allowing panic is not that harsh. I believe we can make not to panic in destructor logic developers's responsibility and even we can guide developer's on actions that can panic with LSP. If my destructor panic's at some point, I should be ok with remaining part is not executed, and resource is destructed anyway cleanly. ( all not destroyed sub resources cleaned up with the same logic etc ) In a way we can make sure good behaving destructors always run succesfully.

[dsainati1]

Here is a link to a public notion page summarizing our discussions on this topic so far: https://dapperlabs.notion.site/Forced-Resource-Deletion-1e6ac5bd6fe6416fa579a9595b9490a3

[austinkline]

Based on everything I'm seeing, I continue to believe that try/catch is the primary solution we need, here. Panic's can always be snuck in with invalid state, custom destructors are how we track important things on standard-adherent contracts such as TotalSupply, and also capture when things happen in a consistent way (emitting an event on destroy), and it's necessary to protect some items from being destroyed without cleanup. This is a core part of the language, and while we are all accepting that breaking changes will come with stable cadence, this one is in its own territory as there will be absolutely no way to achieve an equivalent behavior

this is not just a problem with attachments

The FLIP should callout that this isn't just attachments, then. Because right now it says
"The primary motivation for this is to resolve the "trolling attack" that can be used to
exploit the attachments feature"

the storage size is not the only issue here, illegal or otherwise offensive content is also problematic

In what scenario? You mean in some world where attachments are live and I can attach something to an item you don't want? Because in other cases I can think of, you could just move the offending resource to another account as has been argued for why TotalSupply isn't something that can be maintained.

It sounds like attachments have many potential problems currently. As of now, an actual resource can't really cause these same problems that a typical user is going to run into. For instance, if there's an NFT that has content a user doesn't like but can't delete, the user would have had to set something up to receive such an item. In that case, it would have to go into a resource that can contain the item in question which means a collection just for this kind of NFT.

By the way, this very problem is why myself and some others very adamantly argued against a shared nft collection so long ago here:
https://forum.onflow.org/t/streamlined-token-standards-proposal/3075/7?u=austin

[dsainati1]

For instance, if there's an NFT that has content a user doesn't like but can't delete, the user would have had to set something up to receive such an item.

Not necessarily the case unfortunately; a user may have chosen to receive an NFT at some point when its content was not objectionable to them, but the NFT contract can still be updated after the fact with a function that allows the contract author to add or edit the NFT's content to something that the user may not want.

Agreed though that this problem is made worse with attachments, since you can get rid of a resource you don't want, while an attachment that you don't like on an NFT that you otherwise like is a bigger problem.

[dete]

Based on everything I'm seeing, I continue to believe that try/catch is the primary solution we need, here.

Agreed 100%, @austinkline. The problem is that try/catch is estimated to take 6-12 months to implement. The question is, what do we do until we have try/catch and can make custom destructors "safe"?

I'm leaning towards removing destructors for Cadence 1.0, and then adding them back in when we have try/catch and can force destructors to not abort (by statically requiring the author to try/catch the entire body of the destructor). This approach is consistent: In 1.0 destroy always works, never aborts. In the future (when try/catch is available), destroy always works, never aborts.

Maintaining the status quo is problematic:

1. We allow the creation of objects that can't be deleted.
2. We'll break any code that depends on failing destructors as a safety mechanism if we introduce forced destruction in future.

I'll actually start with the second one first. Imagine we stick with the status quo with the intention of allowing forced destruction in future (once we get try/catch working). We already have devs who are using abort() in destructors as a guard rail to prevent against accidental destruction (with all the best intentions!). But what about code that assumes that the object doesn't get deleted unless the safety checks all pass. We are laying the groundwork for some very subtle bugs where code assumes invariants that stop being invariant when forced destruction is implemented.

But the most important point, I think, is that the creation of indestructible objects is much bigger than just the trolling attack we've been discussing. At the base level, I think there's just a philosophical disconnect between "Blockchain objects are real property that give users real property rights" and "Yeah, but you can't delete it unless the creator allows it." The trolling attack eating up user storage is real, tho, and once we introduce Attachments, it gets much, much worse. Today, Andy the Adversary can make a trolling NFT that eats up my storage and can't be deleted. But if I don't opt into accepting deposits of Andy's NFT type, I'm safe. But tomorrow, Andy can turn any Top Shot or Flowvatar into a hot potato that can't be deleted. Yikes!

But y'all are clearly not used to thinking evil thoughts! You haven't imagine the worst kind of attacks you can create with Attachments. Imagine an escrow contract that holds onto a Vault and which it later deposits into some Receiver. Andy could stick an indestructible attachment on that Vault. Anyone looking at the escrow contract would assume the payment can go through, but try to deposit that Vault, and it fails because the standard deposit() method destroys the incoming Vault. The escrow contract is now permanently locked, never to be used again. I'm sure there are other examples of object destruction that are implicit, beyond deposit() that we don't think about until we look through the eyes of an attacker.

And, of course, we haven't yet discussed the most vile issue. There are forms of data that are so illegal and immoral I won't even name them. Imagine if something like that was sitting in your account, attached to an asset that you valued. How would you feel about that situation? It's bad enough that it could be stored on the blockchain at all, but in a blockchain where ownership is so direct as in Flow, it's a really upsetting idea.

Finally, I think it's worth noting that the other notable Resource-oriented language, Move, does not have destructors. I would never suggest we copy other languages without thinking about the costs/benefits, but it's an existence proof that a smart-contract language without destructors can still be interesting and useful.

In the end, I think destructors are incredibly valuable for all the reasons outlined in this discussion. Once we have try/catch implemented, I will be at the head of the mob demanding we bring them back. But I can't put all of those advantages above the single most important principle of Web3 (in my mind, anyway): True ownership and the power for users to manage their own property as they see fit, including disposal.

[bluesign]

The problem is that try/catch is estimated to take 6-12 months to implement.

Not that I agree with 6-12 months assessment, but considering it is true, if we go with removing destructors now, it turns to be 6 years to have try/catch instead of 6 months for sure.

I think this is dangerous way to approach, easily we can remove features, but later adding them will create inconsistencies. Some resources emitting events on destructors, some not etc.

Best approach, is to decide how it should be done, how we can go there as fast as possible. We will do someday is not the best approach.

[bjartek]

Has there been any discussions on detaching an attachment as a seperate option to deleting? If the main issue is an attachment that can stop deleting of the main resource then what if we add the option to take it out and save it somewhere else/move it somewhere else.

[bjartek]

I would much rather keep destructors and delay attachments until we can add them safely then going the other way around.

[dsainati1]

Has there been any discussions on detaching an attachment as a seperate option to deleting? If the main issue is an attachment that can stop deleting of the main resource then what if we add the option to take it out and save it somewhere else/move it somewhere else.

Regardless of whether attachments are destroyed or not on detachment, they need explicit handling because they can contain resources, and thus we need to require those nested resources to be explicitly handled so as not to be lost. Any kind of explicit handling can be attacked the same way (e.g. by inserting a panic into the code), so whether or not we call the method destroy or not it poses the same issue.

I would much rather keep destructors and delay attachments until we can add them safely then going the other way around.

This is an appealing option, but unless we are willing to delay Stable Cadence along with attachments (which I don't think we are), it requires a solution to the destruction problem that is not a breaking change. We have not yet identified such a solution.

[bluesign]

It requires a solution to the destruction problem that is not a breaking change. We have not yet identified such a solution.

This part confused me a little, can you expand a bit @dsainati1 ? The point is breaking existing code or @dete 's argument about breaking the behaviour ?

[dsainati1]

This part confused me a little, can you expand a bit @dsainati1 ?

It sounded to me like what @bjartek was proposing (although please correct me if I misunderstood!) was that we would not make the changes proposed in this FLIP, and delay the release of attachments until we can implement a better solution. This is certainly an option, but all of the solutions we have identified to the destruction problem (like try-catch, for example) are breaking changes; they would require users to write their destructors differently.

Since all the solutions we know about are breaking changes, unless we come up with a solution that is not a breaking change to Cadence, we could not release any of these solutions after Stable Cadence is out (since we are not planning on making any more breaking changes after that point). As such, if we wanted to delay attachments until we have a better solution, we would also have to delay Stable Cadence until that point as well, which may not be an option.

[joshuahannan]

Imagine an escrow contract that holds onto a Vault and which it later deposits into some Receiver. Andy could stick an indestructible attachment on that Vault. Anyone looking at the escrow contract would assume the payment can go through, but try to deposit that Vault, and it fails because the standard deposit() method destroys the incoming Vault. The escrow contract is now permanently locked, never to be used again.

I don't understand this example. It might need some clarification.

I'm closer to being convinced that removing custom destructors is a good idea, but I still think it is hard to truly say if the risk of these vulnerabilities outweighs the benefits of having custom destructors. I'm kind of ambivalent now.

[bluesign]

After @dsainati1 's last comment, I am convinced that keeping them like current state is not a good solution. ( in stable cadence context ) After giving some thought with black hat on, I think we can't have complex destructors in attachments in a safe way anyway. ( at least without a huge foot gun )

I think main concern is just events, maybe some auto fired event with resource type and uuid can cover that.
A.012345689.MyNFT.Destroyed(uuid: 1234)

[bjartek]

Or just a general event for destruction with the type and id and uuid

[turbolent]

Thank you everyone for the great feedback so far! As we go into the breakout session to discuss this FLIP further, I just wanted to quickly re-focus us (the last comments are already moving in this direction 👌)

As we discussed before in multiple breakout sessions, the biggest problem is that a user might have an urgent need to destroy/delete a resource, e.g. for legal reasons.

While attachments will make it easier to send any kind of resource to a user, the problem of a user not always being able to delete a resource they own is already a problem today. The FLIP is trying to address the general problem (which should be maybe emphasized a bit more in the FLIP). It would be good if we keep our discussions focused on the proposed solution and the general problem.

Please see the overview of this topic so far: https://dapperlabs.notion.site/Forced-Resource-Deletion-1e6ac5bd6fe6416fa579a9595b9490a3, especially the "Discussion outcomes" section. We should aim to review this FLIP in light of these outcomes that have been established so far, unless there are significant new insights that would warrant re-evaluating them (probably best in a discussion separate from this FLIP).

[dsainati1]

We just had a public breakout session for this FLIP, and we came away with a couple things that need addressing in order for this FLIP to be a feasible solution to the force-deletion issue:

1. We absolutely need to have support for the off-chain tracking use case; developers and users alike need to be able to know when an event is deleted. I will be updating this FLIP once I have a design for a way for resources to define a custom OnDestroy (or similar) event that is automatically emitted when the resource is destroyed, along with the ability to define custom fields (and values for those fields) of that event.

2. There is an open question about metering the destruction (and relatedly the emission of the event described above) of resources; if we need to guarantee that resource destruction cannot fail we need to handle the case where the resource is so large that it cannot be destroyed in a single transaction. Possible solutions to this were proposed in the breakout, including a potential "tombstoning" process where a resource is marked destroyed and then actually physically destroyed later, but this still needs further design and discussion.

3. We also discussed the "FT total supply" use case, and the sentiment generally was that it is not actually that large of a problem not to have a solution here. On-chain total supply is inherently inaccurate as a result of tokens being lost in locked or inaccessible accounts, and as such having 100% perfect tracking of the supply is not possible. For the use cases that use the supply and don't care about perfect accuracy, we might be able to define a burn method for tokens that handles the supply before destroying the token, and request that contracts call that function rather than directly destroying the token.

[dsainati1]

We had another breakout session on this topic, with two main conclusions:

1. We want some feedback from tokenomics experts and dex developers on the potential changes to totalSupply calculation. We should hopefully be getting in touch with some of them to comment on this FLIP.

2. Regarding default event emission, there were some questions about how to handle very large events (similar to very large resources). We have two potential options: limiting the size of what can be included in a default event (banning arrays/dictionaries, truncating strings), or similar to resource tombstoning, electing not to guarantee that the default event is actually emitted in the same transaction that destroyed the resource. Either way this doesn't need to block this FLIP, but we should document it in the proposal, so I am looking for feedback/discussion about which choice people prefer.

[dsainati1]

We discussed this in the LDM today, and the sentiment generally seems positive. I'll leave this open for a few more days, but if there are no more comments before then, we will consider this approved.

[turbolent]

In the future we could maybe allow addition on UInt, all arithmetic on Int, and saturating arithmetic on all numeric types

[turbolent]

🧹 🗑️ 👌