fix: revert failed token transfer (#277)

RequestNetwork · Sep 1, 2020 · 7a22e4c · 7a22e4c
1 parent ffe2f73
commit 7a22e4c
Show file tree

Hide file tree

Showing 6 changed files with 113 additions and 13 deletions.
diff --git a/packages/advanced-logic/specs/payment-network-erc20-fee-proxy-contract-0.1.0.md b/packages/advanced-logic/specs/payment-network-erc20-fee-proxy-contract-0.1.0.md
@@ -43,12 +43,12 @@ The contract contains one function called `transferFromWithReferenceAndFee` whic
 
 The `TransferWithReferenceAndFee` event is emitted when the tokens are transfered. This event contains the same 6 arguments as the `transferFromWithReferenceAndFee` function.
 
-TODO: [See smart contract source]()
+[See smart contract source](https://github.com/RequestNetwork/requestNetwork/blob/master/packages/smart-contracts/src/contracts/ERC20FeeProxy.sol)
 
 | Network | Contract Address                           |
 | ------- | ------------------------------------------ |
-| Mainnet | 0x0aBdF90C2b9e605346d4064654a5d9fb67bE1189 |
-| Rinkeby | 0xf1BEB66c545aDC53dC7e43Ad0D187494e68daA1b |
+| Mainnet | 0x370DE27fdb7D1Ff1e1BaA7D11c5820a324Cf623C |
+| Rinkeby | 0xda46309973bffddd5a10ce12c44d2ee266f45a44 |
 | Private | 0x75c35C980C0d37ef46DF04d31A140b65503c0eEd |
 
 ## Properties

diff --git a/packages/smart-contracts/artifacts/ERC20FeeProxy/artifacts.json b/packages/smart-contracts/artifacts/ERC20FeeProxy/artifacts.json
@@ -8,12 +8,12 @@
         "creationBlockNumber": 0
       },
       "mainnet": {
-        "address": "0x0aBdF90C2b9e605346d4064654a5d9fb67bE1189",
-        "creationBlockNumber": 10740670
+        "address": "0x370DE27fdb7D1Ff1e1BaA7D11c5820a324Cf623C",
+        "creationBlockNumber": 10774767
       },
       "rinkeby": {
-        "address": "0xf1BEB66c545aDC53dC7e43Ad0D187494e68daA1b",
-        "creationBlockNumber": 7089964
+        "address": "0xda46309973bFfDdD5a10cE12c44d2EE266f45A44",
+        "creationBlockNumber": 7118080
       }
     }
   }

diff --git a/packages/smart-contracts/migrations/2_deploy_contracts.js b/packages/smart-contracts/migrations/2_deploy_contracts.js
@@ -1,11 +1,16 @@
 const RequestHashStorage = artifacts.require('./RequestHashStorage.sol');
 const RequestOpenHashSubmitter = artifacts.require('./RequestOpenHashSubmitter.sol');
-const erc20 = artifacts.require('./TestERC20.sol');
-const BadERC20 = artifacts.require('./BadERC20.sol');
 const ERC20Proxy = artifacts.require('./ERC20Proxy.sol');
 const EthereumProxy = artifacts.require('./EthereumProxy.sol');
 const ERC20FeeProxy = artifacts.require('./ERC20FeeProxy.sol');
 
+const erc20 = artifacts.require('./TestERC20.sol');
+const BadERC20 = artifacts.require('./BadERC20.sol');
+const ERC20True = artifacts.require('ERC20True');
+const ERC20False = artifacts.require('ERC20False');
+const ERC20NoReturn = artifacts.require('ERC20NoReturn');
+const ERC20Revert = artifacts.require('ERC20Revert');
+
 const addressContractBurner = '0xfCb4393e7fAef06fAb01c00d67c1895545AfF3b8';
 
 // Deploys, set up the contracts
@@ -51,17 +56,30 @@ module.exports = async function(deployer) {
     );
 
     // Deploy Ethereym proxy contract
-    const instanceRequestEthereumProxy = await deployer.deploy(EthereumProxy);
+    await deployer.deploy(EthereumProxy);
     console.log('EthereumProxy Contract deployed: ' + EthereumProxy.address);
 
     // Deploy ERC20 Fee proxy contract
-    const instanceRequestERC20FeeProxy = await deployer.deploy(ERC20FeeProxy);
+    await deployer.deploy(ERC20FeeProxy);
     console.log('ERC20FeeProxy Contract deployed: ' + ERC20FeeProxy.address);
 
     // Deploy the BadERC20 contract
-    const instanceBadERC20 = await deployer.deploy(BadERC20, 1000, 'BadERC20', 'BAD', 8);
+    await deployer.deploy(BadERC20, 1000, 'BadERC20', 'BAD', 8);
     console.log('BadERC20 Contract deployed: ' + BadERC20.address);
 
