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

Add missing requirements to ERC777 #2212

Merged

Conversation

nventuro
Copy link
Contributor

Fixes #2208 for the v2.5.0 branch.

#2027 made _send, _approve, _callTokensToSend and _callTokensReceived internal. We failed to identify that some of these functions contained arguments that were not validated: when they were private, this validation was performed at their callsite. This code was included in the v2.5.0 release.

Then, #2134 got merged, which made the hook methods private again, and removed the operator argument from _send (the one that was unchecked). This code was published in v3.0.0.

This PR addresses the issues on the v2.5.0 release (and targets that branch). Once merged, v2.5.1 will be released and the branch merged back into master, only keeping the fix for _approve (the only unprotected function in v3.0.0). That will then become v3.0.1.

Because of this, I only added tests for the require statement in _approve: the other one will only exist in the v2.5.1 branch.

@nventuro nventuro requested a review from frangio April 22, 2020 20:15
@nventuro
Copy link
Contributor Author

I forgot to add: I don't think we should add any checks to the hook functions. All they do is call into other contracts, which should have their own validations in place. No token contract should call these hooks outside of a token transfer regardless, which is why we made the functions private for v3.0.0.

@nventuro nventuro merged commit c75b016 into OpenZeppelin:release-v2.5.0 Apr 24, 2020
@nventuro nventuro deleted the erc777-add-requirements branch April 24, 2020 18:33
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.

2 participants