Add no-return-data ERC20 support to SafeERC20.

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))

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));
     }
 }

contracts/token/ERC20/SafeERC20.sol

@@ -5,36 +5,78 @@ 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.
  */
 library SafeERC20 {
     using SafeMath for uint256;
 
     function safeTransfer(IERC20 token, address to, uint256 value) internal {
-        require(token.transfer(to, value));
+        callAndAssertSuccess(token, abi.encodeWithSignature("transfer(address,uint256)", to, value));
     }
 
     function safeTransferFrom(IERC20 token, address from, address to, uint256 value) internal {
-        require(token.transferFrom(from, to, value));
+        callAndAssertSuccess(token, abi.encodeWithSignature("transferFrom(address,address,uint256)", from, to, value));
     }
 
     function safeApprove(IERC20 token, address spender, uint256 value) 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(address(this), spender) == 0));
-        require(token.approve(spender, value));
+        callAndAssertSuccess(token, abi.encodeWithSignature("approve(address,uint256)", 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));
+        callAndAssertSuccess(token, abi.encodeWithSignature("approve(address,uint256)", 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));
+        callAndAssertSuccess(token, abi.encodeWithSignature("approve(address,uint256)", spender, newAllowance));
+    }
+
+    /**
+     * @dev Performs a function call on a token, asserting its success, which is defined by a) the call succeeding,
+     * and b) the return data being either empty or true.
+     * @param token The token targeted byt he call.
+     * @param data The call data (encoded using abi.encode or one of its variants).
+     */
+    function callAndAssertSuccess(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,) = address(token).call(data);
+        require(success);
+
+        require(getBooleanReturnValue());
+    }
+
+    /**
+     * @dev Parses the return value of the last contract call and attempts to return a boolean value out of it.
+     */
+    function getBooleanReturnValue() private pure returns (bool result) {
+        // We need to use inline assembly here, since it is the only way we can utilize the returndatasize opcode.
+
+        // solhint-disable-next-line no-inline-assembly
+        assembly {
+            switch returndatasize()
+            case 0 { // No return value - assume success
+                result := 1
+            }
+            case 32 { // Standard ERC20 - read the returned 32 byte value
+                returndatacopy(0, 0, 32)
+                result := mload(0)
+            }
+            default { // Other return sizes are unsupported
+                revert(0, 0)
+            }
+        }
     }
 }

test/token/ERC20/SafeERC20.test.js

@@ -1,89 +1,160 @@
 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 () {
+    beforeEach(async function () {
+      this.wrapper = await SafeERC20Wrapper.new((await ERC20ReturnTrueMock.new()).address);
+    });
+
+    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.wrapper.approve(100);
+        });
+
+        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('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.wrapper.setAllowance(100);
+        });
+
+        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));
+        });
+      });
+    });
+  });
+
+  describe('with token that returns no boolean values', function () {
+    beforeEach(async function () {
+      this.wrapper = await SafeERC20Wrapper.new((await ERC20NoReturnMock.new()).address);
+    });
+
     it('doesn\'t revert on transfer', async function () {
-      await this.helper.doSucceedingTransfer();
+      await this.wrapper.transfer();
     });
 
     it('doesn\'t revert on transferFrom', async function () {
-      await this.helper.doSucceedingTransferFrom();
+      await this.wrapper.transferFrom();
     });
 
     describe('approvals', function () {
       context('with zero allowance', function () {
         beforeEach(async function () {
-          await this.helper.setAllowance(0);
+          await this.wrapper.setAllowance(0);
         });
 
         it('doesn\'t revert when approving a non-zero allowance', async function () {
-          await this.helper.doSucceedingApprove(100);
+          await this.wrapper.approve(100);
         });
 
         it('doesn\'t revert when approving a zero allowance', async function () {
-          await this.helper.doSucceedingApprove(0);
+          await this.wrapper.approve(0);
         });
 
         it('doesn\'t revert when increasing the allowance', async function () {
-          await this.helper.doSucceedingIncreaseAllowance(10);
+          await this.wrapper.increaseAllowance(10);
         });
 
         it('reverts when decreasing the allowance', async function () {
-          await shouldFail.reverting(this.helper.doSucceedingDecreaseAllowance(10));
+          await shouldFail.reverting(this.wrapper.decreaseAllowance(10));
         });
       });
 
       context('with non-zero allowance', function () {
         beforeEach(async function () {
-          await this.helper.setAllowance(100);
+          await this.wrapper.setAllowance(100);
         });
 
         it('reverts when approving a non-zero allowance', async function () {
-          await shouldFail.reverting(this.helper.doSucceedingApprove(20));
+          await shouldFail.reverting(this.wrapper.approve(20));
         });
 
         it('doesn\'t revert when approving a zero allowance', async function () {
-          await this.helper.doSucceedingApprove(0);
+          await this.wrapper.approve(0);
         });
 
         it('doesn\'t revert when increasing the allowance', async function () {
-          await this.helper.doSucceedingIncreaseAllowance(10);
+          await this.wrapper.increaseAllowance(10);
         });
 
         it('doesn\'t revert when decreasing the allowance to a positive value', async function () {
-          await this.helper.doSucceedingDecreaseAllowance(50);
+          await this.wrapper.decreaseAllowance(50);
         });
 
         it('reverts when decreasing the allowance to a negative value', async function () {
-          await shouldFail.reverting(this.helper.doSucceedingDecreaseAllowance(200));
+          await shouldFail.reverting(this.wrapper.decreaseAllowance(200));
         });
       });
     });