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.

Sign up for GitHub

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

Jump to bottom

Improve SafeERC20 allowance handling #1404

Closed
nventuro opened this issue Oct 10, 2018 · 1 comment
Closed

Improve SafeERC20 allowance handling #1404

nventuro opened this issue Oct 10, 2018 · 1 comment
Labels
breaking change Changes that break backwards compatibility of the public API. contracts Smart contract code.
Milestone
v2.0

Comments

@nventuro
Copy link
Contributor

nventuro commented Oct 10, 2018 •
edited

Currently, SafeERC20's allowance-related functionality is restricted to a safeApprove function, which is vulnerable to the well-known non-zero approval issue in the ERC20 spec. We should:

  • add safeIncreaseAllowance and safeDecreaseAllowance, with require on the return value of the ERC20 functions
  • add extra requires to safeApprove so that it only works if the new or old allowance are 0 (i.e. it only sets an initial allowance, or sets it to 0, which are the only safe scenarios).

This way, we'll be guiding users to the best practices regarding ERC20 tokens.

Props to @cwhinfrey and the Level K team for suggesting this!
@nventuro nventuro added contracts Smart contract code. breaking change Changes that break backwards compatibility of the public API. labels Oct 10, 2018
@nventuro nventuro added this to the v2.0 milestone Oct 10, 2018
@nventuro
Copy link
Contributor Author

@Aniket-Engg this is a great one for you to take, if you're up to it :)

Aniket-Engg added a commit to Aniket-Engg/zeppelin-solidity that referenced this issue Oct 11, 2018
@Aniket-Engg
fixes OpenZeppelin#1404
23df893
@Aniket-Engg Aniket-Engg mentioned this issue Oct 11, 2018
Merged
Aniket-Engg added a commit to Aniket-Engg/zeppelin-solidity that referenced this issue Oct 16, 2018
@Aniket-Engg
Merge branch 'fix/OpenZeppelin#1404' of https://github.com/Aniket-Eng…
998dfd7 
…g/zeppelin-solidity
nventuro pushed a commit that referenced this issue Oct 18, 2018
@Aniket-Engg @nventuro
Improved SafeERC20 allowance handling (#1407)
315f426 
* signing prefix added

* Minor improvement

* Tests changed

* Successfully tested

* Minor improvements

* Minor improvements

* Revert "Dangling commas are now required. (#1359)"

This reverts commit a688977.

* updates

* fixes #1404

* approve failing test

* suggested changes done

* ISafeERC20 removed

* allowance methods in library

* Improved SafeERC20 tests.

* Fixed test coverage.
come-maiz pushed a commit that referenced this issue Oct 21, 2018
@Aniket-Engg @come-maiz
Improved SafeERC20 allowance handling (#1407)
1b27b4b 
* signing prefix added

* Minor improvement

* Tests changed

* Successfully tested

* Minor improvements

* Minor improvements

* Revert "Dangling commas are now required. (#1359)"

This reverts commit a688977.

* updates

* fixes #1404

* approve failing test

* suggested changes done

* ISafeERC20 removed

* allowance methods in library

* Improved SafeERC20 tests.

* Fixed test coverage.

(cherry picked from commit 315f426)
nventuro pushed a commit that referenced this issue Nov 1, 2018
@Aniket-Engg @nventuro
The beneficiary parameter of claimRefund is replaced with refundee (#…
4f356e3 
…1481)

* signing prefix added

* Minor improvement

* Tests changed

* Successfully tested

* Minor improvements

* Minor improvements

* Revert "Dangling commas are now required. (#1359)"

This reverts commit a688977.

* updates

* fixes #1404

* approve failing test

* suggested changes done

* ISafeERC20 removed

* conflict fixes

* fixes #1456
nventuro pushed a commit that referenced this issue Nov 1, 2018
@Aniket-Engg @nventuro
SafeMath Test Coverage Improved (#1477)
b5892a2 
* signing prefix added

* Minor improvement

* Tests changed

* Successfully tested

* Minor improvements

* Minor improvements

* Revert "Dangling commas are now required. (#1359)"

This reverts commit a688977.

* updates

* fixes #1404

* approve failing test

* suggested changes done

* ISafeERC20 removed

* conflict fixes

* fixes #1386

* Update SafeMath.test.js
frangio pushed a commit that referenced this issue Dec 5, 2018
@Aniket-Engg @frangio
getter added for an array of tokens held by an owner (#1522)
5caecf5 
* signing prefix added

* Minor improvement

* Tests changed

* Successfully tested

* Minor improvements

* Minor improvements

* Revert "Dangling commas are now required. (#1359)"

This reverts commit a688977.

* updates

* fixes #1404

* approve failing test

* suggested changes done

* ISafeERC20 removed

* conflict fixes

* fixes #1512

* Update test/token/ERC721/ERC721Full.test.js

Co-Authored-By: Aniket-Engg <30843294+Aniket-Engg@users.noreply.github.com>
nventuro pushed a commit that referenced this issue Dec 7, 2018
@Aniket-Engg @nventuro
Test setup helper added (#1482)
70fd243 
* signing prefix added

* Minor improvement

* Tests changed

* Successfully tested

* Minor improvements

* Minor improvements

* Revert "Dangling commas are now required. (#1359)"

This reverts commit a688977.

* updates

* fixes #1404

* approve failing test

* suggested changes done

* ISafeERC20 removed

* conflict fixes

* fixes #1205

* minor change

* suggested changes

* reviewed changes

* final update
Aniket-Engg added a commit to Aniket-Engg/zeppelin-solidity that referenced this issue Dec 21, 2018
@Aniket-Engg
fixes OpenZeppelin#1404
9bb5670
frangio pushed a commit that referenced this issue Jan 4, 2019
@Aniket-Engg @frangio
BigNumber comparison improved (#1581)
350aae0 
* signing prefix added

* Minor improvement

* Successfully tested

* Minor improvements

* Minor improvements

* Revert "Dangling commas are now required. (#1359)"

This reverts commit a688977.

* updates

* fixes #1404

* approve failing test

* suggested changes done

* ISafeERC20 removed

* conflict fixes

* added examples

* fixes #706

* linting

* fixes #204

* file fixing

* deep bignumber comparison removed

* Update SafeERC20Helper.sol

* Update IERC20.sol

* Update SafeERC20.sol

* Update package-lock.json

* Revert "deep bignumber comparison removed"

This reverts commit 230b272.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
breaking change Changes that break backwards compatibility of the public API. contracts Smart contract code.
Projects
None yet
Milestone
v2.0
Development

No branches or pull requests
1 participant
@nventuro