feat: Add SafeERC20 (#469)

.solcover.js

@@ -3,6 +3,7 @@ const skipFiles = [
   'test',
   'acl/ACLSyntaxSugar.sol',
   'common/DepositableStorage.sol', // Used in tests that send ETH
+  'common/SafeERC20.sol', // solidity-coverage fails on assembly if (https://github.com/sc-forks/solidity-coverage/issues/287)
   'common/UnstructuredStorage.sol' // Used in tests that send ETH
 ]
 

contracts/common/SafeERC20.sol

@@ -0,0 +1,156 @@
+// Inspired by AdEx (https://github.com/AdExNetwork/adex-protocol-eth/blob/b9df617829661a7518ee10f4cb6c4108659dd6d5/contracts/libs/SafeERC20.sol)
+// and 0x (https://github.com/0xProject/0x-monorepo/blob/737d1dc54d72872e24abce5a1dbe1b66d35fa21a/contracts/protocol/contracts/protocol/AssetProxy/ERC20Proxy.sol#L143)
+
+pragma solidity ^0.4.24;
+
+import "../lib/token/ERC20.sol";
+
+
+library SafeERC20 {
+    // Before 0.5, solidity has a mismatch between `address.transfer()` and `token.transfer()`:
+    // https://github.com/ethereum/solidity/issues/3544
+    bytes4 private constant TRANSFER_SELECTOR = 0xa9059cbb;
+
+    string private constant ERROR_TOKEN_BALANCE_REVERTED = "SAFE_ERC_20_BALANCE_REVERTED";
+    string private constant ERROR_TOKEN_ALLOWANCE_REVERTED = "SAFE_ERC_20_ALLOWANCE_REVERTED";
+
+    function invokeAndCheckSuccess(address _addr, bytes memory _calldata)
+        private
+        returns (bool)
+    {
+        bool ret;
+        assembly {
+            let ptr := mload(0x40)    // free memory pointer
+
+            let success := call(
+                gas,                  // forward all gas
+                _addr,                // address
+                0,                    // no value
+                add(_calldata, 0x20), // calldata start
+                mload(_calldata),     // calldata length
+                ptr,                  // write output over free memory
+                0x20                  // uint256 return
+            )
+
+            if gt(success, 0) {
+                // Check number of bytes returned from last function call
+                switch returndatasize
+
+                // No bytes returned: assume success
+                case 0 {
+                    ret := 1
+                }
+
+                // 32 bytes returned: check if non-zero
+                case 0x20 {
+                    // Only return success if returned data was true
+                    // Already have output in ptr
+                    ret := eq(mload(ptr), 1)
+                }
+
+                // Not sure what was returned: don't mark as success
+                default { }
+            }
+        }
+        return ret;
+    }
+
+    function staticInvoke(address _addr, bytes memory _calldata)
+        private
+        view
+        returns (bool, uint256)
+    {
+        bool success;
+        uint256 ret;
+        assembly {
+            let ptr := mload(0x40)    // free memory pointer
+
+            success := staticcall(
+                gas,                  // forward all gas
+                _addr,                // address
+                add(_calldata, 0x20), // calldata start
+                mload(_calldata),     // calldata length
+                ptr,                  // write output over free memory
+                0x20                  // uint256 return
+            )
+
+            if gt(success, 0) {
+                ret := mload(ptr)
+            }
+        }
+        return (success, ret);
+    }
+
+    /**
+    * @dev Same as a standards-compliant ERC20.transfer() that never reverts (returns false).
+    *      Note that this makes an external call to the token.
+    */
+    function safeTransfer(ERC20 _token, address _to, uint256 _amount) internal returns (bool) {
+        bytes memory transferCallData = abi.encodeWithSelector(
+            TRANSFER_SELECTOR,
+            _to,
+            _amount
+        );
+        return invokeAndCheckSuccess(_token, transferCallData);
+    }
+
+    /**
+    * @dev Same as a standards-compliant ERC20.transferFrom() that never reverts (returns false).
+    *      Note that this makes an external call to the token.
+    */
+    function safeTransferFrom(ERC20 _token, address _from, address _to, uint256 _amount) internal returns (bool) {
+        bytes memory transferFromCallData = abi.encodeWithSelector(
+            _token.transferFrom.selector,
+            _from,
+            _to,
+            _amount
+        );
+        return invokeAndCheckSuccess(_token, transferFromCallData);
+    }
+
+    /**
+    * @dev Same as a standards-compliant ERC20.approve() that never reverts (returns false).
+    *      Note that this makes an external call to the token.
+    */
+    function safeApprove(ERC20 _token, address _spender, uint256 _amount) internal returns (bool) {
+        bytes memory approveCallData = abi.encodeWithSelector(
+            _token.approve.selector,
+            _spender,
+            _amount
+        );
+        return invokeAndCheckSuccess(_token, approveCallData);
+    }
+
+    /**
+    * @dev Static call into ERC20.balanceOf().
+    * Reverts if the call fails for some reason (should never fail).
+    */
+    function staticBalanceOf(ERC20 _token, address _owner) internal view returns (uint256) {
+        bytes memory balanceOfCallData = abi.encodeWithSelector(
+            _token.balanceOf.selector,
+            _owner
+        );
+
+        (bool success, uint256 tokenBalance) = staticInvoke(_token, balanceOfCallData);
+        require(success, ERROR_TOKEN_BALANCE_REVERTED);
+
+        return tokenBalance;
+    }
+
+    /**
+    * @dev Static call into ERC20.allowance().
+    * Reverts if the call fails for some reason (should never fail).
+    */
+    function staticAllowance(ERC20 _token, address _owner, address _spender) internal view returns (uint256) {
+        bytes memory allowanceCallData = abi.encodeWithSelector(
+            _token.allowance.selector,
+            _owner,
+            _spender
+        );
+
+        (bool success, uint256 allowance) = staticInvoke(_token, allowanceCallData);
+        require(success, ERROR_TOKEN_ALLOWANCE_REVERTED);
+
+        return allowance;
+    }
+}

contracts/common/VaultRecoverable.sol

@@ -8,11 +8,15 @@ import "../lib/token/ERC20.sol";
 import "./EtherTokenConstant.sol";
 import "./IsContract.sol";
 import "./IVaultRecoverable.sol";
+import "./SafeERC20.sol";
 
 
 contract VaultRecoverable is IVaultRecoverable, EtherTokenConstant, IsContract {
+    using SafeERC20 for ERC20;
+
     string private constant ERROR_DISALLOWED = "RECOVER_DISALLOWED";
     string private constant ERROR_VAULT_NOT_CONTRACT = "RECOVER_VAULT_NOT_CONTRACT";
+    string private constant ERROR_TOKEN_TRANSFER_FAILED = "RECOVER_TOKEN_TRANSFER_FAILED";
 
     /**
      * @notice Send funds to recovery Vault. This contract should never receive funds,
@@ -27,8 +31,9 @@ contract VaultRecoverable is IVaultRecoverable, EtherTokenConstant, IsContract {
         if (_token == ETH) {
             vault.transfer(address(this).balance);
         } else {
-            uint256 amount = ERC20(_token).balanceOf(this);
-            ERC20(_token).transfer(vault, amount);
+            ERC20 token = ERC20(_token);
+            uint256 amount = token.staticBalanceOf(this);
+            require(token.safeTransfer(vault, amount), ERROR_TOKEN_TRANSFER_FAILED);
         }
     }
 

contracts/test/mocks/SafeERC20Mock.sol

@@ -0,0 +1,36 @@
+pragma solidity 0.4.24;
+
+import "../../common/SafeERC20.sol";
+import "../../lib/token/ERC20.sol";
+
+
+contract SafeERC20Mock {
+    using SafeERC20 for ERC20;
+    event Result(bool result);
+
+    function transfer(ERC20 token, address to, uint256 amount) external returns (bool) {
+        bool result = token.safeTransfer(to, amount);
+        emit Result(result);
+        return result;
+    }
+
+    function transferFrom(ERC20 token, address from, address to, uint256 amount) external returns (bool) {
+        bool result = token.safeTransferFrom(from, to, amount);
+        emit Result(result);
+        return result;
+    }
+
+    function approve(ERC20 token, address spender, uint256 amount) external returns (bool) {
+        bool result = token.safeApprove(spender, amount);
+        emit Result(result);
+        return result;
+    }
+
+    function allowance(ERC20 token, address owner, address spender) external view returns (uint256) {
+        return token.staticAllowance(owner, spender);
+    }
+
+    function balanceOf(ERC20 token, address owner) external view returns (uint256) {
+        return token.staticBalanceOf(owner);
+    }
+}

contracts/test/mocks/TokenMock.sol

@@ -1,4 +1,4 @@
-// Stripped from https://github.com/OpenZeppelin/openzeppelin-solidity/blob/a9f910d34f0ab33a1ae5e714f69f9596a02b4d91/contracts/token/ERC20/StandardToken.sol
+// Modified from https://github.com/OpenZeppelin/openzeppelin-solidity/blob/a9f910d34f0ab33a1ae5e714f69f9596a02b4d91/contracts/token/ERC20/StandardToken.sol
 
 pragma solidity 0.4.24;
 
@@ -8,14 +8,18 @@ import "../../lib/math/SafeMath.sol";
 contract TokenMock {
     using SafeMath for uint256;
     mapping (address => uint256) private balances;
+    mapping (address => mapping (address => uint256)) private allowed;
     uint256 private totalSupply_;
+    bool private allowTransfer_;
 
+    event Approval(address indexed owner, address indexed spender, uint256 value);
     event Transfer(address indexed from, address indexed to, uint256 value);
 
     // Allow us to set the inital balance for an account on construction
     constructor(address initialAccount, uint256 initialBalance) public {
         balances[initialAccount] = initialBalance;
         totalSupply_ = initialBalance;
+        allowTransfer_ = true;
     }
 
     function totalSupply() public view returns (uint256) { return totalSupply_; }
@@ -29,12 +33,31 @@ contract TokenMock {
         return balances[_owner];
     }
 
+    /**
+    * @dev Function to check the amount of tokens that an owner allowed to a spender.
+    * @param _owner address The address which owns the funds.
+    * @param _spender address The address which will spend the funds.
+    * @return A uint256 specifying the amount of tokens still available for the spender.
+    */
+    function allowance(address _owner, address _spender) public view returns (uint256) {
+        return allowed[_owner][_spender];
+    }
+
+    /**
+    * @dev Set whether the token is transferable or not
+    * @param _allowTransfer Should token be transferable
+    */
+    function setAllowTransfer(bool _allowTransfer) public {
+        allowTransfer_ = _allowTransfer;
+    }
+
     /**
     * @dev Transfer token for a specified address
     * @param _to The address to transfer to.
     * @param _value The amount to be transferred.
     */
     function transfer(address _to, uint256 _value) public returns (bool) {
+        require(allowTransfer_);
         require(_value <= balances[msg.sender]);
         require(_to != address(0));
 
@@ -43,4 +66,41 @@ contract TokenMock {
         emit Transfer(msg.sender, _to, _value);
         return true;
     }
+
+    /**
+    * @dev Approve the passed address to spend the specified amount of tokens on behalf of msg.sender.
+    * Beware that changing an allowance with this method brings the risk that someone may use both the old
+    * and the new allowance by unfortunate transaction ordering. One possible solution to mitigate this
+    * race condition is to first reduce the spender's allowance to 0 and set the desired value afterwards:
+    * https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729
+    * @param _spender The address which will spend the funds.
+    * @param _value The amount of tokens to be spent.
+    */
+    function approve(address _spender, uint256 _value) public returns (bool) {
+        // Assume we want to protect for the race condition
+        require(allowed[msg.sender][_spender] == 0);
+
+        allowed[msg.sender][_spender] = _value;
+        emit Approval(msg.sender, _spender, _value);
+        return true;
+    }
+
+    /**
+    * @dev Transfer tokens from one address to another
+    * @param _from address The address which you want to send tokens from
+    * @param _to address The address which you want to transfer to
+    * @param _value uint256 the amount of tokens to be transferred
+    */
+    function transferFrom(address _from, address _to, uint256 _value) public returns (bool) {
+        require(allowTransfer_);
+        require(_value <= balances[_from]);
+        require(_value <= allowed[_from][msg.sender]);
+        require(_to != address(0));
+
+        balances[_from] = balances[_from].sub(_value);
+        balances[_to] = balances[_to].add(_value);
+        allowed[_from][msg.sender] = allowed[_from][msg.sender].sub(_value);
+        emit Transfer(_from, _to, _value);
+        return true;
+    }
 }