Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implicitly clear ERC721 approval on transfers #3481

Merged
merged 4 commits into from
Jul 4, 2022

Conversation

AnAllergyToAnalogy
Copy link
Contributor

@AnAllergyToAnalogy AnAllergyToAnalogy commented Jun 16, 2022

As per the ERC721 standard, the transfer function shouldn't emit an Approve event, as clearing the approval is implicit in the Transfer event.

Docs regarding Approve event:

    /// @dev This emits when the approved address for an NFT is changed or
    ///  reaffirmed. The zero address indicates there is no approved address.
    ///  When a Transfer event emits, this also indicates that the approved
    ///  address for that NFT (if any) is reset to none.

EIP-721

Fixes #3484

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

As per the ERC721 standard, the transfer function shouldn't emit an Approve event, as clearing the approval is implicit in the Transfer event.

Docs regarding Approve event:
```
    /// @dev This emits when the approved address for an NFT is changed or
    ///  reaffirmed. The zero address indicates there is no approved address.
    ///  When a Transfer event emits, this also indicates that the approved
    ///  address for that NFT (if any) is reset to none.
```
[EIP-721](https://github.com/ethereum/EIPs/blob/master/EIPS/eip-721.md)
@Amxx
Copy link
Collaborator

Amxx commented Jun 16, 2022

Hello @AnAllergyToAnalogy

I think we already had this discussion sometime ago.

The specification for the Approval event are

/// @dev This emits when the approved address for an NFT is changed or
/// reaffirmed. The zero address indicates there is no approved address.
/// When a Transfer event emits, this also indicates that the approved
/// address for that NFT (if any) is reset to none.
event Approval(address indexed _owner, address indexed _approved, uint256 indexed _tokenId);

Which can indeed be interpreted as "Transfer event implies approval is reset, so no approval event is required".
Still, I believe many systems are expecting the Approval event to also be triggered.

Removing it might break things, but one could argue that the "things" in question should follow the ERC and not our implementation. The question is then, is it clear enough to everybody that they should not expect an Approval event on transfers?

@frangio

@AnAllergyToAnalogy
Copy link
Contributor Author

I would argue that things are currently broken as technically the contract doesn't properly adhere to the standard. Non-OpenZepp ERC721 implementations won't be emitting the event on Transfers, so any 3rd party projects would need to account for that anyway. I would say the onus is on other projects to adhere to the standard as written and not just common implementations.

As a side note, not emitting the event saves about 2100 gas. Given the vast majority of ERC721s no mainnet use OpenZepp, and given gas prices and NFT usage over the last few years, this one bug has probably cost a couple of hundred million dollars of wasted gas. It's probably worth patching.

@frangio
Copy link
Contributor

frangio commented Jun 16, 2022

This is absolutely not a bug. Implementing additional behavior than that required by the standard is okay as long as it's consistent with the rules in it. That approval reset is implied doesn't mean it's wrong to explicitly emit the event.

Regardless, I agree this is worth considering because of the large gas savings. I agree that systems integrating with the standard should not be expecting an Approve event.

@frangio frangio added this to the 4.8 milestone Jun 16, 2022
@AnAllergyToAnalogy
Copy link
Contributor Author

@frangio you're right that additional behavior not mentioned in the rules isn't contrary to the standard, but i think in the case of this event since it explicitly says not to emit it, it would be considered a bug.

I think beyond the gas it would be low-impact, given it's just an event, but for any system that's using events in even a superficial way it'll produce anomolous behavior that needs to be accounted for. For example, an interface using events to show an NFT's activity would falsely show that the approval was cleared each time it was transferred.

@Amxx
Copy link
Collaborator

Amxx commented Jun 17, 2022

would falsely show that the approval was cleared each time it was transferred.

... but the approval was cleared each time it was transferred.

The reset happens, that is clear. Whether it's notified by the Transfer event itself, or by an additional Approval event, that doesn't change the reality that adddess(0) is now approved for this token. The specs don't require the Approval to be emitted only if the approved address changes, but also if it's reaffirmed.

So yes, emitting an Approval event is wasteful in terms of gas, but no, it's not a bug

@AnAllergyToAnalogy
Copy link
Contributor Author

Let me rephrase, it implies that the user called the approval function to manually clear the approval. Just because the impact is small doesn't make it not a bug. I discovered this bug because I was running unit tests on a project that inherits OpenZeppelin ERC721 contracts, and it was anomalously emitting Approval events when it shouldn't.

The spec is very clear that Approval shouldn't be emitted on token transfer,

    ///  When a Transfer event emits, this also indicates that the approved
    ///  address for that NFT (if any) is reset to none.

At the moment the OpenZeppelin contracts aren't fully ERC721 compliant.

@Amxx
Copy link
Collaborator

Amxx commented Jun 17, 2022

it implies that the user called the approval function to manually clear the approval

I'll have to disagree on that.

The ERC states that the event should be emitted when the approved address for an NFT is changed or reaffirmed. Apart from the reset being implicit on transfer, it says nothing about the circumstances leading to this change (or reaffirmation) that trigger the event. In particular, some contracts may have logic that set the approval through a workflow that doesn't involve approve() (for example using social recovery), and in that case, we would expect an Approval event.

@Amxx
Copy link
Collaborator

Amxx commented Jun 20, 2022

✅ Tests are good.
❌ We need a changelog entry (and possibly a backward compatibility warning?)

Otherwise, code is good for me.

@frangio, do you see anything more needed?

@frangio
Copy link
Contributor

frangio commented Jun 23, 2022

No. Agree with the changelog and compatibility warning.

frangio
frangio previously requested changes Jul 1, 2022
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any chance you can add the things we mentioned above, or should we go ahead and do it?

Sorry, realizing it may be better for us to do it and get the wording how we want it.

@frangio frangio dismissed their stale review July 1, 2022 23:17

Addressed

@frangio frangio changed the title Implicitly clear approval Implicitly clear ERC721 approval on transfers Jul 1, 2022
@frangio frangio requested a review from Amxx July 1, 2022 23:17
@frangio frangio merged commit e02c378 into OpenZeppelin:master Jul 4, 2022
ronhuafeng added a commit to ronhuafeng/openzeppelin-contracts that referenced this pull request Sep 9, 2022
Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Co-authored-by: Francisco Giordano <frangio.1@gmail.com>
@Amxx Amxx mentioned this pull request Apr 4, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ERC721 transfers should not emit Approval events
3 participants