openzeppelin-contracts icon indicating copy to clipboard operation
openzeppelin-contracts copied to clipboard

ERC721 - Restricted zero address transfer issue in regards to how opensea treats dead addresses

Open kanedaaaa opened this issue 2 years ago • 2 comments

In the last year or so, I had to work with many ERC721 projects and in almost every single one, I have to tackle the absence of a standard way of burning tokens, which is sending to black hole (0x00...000) address.

I'm pretty sure everyone is aware of this ongoing issue (from my POV it's a huge issue, will explain why below), but still, in ERC721.sol (Same goes for ERC20), zero address transfers are restricted in internal _transfer function (the only way to transfer assets):

require(to != address(0), "ERC721: transfer to the zero address");

I know, I know someone already made an issue about this years ago, that was closed and I decided to open a new one, just to make the old responses clear, exactly why do we avoid removing it, and making it optional? I also know that there is an extension, I also know that developers can expose internal _burn into public function, but why exactly? It's one line VS. extensions and function. (Not arguing here, just trying to understand exactly what is the reason)

Yes, I'm aware that 0x0 isn't only burning address, but the issue comes when we see how Opensea, currently N1 NFT marketplace doesn't recognize other burn addresses, as a black hole (0x0) and won't remove items from the collection page (neither from stats) if it's sent to those mentioned addresses.

Over time, I just keep telling myself that devs will learn, they will eventually start implementing burn functions or using extensions to cover that functionality, but it just keeps popping out.

Again, I don't want to create a huge problem from something simple like this, but since Opensea won't listen to us, I'm sure I can have a chance to make this point here.

P.S Can we at least outline the fact (somewhere in contract, or documentation) that devs must implement burn function for future use?

kanedaaaa avatar Mar 06 '22 14:03 kanedaaaa

Can you explain the importance of burning for NFTs? Why would we recommend that devs "must" implement a burn function? The way we see it burning is a feature that may or may not be relevant for a particular NFT, and a dev should make this choice when they are developing the contract. We don't include a burn function by default because it is not part of ERC721.

One of the things we could very easily do is enable "Burnable" by default in our ERC721 Wizard. A change like this to the contracts is going to require a lot more discussion.


I know someone already made an issue about this years ago

Can you share the link to this previous discussion?

frangio avatar Mar 08 '22 03:03 frangio

Lately, it has become a trend, and usually marketing tool to offer some advantages, in exchange for the burn. And in general, due to the problem I described (Opensea), it's impossible (Not we can implement later, but literally impossible) to burn the tokens properly. Something that comes in need for many projects.

I do agree Must is an exaggerated term to use, I would prefer to call it Consider to add a given feature, due to the reasons mentioned above. For example, adding such a comment in Wizard. Since usually it is being used by less experienced people who do not realize the importance of such add-ons.

For example, pausable modification has this kind of explanation: Privileged accounts will be able to pause the functionality marked as whenNotPaused. Useful for emergency response.

Would be nice to add that useful comment as well, such as Useful for future use-cases. (Not the best one, but you get the point)

Again, not asking for a change, just notice or comment, that will be sufficient. No need to alter the original implementation just because Opensea won't fix their stuff properly. BUT if it comes to modifying OG implementation, my vote would definitely go to removing require(to != address(0)) (Which i'm not asking for, since I understood that it poses some security risks from old discussion)

BTW, here is the old discussion about ERC20 burning

kanedaaaa avatar Mar 08 '22 15:03 kanedaaaa

Closing due to unclear actionable. If there is a concrete suggestion for documentation please submit a pull request.

frangio avatar Dec 02 '22 21:12 frangio