add SafeERC20 lib

contracts/controller/Avatar.sol

@@ -4,14 +4,14 @@ import "@daostack/infra/contracts/Reputation.sol";
 import "./DAOToken.sol";
 import "openzeppelin-solidity/contracts/ownership/Ownable.sol";
 import "openzeppelin-solidity/contracts/token/ERC20/IERC20.sol";
-import "openzeppelin-solidity/contracts/token/ERC20/SafeERC20.sol";
+import "../libs/SafeERC20.sol";
 
 
 /**
  * @title An Avatar holds tokens, reputation and ether for a controller
  */
 contract Avatar is Ownable {
-    using SafeERC20 for IERC20;
+    using SafeERC20 for address;
 
     string public orgName;
     DAOToken public nativeToken;
@@ -21,7 +21,7 @@ contract Avatar is Ownable {
     event SendEther(uint256 _amountInWei, address indexed _to);
     event ExternalTokenTransfer(address indexed _externalToken, address indexed _to, uint256 _value);
     event ExternalTokenTransferFrom(address indexed _externalToken, address _from, address _to, uint256 _value);
-    event ExternalTokenApproval(IERC20 indexed _externalToken, address _spender, uint256 _value);
+    event ExternalTokenApproval(address indexed _externalToken, address _spender, uint256 _value);
     event ReceiveEther(address indexed _sender, uint256 _value);
 
     /**
@@ -79,7 +79,7 @@ contract Avatar is Ownable {
     function externalTokenTransfer(IERC20 _externalToken, address _to, uint256 _value)
     public onlyOwner returns(bool)
     {
-        _externalToken.safeTransfer(_to, _value);
+        address(_externalToken).safeTransfer(_to, _value);
         emit ExternalTokenTransfer(address(_externalToken), _to, _value);
         return true;
     }
@@ -100,7 +100,7 @@ contract Avatar is Ownable {
     )
     public onlyOwner returns(bool)
     {
-        _externalToken.safeTransferFrom(_from, _to, _value);
+        address(_externalToken).safeTransferFrom(_from, _to, _value);
         emit ExternalTokenTransferFrom(address(_externalToken), _from, _to, _value);
         return true;
     }
@@ -116,8 +116,8 @@ contract Avatar is Ownable {
     function externalTokenApproval(IERC20 _externalToken, address _spender, uint256 _value)
     public onlyOwner returns(bool)
     {
-        require(_externalToken.approve(_spender, _value), "approve must succeed");
-        emit ExternalTokenApproval(_externalToken, _spender, _value);
+        address(_externalToken).safeApprove(_spender, _value);
+        emit ExternalTokenApproval(address(_externalToken), _spender, _value);
         return true;
     }
 

contracts/libs/SafeERC20.sol

@@ -0,0 +1,72 @@
+/*
+
+SafeERC20 by daostack.
+The code is based on a fix by SECBIT Team.
+
+USE WITH CAUTION & NO WARRANTY
+
+REFERENCE & RELATED READING
+- https://github.com/ethereum/solidity/issues/4116
+- https://medium.com/@chris_77367/explaining-unexpected-reverts-starting-with-solidity-0-4-22-3ada6e82308c
+- https://medium.com/coinmonks/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca
+- https://gist.github.com/BrendanChou/88a2eeb80947ff00bcf58ffdafeaeb61
+
+*/
+pragma solidity ^0.5.2;
+
+import "openzeppelin-solidity/contracts/utils/Address.sol";
+import "openzeppelin-solidity/contracts/token/ERC20/IERC20.sol";
+
+library SafeERC20 {
+    using Address for address;
+
+    bytes4 constant private TRANSFER_SELECTOR = bytes4(keccak256(bytes("transfer(address,uint256)")));
+    bytes4 constant private TRANSFERFROM_SELECTOR = bytes4(keccak256(bytes("transferFrom(address,address,uint256)")));
+    bytes4 constant private APPROVE_SELECTOR = bytes4(keccak256(bytes("approve(address,uint256)")));
+
+    function safeTransfer(address _erc20Addr, address _to, uint256 _value) internal {
+
+        // Must be a contract addr first!
+        require(_erc20Addr.isContract());
+
+        (bool success, bytes memory returnValue) =
+        // solhint-disable-next-line avoid-low-level-calls
+        _erc20Addr.call(abi.encodeWithSelector(TRANSFER_SELECTOR, _to, _value));
+        // call return false when something wrong
+        require(success);
+        //check return value
+        require(returnValue.length == 0 || (returnValue.length == 32 && (returnValue[31] != 0)));
+    }
+
+    function safeTransferFrom(address _erc20Addr, address _from, address _to, uint256 _value) internal {
+
+        // Must be a contract addr first!
+        require(_erc20Addr.isContract());
+
+        (bool success, bytes memory returnValue) =
+        // solhint-disable-next-line avoid-low-level-calls
+        _erc20Addr.call(abi.encodeWithSelector(TRANSFERFROM_SELECTOR, _from, _to, _value));
+        // call return false when something wrong
+        require(success);
+        //check return value
+        require(returnValue.length == 0 || (returnValue.length == 32 && (returnValue[31] != 0)));
+    }
+
+    function safeApprove(address _erc20Addr, address _spender, uint256 _value) internal {
+
+        // Must be a contract addr first!
+        require(_erc20Addr.isContract());
+
+        // safeApprove should only be called when setting an initial allowance,
+        // or when resetting it to zero.
+        require((_value == 0) || (IERC20(_erc20Addr).allowance(msg.sender, _spender) == 0));
+
+        (bool success, bytes memory returnValue) =
+        // solhint-disable-next-line avoid-low-level-calls
+        _erc20Addr.call(abi.encodeWithSelector(APPROVE_SELECTOR, _spender, _value));
+        // call return false when something wrong
+        require(success);
+        //check return value
+        require(returnValue.length == 0 || (returnValue.length == 32 && (returnValue[31] != 0)));
+    }
+}

contracts/schemes/Auction4Reputation.sol

@@ -3,6 +3,7 @@ pragma solidity ^0.5.2;
 import "openzeppelin-solidity/contracts/math/SafeMath.sol";
 import "../controller/ControllerInterface.sol";
 import "openzeppelin-solidity/contracts/ownership/Ownable.sol";
+import "../libs/SafeERC20.sol";
 
 /**
  * @title A scheme for conduct ERC20 Tokens auction for reputation
@@ -11,6 +12,7 @@ import "openzeppelin-solidity/contracts/ownership/Ownable.sol";
 
 contract Auction4Reputation is Ownable {
     using SafeMath for uint256;
+    using SafeERC20 for address;
 
     event Bid(address indexed _bidder, uint256 indexed _auctionId, uint256 _amount);
     event Redeem(uint256 indexed _auctionId, address indexed _beneficiary, uint256 _amount);
@@ -32,7 +34,7 @@ contract Auction4Reputation is Ownable {
     uint256 public auctionReputationReward;
     uint256 public auctionPeriod;
     uint256 public redeemEnableTime;
-    IERC20  public token;
+    IERC20 public token;
     address public wallet;
 
     /**
@@ -116,7 +118,7 @@ contract Auction4Reputation is Ownable {
         require(now <= auctionsEndTime, "bidding should be within the allowed bidding period");
         // solhint-disable-next-line not-rely-on-time
         require(now >= auctionsStartTime, "bidding is enable only after bidding auctionsStartTime");
-        require(token.transferFrom(msg.sender, address(this), _amount), "transferFrom should succeed");
+        address(token).safeTransferFrom(msg.sender, address(this), _amount);
         // solhint-disable-next-line not-rely-on-time
         auctionId = (now - auctionsStartTime) / auctionPeriod;
         Auction storage auction = auctions[auctionId];
@@ -143,7 +145,7 @@ contract Auction4Reputation is Ownable {
       // solhint-disable-next-line not-rely-on-time
         require(now > auctionsEndTime, "now > auctionsEndTime");
         uint256 tokenBalance = token.balanceOf(address(this));
-        require(token.transfer(wallet, tokenBalance), "transfer should succeed");
+        address(token).safeTransfer(wallet, tokenBalance);
     }
 
 }

contracts/schemes/LockingToken4Reputation.sol

@@ -3,18 +3,19 @@ pragma solidity ^0.5.2;
 import "./Locking4Reputation.sol";
 import "./PriceOracleInterface.sol";
 import "openzeppelin-solidity/contracts/ownership/Ownable.sol";
-import "openzeppelin-solidity/contracts/token/ERC20/IERC20.sol";
+import "../libs/SafeERC20.sol";
 
 
 /**
  * @title A scheme for locking ERC20 Tokens for reputation
  */
 
 contract LockingToken4Reputation is Locking4Reputation, Ownable {
+    using SafeERC20 for address;
 
     PriceOracleInterface public priceOracleContract;
     //      lockingId => token
-    mapping(bytes32   => IERC20) public lockedTokens;
+    mapping(bytes32   => address) public lockedTokens;
 
     event LockToken(bytes32 indexed _lockingId, address indexed _token, uint256 _numerator, uint256 _denominator);
 
@@ -61,7 +62,7 @@ contract LockingToken4Reputation is Locking4Reputation, Ownable {
      */
     function release(address _beneficiary, bytes32 _lockingId) public returns(bool) {
         uint256 amount = super._release(_beneficiary, _lockingId);
-        require(lockedTokens[_lockingId].transfer(_beneficiary, amount), "transfer should succeed");
+        lockedTokens[_lockingId].safeTransfer(_beneficiary, amount);
 
         return true;
     }
@@ -73,22 +74,22 @@ contract LockingToken4Reputation is Locking4Reputation, Ownable {
      * @param _token the token to lock - this should be whitelisted at the priceOracleContract
      * @return lockingId
      */
-    function lock(uint256 _amount, uint256 _period, IERC20 _token) public returns(bytes32 lockingId) {
+    function lock(uint256 _amount, uint256 _period, address _token) public returns(bytes32 lockingId) {
 
         uint256 numerator;
         uint256 denominator;
 
-        (numerator, denominator) = priceOracleContract.getPrice(address(_token));
+        (numerator, denominator) = priceOracleContract.getPrice(_token);
 
         require(numerator > 0, "numerator should be > 0");
         require(denominator > 0, "denominator should be > 0");
 
-        require(_token.transferFrom(msg.sender, address(this), _amount), "transferFrom should succeed");
+        _token.safeTransferFrom(msg.sender, address(this), _amount);
 
         lockingId = super._lock(_amount, _period, msg.sender, numerator, denominator);
 
         lockedTokens[lockingId] = _token;
 
-        emit LockToken(lockingId, address(_token), numerator, denominator);
+        emit LockToken(lockingId, _token, numerator, denominator);
     }
 }

contracts/test/BadERC20.sol

@@ -0,0 +1,22 @@
+pragma solidity ^0.5.2;
+
+//this is a mock to simulate bad ERC20 token implementation as describe at
+//https://github.com/ethereum/solidity/issues/4116
+contract BadERC20 {
+
+    mapping(address => uint256) public balances;
+    mapping (address => mapping (address => uint256)) public allowance;
+
+    function transfer(address _to, uint256 _value) public {
+        balances[_to] = _value;
+    }
+
+    function transferFrom(address, address _to, uint256 _value) public {
+        balances[_to] += _value;
+    }
+
+    function approve(address _spender, uint256 _value) public {
+        allowance[msg.sender][_spender] = _value;
+    }
+
+}

contracts/test/SafeERC20Mock.sol

@@ -0,0 +1,36 @@
+pragma solidity ^0.5.2;
+
+import "./BadERC20.sol";
+import "openzeppelin-solidity/contracts/token/ERC20/IERC20.sol";
+import "../libs/SafeERC20.sol";
+
+
+contract SafeERC20Mock {
+    using SafeERC20 for address;
+
+    address public token;
+
+    constructor(IERC20 _token) public {
+        token = address(_token);
+    }
+
+    function transfer(address _to, uint256 _value) public returns(bool) {
+        require(IERC20(token).transfer(_to, _value));
+    }
+
+    function transferWithFix(address _to, uint256 _value) public returns(bool) {
+        token.safeTransfer(_to, _value);
+        return true;
+    }
+
+    function transferFromWithFix(address _from, address _to, uint256 _value) public returns(bool) {
+        token.safeTransferFrom(_from, _to, _value);
+        return true;
+    }
+
+    function approveWithFix(address _spender, uint256 _value) public returns(bool) {
+        token.safeApprove(_spender, _value);
+        return true;
+    }
+
+}

contracts/universalSchemes/OrganizationRegister.sol

@@ -1,16 +1,17 @@
 pragma solidity ^0.5.2;
 
 import "./UniversalScheme.sol";
+import "../libs/SafeERC20.sol";
 
 /**
  * @title A universal organization registry.
  * @dev Organizations can use this scheme to open a registry.
  * Other organizations can then add and promote themselves on this registry.
  */
 
-
 contract OrganizationRegister is UniversalScheme {
     using SafeMath for uint;
+    using SafeERC20 for address;
 
     struct Parameters {
         uint256 fee;
@@ -72,7 +73,7 @@ contract OrganizationRegister is UniversalScheme {
         // Pay promotion, if the org was not listed the minimum is the fee:
         require((organizationsRegistry[address(_avatar)][_record] > 0) || (_amount >= params.fee));
 
-        require(params.token.transferFrom(msg.sender, params.beneficiary, _amount));
+        address(params.token).safeTransferFrom(msg.sender, params.beneficiary, _amount);
         if (organizationsRegistry[address(_avatar)][_record] == 0) {
             emit OrgAdded(address(_avatar), _record);
         }