+    // Deploy test ERC20 contracts
+    await deployer.deploy(ERC20True);
+    console.log('ERC20True Contract deployed: ' + ERC20True.address);
+
+    await deployer.deploy(ERC20False);
+    console.log('ERC20False Contract deployed: ' + ERC20False.address);
+
+    await deployer.deploy(ERC20NoReturn);
+    console.log('ERC20NoReturn Contract deployed: ' + ERC20NoReturn.address);
+
+    await deployer.deploy(ERC20Revert);
+    console.log('ERC20Revert Contract deployed: ' + ERC20Revert.address);
+
     // ----------------------------------
     console.log('Contracts initialized');
     console.log(`
@@ -72,6 +90,10 @@ module.exports = async function(deployer) {
       EthereumProxy:            ${EthereumProxy.address}
       ERC20FeeProxy:            ${ERC20FeeProxy.address}
       BadERC20:                 ${BadERC20.address}
+      ERC20True:                ${ERC20True.address}
+      ERC20False:               ${ERC20False.address}
+      ERC20NoReturn:            ${ERC20NoReturn.address}
+      ERC20Revert:              ${ERC20Revert.address}
     `);
   } catch (e) {
     console.error(e);

diff --git a/packages/smart-contracts/src/contracts/ERC20FeeProxy.sol b/packages/smart-contracts/src/contracts/ERC20FeeProxy.sol
@@ -66,7 +66,7 @@ contract ERC20FeeProxy {
     }
 
     // solium-disable-next-line security/no-low-level-calls
-    (result,) = _tokenAddress.call(abi.encodeWithSignature(
+    (bool success, ) = _tokenAddress.call(abi.encodeWithSignature(
       "transferFrom(address,address,uint256)",
       msg.sender,
       _to,
@@ -86,6 +86,9 @@ contract ERC20FeeProxy {
             revert(0, 0)
         }
     }
+
+    require(success, "transferFrom() has been reverted");
+
     /* solium-enable security/no-inline-assembly */
     return result;
   }

diff --git a/packages/smart-contracts/src/contracts/TestERC20.sol b/packages/smart-contracts/src/contracts/TestERC20.sol
@@ -15,3 +15,29 @@ contract TestERC20 is ERC20, ERC20Detailed {
     transfer(0xf17f52151EbEF6C7334FAD080c5704D77216b732, 10);
   }
 }
+
+
+contract ERC20True {
+  function transferFrom(address _from, address _to, uint _value) public returns (bool) {
+    return true;
+  }
+}
+
+
+contract ERC20False {
+  function transferFrom(address _from, address _to, uint _value) public returns (bool) {
+    return false;
+  }
+}
+
+
+contract ERC20NoReturn {
+  function transferFrom(address _from, address _to, uint _value) public {}
+}
+
+
+contract ERC20Revert {
+  function transferFrom(address _from, address _to, uint _value) public {
+    revert("bad thing happened");
+  }
+}
diff --git a/packages/smart-contracts/test/contracts/ERC20FeeProxy.js b/packages/smart-contracts/test/contracts/ERC20FeeProxy.js
@@ -4,6 +4,10 @@ const { expectEvent, shouldFail } = require('openzeppelin-test-helpers');
 const ERC20FeeProxy = artifacts.require('./ERC20FeeProxy.sol');
 const TestERC20 = artifacts.require('./TestERC20.sol');
 const BadERC20 = artifacts.require('./BadERC20.sol');
+const ERC20True = artifacts.require('ERC20True');
+const ERC20False = artifacts.require('ERC20False');
+const ERC20NoReturn = artifacts.require('ERC20NoReturn');
+const ERC20Revert = artifacts.require('ERC20Revert');
 
 contract('ERC20FeeProxy', function(accounts) {
   const from = accounts[0];
@@ -218,4 +222,49 @@ contract('ERC20FeeProxy', function(accounts) {
     expect(toNewBalance.toNumber()).to.equals(toOldBalance.toNumber() + 100);
     expect(feeNewBalance.toNumber()).to.equals(feeOldBalance.toNumber() + 2);
   });
+
+  it('transfers tokens for payment and fees on a variety of ERC20 contract formats', async function() {
+    const passContracts = [await ERC20True.new(), await ERC20NoReturn.new()];
+
+    const failContracts = [await ERC20False.new(), await ERC20Revert.new()];
+
+    for (let contract of passContracts) {
+      let { logs } = await erc20FeeProxy.transferFromWithReferenceAndFee(
+        contract.address,
+        to,
+        '100',
+        referenceExample,
+        '2',
+        feeAddress,
+        {
+          from,
+        },
+      );
+
+      expectEvent.inLogs(logs, 'TransferWithReferenceAndFee', {
+        tokenAddress: contract.address,
+        to,
+        amount: '100',
+        paymentReference: ethers.utils.keccak256(referenceExample),
+        feeAmount: '2',
+        feeAddress,
+      });
+    }
+
+    for (let contract of failContracts) {
+      await shouldFail.reverting(
+        erc20FeeProxy.transferFromWithReferenceAndFee(
+          contract.address,
+          to,
+          '100',
+          referenceExample,
+          '2',
+          feeAddress,
+          {
+            from,
+          },
+        ),
+      );
+    }
+  });
 });