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

* 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.
OpenZeppelin · Feb 28, 2019 · 41aa39a · 41aa39a
1 parent 0dded49
commit 41aa39a
Show file tree

Hide file tree

Showing 4 changed files with 136 additions and 88 deletions.
diff --git 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
@@ -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
@@ -5,36 +5,58 @@ 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));
+        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 {
         // 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));
+        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
@@ -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));
       });
     });
   });
-});
+}