Fix claimTokens to support all token transfers (#213)

* Add contract `Claimable` to contain functionality related ability to claim tokens
* The new contract is used in the bridge contracts and the erc677 token implementation
* Fix claimTokens to support all token transfers

contracts/ERC677BridgeToken.sol

@@ -4,13 +4,14 @@ import "openzeppelin-solidity/contracts/token/ERC20/BurnableToken.sol";
 import "openzeppelin-solidity/contracts/token/ERC20/MintableToken.sol";
 import "openzeppelin-solidity/contracts/token/ERC20/DetailedERC20.sol";
 import "./interfaces/IBurnableMintableERC677Token.sol";
-
+import "./upgradeable_contracts/Claimable.sol";
 
 contract ERC677BridgeToken is
     IBurnableMintableERC677Token,
     DetailedERC20,
     BurnableToken,
-    MintableToken {
+    MintableToken,
+    Claimable {
 
     address public bridgeContract;
 
@@ -91,17 +92,7 @@ contract ERC677BridgeToken is
         revert();
     }
 
-    function claimTokens(address _token, address _to) public onlyOwner {
-        require(_to != address(0));
-        if (_token == address(0)) {
-            _to.transfer(address(this).balance);
-            return;
-        }
-
-        DetailedERC20 token = DetailedERC20(_token);
-        uint256 balance = token.balanceOf(address(this));
-        require(token.transfer(_to, balance));
+    function claimTokens(address _token, address _to) public onlyOwner validAddress(_to) {
+        claimValues(_token, _to);
     }
-
-
 }

contracts/upgradeable_contracts/BasicBridge.sol

@@ -5,10 +5,10 @@ import "../upgradeability/EternalStorage.sol";
 import "../libraries/SafeMath.sol";
 import "./Validatable.sol";
 import "./Ownable.sol";
-import "openzeppelin-solidity/contracts/token/ERC20/ERC20Basic.sol";
+import "./Claimable.sol";
 
 
-contract BasicBridge is EternalStorage, Validatable, Ownable, OwnedUpgradeability {
+contract BasicBridge is EternalStorage, Validatable, Ownable, OwnedUpgradeability, Claimable {
     using SafeMath for uint256;
 
     event GasPriceChanged(uint256 gasPrice);
@@ -127,19 +127,10 @@ contract BasicBridge is EternalStorage, Validatable, Ownable, OwnedUpgradeabilit
         return executionDailyLimit() >= nextLimit && _amount <= executionMaxPerTx();
     }
 
-    function claimTokens(address _token, address _to) public onlyIfUpgradeabilityOwner {
-        require(_to != address(0));
-        if (_token == address(0)) {
-            _to.transfer(address(this).balance);
-            return;
-        }
-
-        ERC20Basic token = ERC20Basic(_token);
-        uint256 balance = token.balanceOf(this);
-        require(token.transfer(_to, balance));
+    function claimTokens(address _token, address _to) public onlyIfUpgradeabilityOwner validAddress(_to) {
+        claimValues(_token, _to);
     }
 
-
     function isContract(address _addr) internal view returns (bool)
     {
         uint length;

contracts/upgradeable_contracts/Claimable.sol

@@ -0,0 +1,52 @@
+pragma solidity 0.4.24;
+
+import "openzeppelin-solidity/contracts/token/ERC20/ERC20Basic.sol";
+import "./Sacrifice.sol";
+
+contract Claimable {
+
+    modifier validAddress(address _to) {
+        require(_to != address(0));
+        _;
+    }
+
+    function claimValues(address _token, address _to) internal {
+        if (_token == address(0)) {
+            claimNativeCoins(_to);
+        } else {
+            claimErc20Tokens(_token, _to);
+        }
+    }
+
+    function claimNativeCoins(address _to) internal {
+        uint256 value = address(this).balance;
+        if (!_to.send(value)) {
+            (new Sacrifice).value(value)(_to);
+        }
+    }
+
+    function claimErc20Tokens(address _token, address _to) internal {
+        ERC20Basic token = ERC20Basic(_token);
+        uint256 balance = token.balanceOf(this);
+        safeTransfer(_token, _to, balance);
+    }
+
+    function safeTransfer(address _token, address _to, uint256 _value) internal {
+        bytes memory returnData;
+        bool returnDataResult;
+        bytes memory callData = abi.encodeWithSignature("transfer(address,uint256)", _to, _value);
+        assembly {
+            let result := call(gas, _token, 0x0, add(callData, 0x20), mload(callData), 0, 32)
+            returnData := mload(0)
+            returnDataResult := mload(0)
+
+            switch result
+            case 0 { revert(0, 0) }
+        }
+
+        // Return data is optional
+        if (returnData.length > 0) {
+            require(returnDataResult);
+        }
+    }
+}

test/mockContracts/NoReturnTransferTokenMock.sol

@@ -0,0 +1,37 @@
+pragma solidity 0.4.24;
+
+import "openzeppelin-solidity/contracts/math/SafeMath.sol";
+
+
+contract NoReturnTransferTokenMock {
+    using SafeMath for uint256;
+
+    event Transfer(address indexed from, address indexed to, uint256 value);
+
+    mapping(address => uint256) internal balances;
+    uint256 internal totalSupply_;
+
+    function totalSupply() public view returns (uint256) {
+        return totalSupply_;
+    }
+
+    function mint(address _to, uint256 _amount) public returns (bool) {
+        totalSupply_ = totalSupply_.add(_amount);
+        balances[_to] = balances[_to].add(_amount);
+        emit Transfer(address(0), _to, _amount);
+        return true;
+    }
+
+    function balanceOf(address _owner) public view returns (uint256) {
+        return balances[_owner];
+    }
+
+    function transfer(address _to, uint256 _value) public {
+        require(_value <= balances[msg.sender]);
+        require(_to != address(0));
+
+        balances[msg.sender] = balances[msg.sender].sub(_value);
+        balances[_to] = balances[_to].add(_value);
+        emit Transfer(msg.sender, _to, _value);
+    }
+}

test/native_to_erc/foreign_bridge_test.js

@@ -5,6 +5,7 @@ const EternalStorageProxy = artifacts.require('EternalStorageProxy.sol')
 const FeeManagerNativeToErc = artifacts.require('FeeManagerNativeToErc.sol')
 const RewardableValidators = artifacts.require('RewardableValidators.sol')
 const POA20 = artifacts.require('ERC677BridgeToken.sol')
+const NoReturnTransferTokenMock = artifacts.require('NoReturnTransferTokenMock.sol')
 
 const { expect } = require('chai')
 const { ERROR_MSG, ZERO_ADDRESS, toBN } = require('../setup')
@@ -750,6 +751,39 @@ contract('ForeignBridge', async accounts => {
       expect(await tokenSecond.balanceOf(foreignBridge.address)).to.be.bignumber.equal(ZERO)
       expect(await tokenSecond.balanceOf(accounts[3])).to.be.bignumber.equal('150')
     })
+    it('works with token that not return on transfer', async () => {
+      const owner = accounts[0]
+      token = await POA20.new('POA ERC20 Foundation', 'POA20', 18)
+      const foreignBridgeImpl = await ForeignBridge.new()
+      const storageProxy = await EternalStorageProxy.new().should.be.fulfilled
+      await storageProxy.upgradeTo('1', foreignBridgeImpl.address).should.be.fulfilled
+      const foreignBridge = await ForeignBridge.at(storageProxy.address)
+      await foreignBridge.initialize(
+        validatorContract.address,
+        token.address,
+        oneEther,
+        halfEther,
+        minPerTx,
+        gasPrice,
+        requireBlockConfirmations,
+        homeDailyLimit,
+        homeMaxPerTx,
+        owner
+      )
+
+      const tokenMock = await NoReturnTransferTokenMock.new()
+
+      await tokenMock.mint(accounts[0], halfEther).should.be.fulfilled
+      expect(await tokenMock.balanceOf(accounts[0])).to.be.bignumber.equal(halfEther)
+
+      await tokenMock.transfer(foreignBridge.address, halfEther).should.be.fulfilled
+      expect(await tokenMock.balanceOf(accounts[0])).to.be.bignumber.equal(ZERO)
+      expect(await tokenMock.balanceOf(foreignBridge.address)).to.be.bignumber.equal(halfEther)
+
+      await foreignBridge.claimTokens(tokenMock.address, accounts[3], { from: owner }).should.be.fulfilled
+      expect(await tokenMock.balanceOf(foreignBridge.address)).to.be.bignumber.equal(ZERO)
+      expect(await tokenMock.balanceOf(accounts[3])).to.be.bignumber.equal(halfEther)
+    })
   })
 
   describe('#rewardableInitialize', async () => {

test/native_to_erc/home_bridge_test.js

@@ -5,6 +5,8 @@ const RevertFallback = artifacts.require('RevertFallback.sol')
 const FeeManagerNativeToErc = artifacts.require('FeeManagerNativeToErc.sol')
 const FeeManagerNativeToErcBothDirections = artifacts.require('FeeManagerNativeToErcBothDirections.sol')
 const RewardableValidators = artifacts.require('RewardableValidators.sol')
+const ERC677BridgeToken = artifacts.require('ERC677BridgeToken.sol')
+const NoReturnTransferTokenMock = artifacts.require('NoReturnTransferTokenMock.sol')
 
 const { expect } = require('chai')
 const { ERROR_MSG, ZERO_ADDRESS, toBN } = require('../setup')
@@ -816,6 +818,78 @@ contract('HomeBridge', async accounts => {
     })
   })
 
+  describe('#claimTokens', () => {
+    it('should work with token that return bool on transfer', async () => {
+      const storageProxy = await EternalStorageProxy.new()
+      const data = homeContract.contract.methods
+        .initialize(validatorContract.address, '3', '2', '1', gasPrice, requireBlockConfirmations, '3', '2', owner)
+        .encodeABI()
+      await storageProxy.upgradeToAndCall('1', homeContract.address, data).should.be.fulfilled
+      const homeBridge = await HomeBridge.at(storageProxy.address)
+
+      const token = await ERC677BridgeToken.new('Test', 'TST', 18)
+
+      await token.mint(accounts[0], halfEther).should.be.fulfilled
+      expect(await token.balanceOf(accounts[0])).to.be.bignumber.equal(halfEther)
+
+      await token.transfer(homeBridge.address, halfEther).should.be.fulfilled
+      expect(await token.balanceOf(accounts[0])).to.be.bignumber.equal(ZERO)
+      expect(await token.balanceOf(homeBridge.address)).to.be.bignumber.equal(halfEther)
+
+      await homeBridge.claimTokens(token.address, accounts[3], { from: owner }).should.be.fulfilled
+      expect(await token.balanceOf(homeBridge.address)).to.be.bignumber.equal(ZERO)
+      expect(await token.balanceOf(accounts[3])).to.be.bignumber.equal(halfEther)
+    })
+    it('should works with token that not return on transfer', async () => {
+      const storageProxy = await EternalStorageProxy.new()
+      const data = homeContract.contract.methods
+        .initialize(validatorContract.address, '3', '2', '1', gasPrice, requireBlockConfirmations, '3', '2', owner)
+        .encodeABI()
+      await storageProxy.upgradeToAndCall('1', homeContract.address, data).should.be.fulfilled
+      const homeBridge = await HomeBridge.at(storageProxy.address)
+
+      const tokenMock = await NoReturnTransferTokenMock.new()
+
+      await tokenMock.mint(accounts[0], halfEther).should.be.fulfilled
+      expect(await tokenMock.balanceOf(accounts[0])).to.be.bignumber.equal(halfEther)
+
+      await tokenMock.transfer(homeBridge.address, halfEther).should.be.fulfilled
+      expect(await tokenMock.balanceOf(accounts[0])).to.be.bignumber.equal(ZERO)
+      expect(await tokenMock.balanceOf(homeBridge.address)).to.be.bignumber.equal(halfEther)
+
+      await homeBridge.claimTokens(tokenMock.address, accounts[3], { from: owner }).should.be.fulfilled
+      expect(await tokenMock.balanceOf(homeBridge.address)).to.be.bignumber.equal(ZERO)
+      expect(await tokenMock.balanceOf(accounts[3])).to.be.bignumber.equal(halfEther)
+    })
+    it('should work for native coins', async () => {
+      const storageProxy = await EternalStorageProxy.new()
+      const data = homeContract.contract.methods
+        .initialize(
+          validatorContract.address,
+          oneEther.toString(),
+          halfEther.toString(),
+          '1',
+          gasPrice,
+          requireBlockConfirmations,
+          oneEther.toString(),
+          halfEther.toString(),
+          owner
+        )
+        .encodeABI()
+      await storageProxy.upgradeToAndCall('1', homeContract.address, data).should.be.fulfilled
+      const homeBridge = await HomeBridge.at(storageProxy.address)
+
+      const balanceBefore = toBN(await web3.eth.getBalance(accounts[3]))
+
+      await homeBridge.sendTransaction({ from: accounts[2], value: halfEther }).should.be.fulfilled
+      expect(toBN(await web3.eth.getBalance(homeBridge.address))).to.be.bignumber.equal(halfEther)
+
+      await homeBridge.claimTokens(ZERO_ADDRESS, accounts[3], { from: owner }).should.be.fulfilled
+      expect(toBN(await web3.eth.getBalance(homeBridge.address))).to.be.bignumber.equal(ZERO)
+      expect(toBN(await web3.eth.getBalance(accounts[3]))).to.be.bignumber.equal(balanceBefore.add(halfEther))
+    })
+  })
+
   describe('#rewardableInitialize', async () => {
     let homeFee
     let foreignFee

test/poa20_test.js

@@ -1,4 +1,5 @@
 const POA20 = artifacts.require('ERC677BridgeToken.sol')
+const NoReturnTransferTokenMock = artifacts.require('NoReturnTransferTokenMock.sol')
 const POA20RewardableMock = artifacts.require('./mockContracts/ERC677BridgeTokenRewardableMock')
 const ERC677ReceiverTest = artifacts.require('ERC677ReceiverTest.sol')
 const BlockRewardTest = artifacts.require('BlockReward.sol')
@@ -510,6 +511,22 @@ async function testERC677BridgeToken(accounts, rewardable) {
       expect(await tokenSecond.balanceOf(token.address)).to.be.bignumber.equal(ZERO)
       halfEther.should.be.bignumber.equal(await tokenSecond.balanceOf(accounts[3]))
     })
+    it('works with token that not return on transfer', async () => {
+      const owner = accounts[0]
+      const halfEther = ether('0.5')
+      const tokenMock = await NoReturnTransferTokenMock.new()
+
+      await tokenMock.mint(accounts[0], halfEther).should.be.fulfilled
+      expect(await tokenMock.balanceOf(accounts[0])).to.be.bignumber.equal(halfEther)
+
+      await tokenMock.transfer(token.address, halfEther).should.be.fulfilled
+      expect(await tokenMock.balanceOf(accounts[0])).to.be.bignumber.equal(ZERO)
+      expect(await tokenMock.balanceOf(token.address)).to.be.bignumber.equal(halfEther)
+
+      await token.claimTokens(tokenMock.address, accounts[3], { from: owner }).should.be.fulfilled
+      expect(await tokenMock.balanceOf(token.address)).to.be.bignumber.equal(ZERO)
+      expect(await tokenMock.balanceOf(accounts[3])).to.be.bignumber.equal(halfEther)
+    })
   })
   describe('#transfer', async () => {
     it('if transfer called on contract, onTokenTransfer is also invoked', async () => {