Fix SafeERC20.safeApprove bug #1647
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
frangio
approved these changes
Feb 25, 2019
nventuro
added a commit
that referenced
this pull request
Feb 26, 2019
Fix SafeERC20.safeApprove bug (cherry picked from commit 3111291)
nventuro
added a commit
that referenced
this pull request
Feb 26, 2019
Fix SafeERC20.safeApprove bug (cherry picked from commit 3111291)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes a bug reported by @nikeshnazareth (thanks a lot!), where
SafeERC20.safeApproveperforms a safety check using the allowance of the contract caller, instead of the contract's own address. I also improved theSafeERC20mock contracts so that the tests fail with the old code and pass with the new one (but still kept the mock as simple as possible).This check was added in v2.0 (see #1407) as a recommendation from @cwhinfrey and the LevelK team during their audit of OpenZeppelin v2.0 (which was unreleased at the time). As such, we consider this to be a minor bug, since the safety check's purpose was discouraging unsafe use of
approveby forcing usage under safe conditions, and the only consequence is that this feature was incorrectly implemented. And even then, theapprovebug can only be exploited under specific conditions, has been known for a very long time, and only affects the user callingapprove.We will be releasing a hotfix for both v2.1 and v2.0 (to support both Solidity v0.5.x and v0.4.25).