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 a6889776f4.

* updates

* fixes #1404

* approve failing test

* suggested changes done

* ISafeERC20 removed

* allowance methods in library

* Improved SafeERC20 tests.

* Fixed test coverage.
pull/1451/head
Aniket 6 years ago committed by Nicolás Venturo
parent 67dac7ae99
commit 315f426f5c
  1. 58
      contracts/mocks/SafeERC20Helper.sol
  2. 29
      contracts/token/ERC20/SafeERC20.sol
  3. 90
      test/token/ERC20/SafeERC20.test.js

@ -3,10 +3,8 @@ pragma solidity ^0.4.24;
import "../token/ERC20/IERC20.sol"; import "../token/ERC20/IERC20.sol";
import "../token/ERC20/SafeERC20.sol"; import "../token/ERC20/SafeERC20.sol";
contract ERC20FailingMock is IERC20 { contract ERC20FailingMock {
function totalSupply() public view returns (uint256) { uint256 private _allowance;
return 0;
}
function transfer(address, uint256) public returns (bool) { function transfer(address, uint256) public returns (bool) {
return false; return false;
@ -20,19 +18,13 @@ contract ERC20FailingMock is IERC20 {
return false; return false;
} }
function balanceOf(address) public view returns (uint256) {
return 0;
}
function allowance(address, address) public view returns (uint256) { function allowance(address, address) public view returns (uint256) {
return 0; return 0;
} }
} }
contract ERC20SucceedingMock is IERC20 { contract ERC20SucceedingMock {
function totalSupply() public view returns (uint256) { uint256 private _allowance;
return 0;
}
function transfer(address, uint256) public returns (bool) { function transfer(address, uint256) public returns (bool) {
return true; return true;
@ -46,12 +38,12 @@ contract ERC20SucceedingMock is IERC20 {
return true; return true;
} }
function balanceOf(address) public view returns (uint256) { function setAllowance(uint256 allowance_) public {
return 0; _allowance = allowance_;
} }
function allowance(address, address) public view returns (uint256) { function allowance(address, address) public view returns (uint256) {
return 0; return _allowance;
} }
} }
@ -62,10 +54,12 @@ contract SafeERC20Helper {
IERC20 private _succeeding; IERC20 private _succeeding;
constructor() public { constructor() public {
_failing = new ERC20FailingMock(); _failing = IERC20(new ERC20FailingMock());
_succeeding = new ERC20SucceedingMock(); _succeeding = IERC20(new ERC20SucceedingMock());
} }
// Using _failing
function doFailingTransfer() public { function doFailingTransfer() public {
_failing.safeTransfer(address(0), 0); _failing.safeTransfer(address(0), 0);
} }
@ -78,6 +72,16 @@ contract SafeERC20Helper {
_failing.safeApprove(address(0), 0); _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 { function doSucceedingTransfer() public {
_succeeding.safeTransfer(address(0), 0); _succeeding.safeTransfer(address(0), 0);
} }
@ -86,7 +90,23 @@ contract SafeERC20Helper {
_succeeding.safeTransferFrom(address(0), address(0), 0); _succeeding.safeTransferFrom(address(0), address(0), 0);
} }
function doSucceedingApprove() public { function doSucceedingApprove(uint256 amount) public {
_succeeding.safeApprove(address(0), 0); _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));
} }
} }

@ -10,6 +10,9 @@ import "./IERC20.sol";
* which allows you to call the safe operations as `token.safeTransfer(...)`, etc. * which allows you to call the safe operations as `token.safeTransfer(...)`, etc.
*/ */
library SafeERC20 { library SafeERC20 {
using SafeMath for uint256;
function safeTransfer( function safeTransfer(
IERC20 token, IERC20 token,
address to, address to,
@ -38,6 +41,32 @@ library SafeERC20 {
) )
internal 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)); 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));
}
} }

@ -10,27 +10,85 @@ contract('SafeERC20', function () {
this.helper = await SafeERC20Helper.new(); this.helper = await SafeERC20Helper.new();
}); });
it('should throw on failed transfer', async function () { describe('with token that returns false on all calls', function () {
await shouldFail.reverting(this.helper.doFailingTransfer()); it('reverts on transfer', async function () {
}); await shouldFail.reverting(this.helper.doFailingTransfer());
});
it('should throw on failed transferFrom', async function () { it('reverts on transferFrom', async function () {
await shouldFail.reverting(this.helper.doFailingTransferFrom()); await shouldFail.reverting(this.helper.doFailingTransferFrom());
}); });
it('should throw on failed approve', async function () { it('reverts on approve', async function () {
await shouldFail.reverting(this.helper.doFailingApprove()); await shouldFail.reverting(this.helper.doFailingApprove());
}); });
it('should not throw on succeeding transfer', async function () { it('reverts on increaseAllowance', async function () {
await this.helper.doSucceedingTransfer(); await shouldFail.reverting(this.helper.doFailingIncreaseAllowance());
}); });
it('should not throw on succeeding transferFrom', async function () { it('reverts on decreaseAllowance', async function () {
await this.helper.doSucceedingTransferFrom(); await shouldFail.reverting(this.helper.doFailingDecreaseAllowance());
});
}); });
it('should not throw on succeeding approve', async function () { describe('with token that returns true on all calls', function () {
await this.helper.doSucceedingApprove(); 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));
});
});
});
}); });
}); });

Loading…
Cancel
Save