diff --git a/CHANGELOG.md b/CHANGELOG.md index 3426cbcf7cb..5db2ad7a5d4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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)) diff --git a/contracts/mocks/SafeERC20Helper.sol b/contracts/mocks/SafeERC20Helper.sol index 464b5b7231b..b69d98b528e 100644 --- a/contracts/mocks/SafeERC20Helper.sol +++ b/contracts/mocks/SafeERC20Helper.sol @@ -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 approve(address, uint256) public { + _dummy = 0; } - function doFailingApprove() public { - _failing.safeApprove(address(0), 0); + function setAllowance(uint256 allowance_) public { + _allowances[msg.sender] = allowance_; } - function doFailingIncreaseAllowance() public { - _failing.safeIncreaseAllowance(address(0), 0); + function allowance(address owner, address) public view returns (uint256) { + return _allowances[owner]; } +} + +contract SafeERC20Wrapper { + using SafeERC20 for IERC20; + + IERC20 private _token; - function doFailingDecreaseAllowance() public { - _failing.safeDecreaseAllowance(address(0), 0); + constructor (IERC20 token) public { + _token = token; } - function doSucceedingTransfer() public { - _succeeding.safeTransfer(address(0), 0); + function transfer() public { + _token.safeTransfer(address(0), 0); } - function doSucceedingTransferFrom() public { - _succeeding.safeTransferFrom(address(0), address(0), 0); + function transferFrom() public { + _token.safeTransferFrom(address(0), address(0), 0); } - function doSucceedingApprove(uint256 amount) public { - _succeeding.safeApprove(address(0), amount); + function approve(uint256 amount) public { + _token.safeApprove(address(0), amount); } - function doSucceedingIncreaseAllowance(uint256 amount) public { - _succeeding.safeIncreaseAllowance(address(0), amount); + function increaseAllowance(uint256 amount) public { + _token.safeIncreaseAllowance(address(0), amount); } - function doSucceedingDecreaseAllowance(uint256 amount) public { - _succeeding.safeDecreaseAllowance(address(0), amount); + function decreaseAllowance(uint256 amount) public { + _token.safeDecreaseAllowance(address(0), amount); } function setAllowance(uint256 allowance_) public { - ERC20SucceedingMock(address(_succeeding)).setAllowance(allowance_); + 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)); } } diff --git a/contracts/token/ERC20/SafeERC20.sol b/contracts/token/ERC20/SafeERC20.sol index bc735e96a21..c7c6efb22ab 100644 --- a/contracts/token/ERC20/SafeERC20.sol +++ b/contracts/token/ERC20/SafeERC20.sol @@ -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))); + } } } diff --git a/test/token/ERC20/SafeERC20.test.js b/test/token/ERC20/SafeERC20.test.js index cc3d35fb15b..9acc3a446de 100644 --- a/test/token/ERC20/SafeERC20.test.js +++ b/test/token/ERC20/SafeERC20.test.js @@ -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(); + }); +}); + +function shouldOnlyRevertOnErrors () { + it('doesn\'t revert on transfer', async function () { + await this.wrapper.transfer(); + }); + + it('doesn\'t revert on transferFrom', async function () { + await this.wrapper.transferFrom(); + }); + + describe('approvals', function () { + context('with zero allowance', function () { + beforeEach(async function () { + await this.wrapper.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 non-zero allowance', async function () { + await this.wrapper.approve(100); + }); - it('doesn\'t revert when approving a zero allowance', async function () { - await this.helper.doSucceedingApprove(0); - }); + 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.helper.doSucceedingIncreaseAllowance(10); - }); + it('doesn\'t revert when increasing the allowance', async function () { + await this.wrapper.increaseAllowance(10); + }); - it('reverts when decreasing the allowance', async function () { - await shouldFail.reverting(this.helper.doSucceedingDecreaseAllowance(10)); - }); + it('reverts when decreasing the allowance', async function () { + await shouldFail.reverting(this.wrapper.decreaseAllowance(10)); }); + }); - context('with non-zero allowance', function () { - beforeEach(async function () { - await this.helper.setAllowance(100); - }); + context('with non-zero allowance', function () { + beforeEach(async function () { + await this.wrapper.setAllowance(100); + }); - it('reverts when approving a non-zero allowance', async function () { - await shouldFail.reverting(this.helper.doSucceedingApprove(20)); - }); + 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.helper.doSucceedingApprove(0); - }); + 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.helper.doSucceedingIncreaseAllowance(10); - }); + 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.helper.doSucceedingDecreaseAllowance(50); - }); + 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.helper.doSucceedingDecreaseAllowance(200)); - }); + it('reverts when decreasing the allowance to a negative value', async function () { + await shouldFail.reverting(this.wrapper.decreaseAllowance(200)); }); }); }); -}); +}