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

Fix SafeERC20.safeApprove bug #1647

Merged
merged 1 commit into from
Feb 26, 2019

Conversation

nventuro
Copy link
Contributor

@nventuro nventuro commented Feb 25, 2019

This fixes a bug reported by @nikeshnazareth (thanks a lot!), where SafeERC20.safeApprove performs a safety check using the allowance of the contract caller, instead of the contract's own address. I also improved the SafeERC20 mock 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 approve by forcing usage under safe conditions, and the only consequence is that this feature was incorrectly implemented. And even then, the approve bug can only be exploited under specific conditions, has been known for a very long time, and only affects the user calling approve.

We will be releasing a hotfix for both v2.1 and v2.0 (to support both Solidity v0.5.x and v0.4.25).

@nventuro nventuro added bug contracts Smart contract code. labels Feb 25, 2019
@nventuro nventuro self-assigned this Feb 25, 2019
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.

LGTM. Thanks again for reporting @nikeshnazareth!

@nventuro nventuro merged commit 3111291 into OpenZeppelin:master Feb 26, 2019
@nventuro nventuro deleted the safeerc20-bugfix branch February 26, 2019 15:39
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
Labels
bug contracts Smart contract code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants