From 315f426f5c2db7953b977bbf605b5d796636cd97 Mon Sep 17 00:00:00 2001 From: Aniket <30843294+Aniket-Engg@users.noreply.github.com> Date: Thu, 18 Oct 2018 20:55:03 +0530 Subject: [PATCH] Improved SafeERC20 allowance handling (#1407) * signing prefix added * Minor improvement * Tests changed * Successfully tested * Minor improvements * Minor improvements * Revert "Dangling commas are now required. (#1359)" This reverts commit a6889776f46adca374b6ebf014aa7b0038112a9d. * updates * fixes #1404 * approve failing test * suggested changes done * ISafeERC20 removed * allowance methods in library * Improved SafeERC20 tests. * Fixed test coverage. --- contracts/mocks/SafeERC20Helper.sol | 58 +++++++++++++------ contracts/token/ERC20/SafeERC20.sol | 29 ++++++++++ test/token/ERC20/SafeERC20.test.js | 90 ++++++++++++++++++++++++----- 3 files changed, 142 insertions(+), 35 deletions(-) diff --git a/contracts/mocks/SafeERC20Helper.sol b/contracts/mocks/SafeERC20Helper.sol index c80c762d92f..7e639f86f2e 100644 --- a/contracts/mocks/SafeERC20Helper.sol +++ b/contracts/mocks/SafeERC20Helper.sol @@ -3,10 +3,8 @@ pragma solidity ^0.4.24; import "../token/ERC20/IERC20.sol"; import "../token/ERC20/SafeERC20.sol"; -contract ERC20FailingMock is IERC20 { - function totalSupply() public view returns (uint256) { - return 0; - } +contract ERC20FailingMock { + uint256 private _allowance; function transfer(address, uint256) public returns (bool) { return false; @@ -20,19 +18,13 @@ contract ERC20FailingMock is IERC20 { return false; } - function balanceOf(address) public view returns (uint256) { - return 0; - } - function allowance(address, address) public view returns (uint256) { return 0; } } -contract ERC20SucceedingMock is IERC20 { - function totalSupply() public view returns (uint256) { - return 0; - } +contract ERC20SucceedingMock { + uint256 private _allowance; function transfer(address, uint256) public returns (bool) { return true; @@ -46,12 +38,12 @@ contract ERC20SucceedingMock is IERC20 { return true; } - function balanceOf(address) public view returns (uint256) { - return 0; + function setAllowance(uint256 allowance_) public { + _allowance = allowance_; } function allowance(address, address) public view returns (uint256) { - return 0; + return _allowance; } } @@ -62,10 +54,12 @@ contract SafeERC20Helper { IERC20 private _succeeding; constructor() public { - _failing = new ERC20FailingMock(); - _succeeding = new ERC20SucceedingMock(); + _failing = IERC20(new ERC20FailingMock()); + _succeeding = IERC20(new ERC20SucceedingMock()); } + // Using _failing + function doFailingTransfer() public { _failing.safeTransfer(address(0), 0); } @@ -78,6 +72,16 @@ contract SafeERC20Helper { _failing.safeApprove(address(0), 0); } + function doFailingIncreaseAllowance() public { + _failing.safeIncreaseAllowance(address(0), 0); + } + + function doFailingDecreaseAllowance() public { + _failing.safeDecreaseAllowance(address(0), 0); + } + + // Using _succeeding + function doSucceedingTransfer() public { _succeeding.safeTransfer(address(0), 0); } @@ -86,7 +90,23 @@ contract SafeERC20Helper { _succeeding.safeTransferFrom(address(0), address(0), 0); } - function doSucceedingApprove() public { - _succeeding.safeApprove(address(0), 0); + function doSucceedingApprove(uint256 amount) public { + _succeeding.safeApprove(address(0), amount); + } + + function doSucceedingIncreaseAllowance(uint256 amount) public { + _succeeding.safeIncreaseAllowance(address(0), amount); + } + + function doSucceedingDecreaseAllowance(uint256 amount) public { + _succeeding.safeDecreaseAllowance(address(0), amount); + } + + function setAllowance(uint256 allowance_) public { + ERC20SucceedingMock(_succeeding).setAllowance(allowance_); + } + + function allowance() public view returns (uint256) { + return _succeeding.allowance(address(0), address(0)); } } diff --git a/contracts/token/ERC20/SafeERC20.sol b/contracts/token/ERC20/SafeERC20.sol index 10c6ff2bcf6..9e6bf6a2097 100644 --- a/contracts/token/ERC20/SafeERC20.sol +++ b/contracts/token/ERC20/SafeERC20.sol @@ -10,6 +10,9 @@ import "./IERC20.sol"; * which allows you to call the safe operations as `token.safeTransfer(...)`, etc. */ library SafeERC20 { + + using SafeMath for uint256; + function safeTransfer( IERC20 token, address to, @@ -38,6 +41,32 @@ library SafeERC20 { ) internal { + // safeApprove should only be called when setting an initial allowance, + // or when resetting it to zero. To increase and decrease it, use + // 'safeIncreaseAllowance' and 'safeDecreaseAllowance' + require((value == 0) || (token.allowance(msg.sender, spender) == 0)); require(token.approve(spender, value)); } + + function safeIncreaseAllowance( + IERC20 token, + address spender, + uint256 value + ) + internal + { + uint256 newAllowance = token.allowance(address(this), spender).add(value); + require(token.approve(spender, newAllowance)); + } + + function safeDecreaseAllowance( + IERC20 token, + address spender, + uint256 value + ) + internal + { + uint256 newAllowance = token.allowance(address(this), spender).sub(value); + require(token.approve(spender, newAllowance)); + } } diff --git a/test/token/ERC20/SafeERC20.test.js b/test/token/ERC20/SafeERC20.test.js index b6c87554b41..c8dea73822b 100644 --- a/test/token/ERC20/SafeERC20.test.js +++ b/test/token/ERC20/SafeERC20.test.js @@ -10,27 +10,85 @@ contract('SafeERC20', function () { this.helper = await SafeERC20Helper.new(); }); - it('should throw on failed transfer', async function () { - await shouldFail.reverting(this.helper.doFailingTransfer()); - }); + describe('with token that returns false on all calls', function () { + it('reverts on transfer', async function () { + await shouldFail.reverting(this.helper.doFailingTransfer()); + }); - it('should throw on failed transferFrom', async function () { - await shouldFail.reverting(this.helper.doFailingTransferFrom()); - }); + it('reverts on transferFrom', async function () { + await shouldFail.reverting(this.helper.doFailingTransferFrom()); + }); - it('should throw on failed approve', async function () { - await shouldFail.reverting(this.helper.doFailingApprove()); - }); + it('reverts on approve', async function () { + await shouldFail.reverting(this.helper.doFailingApprove()); + }); - it('should not throw on succeeding transfer', async function () { - await this.helper.doSucceedingTransfer(); - }); + it('reverts on increaseAllowance', async function () { + await shouldFail.reverting(this.helper.doFailingIncreaseAllowance()); + }); - it('should not throw on succeeding transferFrom', async function () { - await this.helper.doSucceedingTransferFrom(); + it('reverts on decreaseAllowance', async function () { + await shouldFail.reverting(this.helper.doFailingDecreaseAllowance()); + }); }); - it('should not throw on succeeding approve', async function () { - await this.helper.doSucceedingApprove(); + describe('with token that returns true on all calls', function () { + it('doesn\'t revert on transfer', async function () { + await this.helper.doSucceedingTransfer(); + }); + + it('doesn\'t revert on transferFrom', async function () { + await this.helper.doSucceedingTransferFrom(); + }); + + describe('approvals', function () { + context('with zero allowance', function () { + beforeEach(async function () { + await this.helper.setAllowance(0); + }); + + it('doesn\'t revert when approving a non-zero allowance', async function () { + await this.helper.doSucceedingApprove(100); + }); + + it('doesn\'t revert when approving a zero allowance', async function () { + await this.helper.doSucceedingApprove(0); + }); + + it('doesn\'t revert when increasing the allowance', async function () { + await this.helper.doSucceedingIncreaseAllowance(10); + }); + + it('reverts when decreasing the allowance', async function () { + await shouldFail.reverting(this.helper.doSucceedingDecreaseAllowance(10)); + }); + }); + + context('with non-zero allowance', function () { + beforeEach(async function () { + await this.helper.setAllowance(100); + }); + + it('reverts when approving a non-zero allowance', async function () { + await shouldFail.reverting(this.helper.doSucceedingApprove(20)); + }); + + it('doesn\'t revert when approving a zero allowance', async function () { + await this.helper.doSucceedingApprove(0); + }); + + it('doesn\'t revert when increasing the allowance', async function () { + await this.helper.doSucceedingIncreaseAllowance(10); + }); + + it('doesn\'t revert when decreasing the allowance to a positive value', async function () { + await this.helper.doSucceedingDecreaseAllowance(50); + }); + + it('reverts when decreasing the allowance to a negative value', async function () { + await shouldFail.reverting(this.helper.doSucceedingDecreaseAllowance(200)); + }); + }); + }); }); });