NoBey/openzeppelin-contracts
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Add no-return-data ERC20 support to SafeERC20. (#1655)

Browse Source 
* Add no-return-data ERC20 support to SafeERC20.

* Add changelog entry.

* Replace abi.encodeWithSignature for encodeWithSelector.

* Remove SafeERC20 test code duplication.

* Replace assembly for abi.decode.

* Fix linter errors.
This commit is contained in:
Nicolás Venturo
2019-02-28 17:34:46 -03:00
committed by GitHub
parent 0dded493a0
commit 41aa39afbc
4 changed files with 153 additions and 105 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
CHANGELOG.md
View File

@ -6,6 +6,7 @@
* `ERC20`: added internal `_approve(address owner, address spender, uint256 value)`, allowing derived contracts to set the allowance of arbitrary accounts. ([#1609](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1609))
* `ERC20Metadata`: added internal `_setTokenURI(string memory tokenURI)`. ([#1618](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1618))
* `ERC20Snapshot`: create snapshots on demand of the token balances and total supply, to later retrieve and e.g. calculate dividends at a past time. ([#1617](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1617))
* `SafeERC20`: `ERC20` contracts with no return value (i.e. that revert on failure) are now supported. ([#1655](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/))
### Improvements:
* Upgraded the minimum compiler version to v0.5.2: this removes many Solidity warnings that were false positives. ([#1606](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1606))

100
contracts/mocks/SafeERC20Helper.sol
View File

@ -3,7 +3,7 @@ pragma solidity ^0.5.2;
import "../token/ERC20/IERC20.sol";
import "../token/ERC20/SafeERC20.sol";
contract ERC20FailingMock {
contract ERC20ReturnFalseMock {
uint256 private _allowance;
// IERC20's functions are not pure, but these mock implementations are: to prevent Solidity from issuing warnings,
@ -31,7 +31,7 @@ contract ERC20FailingMock {
}
}
contract ERC20SucceedingMock {
contract ERC20ReturnTrueMock {
mapping (address => uint256) private _allowances;
// IERC20's functions are not pure, but these mock implementations are: to prevent Solidity from issuing warnings,
@ -62,62 +62,68 @@ contract ERC20SucceedingMock {
}
}
contract SafeERC20Helper {
using SafeERC20 for IERC20;
contract ERC20NoReturnMock {
mapping (address => uint256) private _allowances;
IERC20 private _failing;
IERC20 private _succeeding;
// IERC20's functions are not pure, but these mock implementations are: to prevent Solidity from issuing warnings,
// we write to a dummy state variable.
uint256 private _dummy;
constructor () public {
_failing = IERC20(address(new ERC20FailingMock()));
_succeeding = IERC20(address(new ERC20SucceedingMock()));
function transfer(address, uint256) public {
_dummy = 0;
}
function doFailingTransfer() public {
_failing.safeTransfer(address(0), 0);
function transferFrom(address, address, uint256) public {
_dummy = 0;
}
function doFailingTransferFrom() public {
_failing.safeTransferFrom(address(0), address(0), 0);
}
function doFailingApprove() public {
_failing.safeApprove(address(0), 0);
}
function doFailingIncreaseAllowance() public {
_failing.safeIncreaseAllowance(address(0), 0);
}
function doFailingDecreaseAllowance() public {
_failing.safeDecreaseAllowance(address(0), 0);
}
function doSucceedingTransfer() public {
_succeeding.safeTransfer(address(0), 0);
}
function doSucceedingTransferFrom() public {
_succeeding.safeTransferFrom(address(0), 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 approve(address, uint256) public {
_dummy = 0;
}
function setAllowance(uint256 allowance_) public {
ERC20SucceedingMock(address(_succeeding)).setAllowance(allowance_);
_allowances[msg.sender] = allowance_;
}
function allowance(address owner, address) public view returns (uint256) {
return _allowances[owner];
}
}
contract SafeERC20Wrapper {
using SafeERC20 for IERC20;
IERC20 private _token;
constructor (IERC20 token) public {
_token = token;
}
function transfer() public {
_token.safeTransfer(address(0), 0);
}
function transferFrom() public {
_token.safeTransferFrom(address(0), address(0), 0);
}
function approve(uint256 amount) public {
_token.safeApprove(address(0), amount);
}
function increaseAllowance(uint256 amount) public {
_token.safeIncreaseAllowance(address(0), amount);
}
function decreaseAllowance(uint256 amount) public {
_token.safeDecreaseAllowance(address(0), amount);
}
function setAllowance(uint256 allowance_) public {
ERC20ReturnTrueMock(address(_token)).setAllowance(allowance_);
}
function allowance() public view returns (uint256) {
return _succeeding.allowance(address(0), address(0));
return _token.allowance(address(0), address(0));
}
}

34
contracts/token/ERC20/SafeERC20.sol
View File

@ -5,7 +5,10 @@ import "../../math/SafeMath.sol";
/**
* @title SafeERC20
* @dev Wrappers around ERC20 operations that throw on failure.
* @dev Wrappers around ERC20 operations that throw on failure (when the token
* contract returns false). Tokens that return no value (and instead revert or
* throw on failure) are also supported, non-reverting calls are assumed to be
* successful.
* To use this library you can add a `using SafeERC20 for ERC20;` statement to your contract,
* which allows you to call the safe operations as `token.safeTransfer(...)`, etc.
*/
@ -13,11 +16,11 @@ library SafeERC20 {
using SafeMath for uint256;
function safeTransfer(IERC20 token, address to, uint256 value) internal {
require(token.transfer(to, value));
callOptionalReturn(token, abi.encodeWithSelector(token.transfer.selector, to, value));
}
function safeTransferFrom(IERC20 token, address from, address to, uint256 value) internal {
require(token.transferFrom(from, to, value));
callOptionalReturn(token, abi.encodeWithSelector(token.transferFrom.selector, from, to, value));
}
function safeApprove(IERC20 token, address spender, uint256 value) internal {
@ -25,16 +28,35 @@ library SafeERC20 {
// or when resetting it to zero. To increase and decrease it, use
// 'safeIncreaseAllowance' and 'safeDecreaseAllowance'
require((value == 0) || (token.allowance(address(this), spender) == 0));
require(token.approve(spender, value));
callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, 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));
callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, 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));
callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, spender, newAllowance));
}
/**
* @dev Imitates a Solidity high-level call (i.e. a regular function call to a contract), relaxing the requirement
* on the return value: the return value is optional (but if data is returned, it must equal true).
* @param token The token targeted by the call.
* @param data The call data (encoded using abi.encode or one of its variants).
*/
function callOptionalReturn(IERC20 token, bytes memory data) private {
// We need to perform a low level call here, to bypass Solidity's return data size checking mechanism, since
// we're implementing it ourselves.
// solhint-disable-next-line avoid-low-level-calls
(bool success, bytes memory returndata) = address(token).call(data);
require(success);
if (returndata.length > 0) {
require(abi.decode(returndata, (bool)));
}
}
}

123
test/token/ERC20/SafeERC20.test.js
View File

@ -1,91 +1,110 @@
const { shouldFail } = require('openzeppelin-test-helpers');
const SafeERC20Helper = artifacts.require('SafeERC20Helper');
const ERC20ReturnFalseMock = artifacts.require('ERC20ReturnFalseMock');
const ERC20ReturnTrueMock = artifacts.require('ERC20ReturnTrueMock');
const ERC20NoReturnMock = artifacts.require('ERC20NoReturnMock');
const SafeERC20Wrapper = artifacts.require('SafeERC20Wrapper');
contract('SafeERC20', function () {
beforeEach(async function () {
this.helper = await SafeERC20Helper.new();
});
describe('with token that returns false on all calls', function () {
beforeEach(async function () {
this.wrapper = await SafeERC20Wrapper.new((await ERC20ReturnFalseMock.new()).address);
});
it('reverts on transfer', async function () {
await shouldFail.reverting(this.helper.doFailingTransfer());
await shouldFail.reverting(this.wrapper.transfer());
});
it('reverts on transferFrom', async function () {
await shouldFail.reverting(this.helper.doFailingTransferFrom());
await shouldFail.reverting(this.wrapper.transferFrom());
});
it('reverts on approve', async function () {
await shouldFail.reverting(this.helper.doFailingApprove());
await shouldFail.reverting(this.wrapper.approve(0));
});
it('reverts on increaseAllowance', async function () {
await shouldFail.reverting(this.helper.doFailingIncreaseAllowance());
await shouldFail.reverting(this.wrapper.increaseAllowance(0));
});
it('reverts on decreaseAllowance', async function () {
await shouldFail.reverting(this.helper.doFailingDecreaseAllowance());
await shouldFail.reverting(this.wrapper.decreaseAllowance(0));
});
});
describe('with token that returns true on all calls', function () {
it('doesn\'t revert on transfer', async function () {
await this.helper.doSucceedingTransfer();
beforeEach(async function () {
this.wrapper = await SafeERC20Wrapper.new((await ERC20ReturnTrueMock.new()).address);
});
it('doesn\'t revert on transferFrom', async function () {
await this.helper.doSucceedingTransferFrom();
shouldOnlyRevertOnErrors();
});
describe('with token that returns no boolean values', function () {
beforeEach(async function () {
this.wrapper = await SafeERC20Wrapper.new((await ERC20NoReturnMock.new()).address);
});
describe('approvals', function () {
context('with zero allowance', function () {
beforeEach(async function () {
await this.helper.setAllowance(0);
});
shouldOnlyRevertOnErrors();
});
});
it('doesn\'t revert when approving a non-zero allowance', async function () {
await this.helper.doSucceedingApprove(100);
});
function shouldOnlyRevertOnErrors () {
it('doesn\'t revert on transfer', async function () {
await this.wrapper.transfer();
});
it('doesn\'t revert when approving a zero allowance', async function () {
await this.helper.doSucceedingApprove(0);
});
it('doesn\'t revert on transferFrom', async function () {
await this.wrapper.transferFrom();
});
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));
});
describe('approvals', function () {
context('with zero allowance', function () {
beforeEach(async function () {
await this.wrapper.setAllowance(0);
});
context('with non-zero allowance', function () {
beforeEach(async function () {
await this.helper.setAllowance(100);
});
it('doesn\'t revert when approving a non-zero allowance', async function () {
await this.wrapper.approve(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.wrapper.approve(0);
});
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.wrapper.increaseAllowance(10);
});
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.wrapper.decreaseAllowance(10));
});
});
it('doesn\'t revert when decreasing the allowance to a positive value', async function () {
await this.helper.doSucceedingDecreaseAllowance(50);
});
context('with non-zero allowance', function () {
beforeEach(async function () {
await this.wrapper.setAllowance(100);
});
it('reverts when decreasing the allowance to a negative value', async function () {
await shouldFail.reverting(this.helper.doSucceedingDecreaseAllowance(200));
});
it('reverts when approving a non-zero allowance', async function () {
await shouldFail.reverting(this.wrapper.approve(20));
});
it('doesn\'t revert when approving a zero allowance', async function () {
await this.wrapper.approve(0);
});
it('doesn\'t revert when increasing the allowance', async function () {
await this.wrapper.increaseAllowance(10);
});
it('doesn\'t revert when decreasing the allowance to a positive value', async function () {
await this.wrapper.decreaseAllowance(50);
});
it('reverts when decreasing the allowance to a negative value', async function () {
await shouldFail.reverting(this.wrapper.decreaseAllowance(200));
});
});
});
});
}