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.

[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]

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 )

[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 :ok_hand:)

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.