finalized implementation of optional user tx fee payment via deposits, refs omni#1

Author: d10r

contracts/upgradeable_contracts/BasicForeignBridge.sol

@@ -10,8 +10,14 @@ contract BasicForeignBridge is EternalStorage, Validatable {
     using SafeMath for uint256;
     /// triggered when relay of deposit from HomeBridge is complete
     event RelayedMessage(address recipient, uint value, bytes32 transactionHash);
+
+    /// Anybody can relay a task from the home chain. All required proof is contained in the signatures
     function executeSignatures(uint8[] vs, bytes32[] rs, bytes32[] ss, bytes message) external {
         Message.hasEnoughValidSignatures(message, vs, rs, ss, validatorContract());
+        processMessage(message);
+    }
+
+    function processMessage(bytes message) internal returns(address) {
         address recipient;
         uint256 amount;
         bytes32 txHash;
@@ -26,28 +32,33 @@ contract BasicForeignBridge is EternalStorage, Validatable {
         } else {
             onFailedMessage(recipient, amount, txHash);
         }
+        return recipient;
     }
 
     function () payable public {
         if(msg.value > 0) {
-            addDeposit();
+            addFeeDepositFor(msg.sender);
         } else {
-            withdrawDeposit();
+            withdrawFeeDeposit();
         }
     }
 
-    function addDeposit() payable public {
-        // equivalent to deposits[msg.sender] += msg.value;
-        uintStorage[keccak256(abi.encodePacked("feeDeposits", msg.sender))] += msg.value;
+    function addFeeDepositFor(address addr) payable public {
+        uintStorage[keccak256(abi.encodePacked("feeDeposits", addr))] += msg.value;
     }
 
-    function withdrawDeposit() public {
+    function withdrawFeeDeposit() public {
         uint256 withdrawAmount = uintStorage[keccak256(abi.encodePacked("feeDeposits", msg.sender))];
-        require(withdrawAmount > 0);
+        require(withdrawAmount > 0, "no fee deposits");
         delete uintStorage[keccak256(abi.encodePacked("feeDeposits", msg.sender))]; // implies setting the value to 0
         msg.sender.transfer(withdrawAmount); // throws on failure
     }
 
+    /// convenience method for checking current deposits of a given address
+    function feeDepositOf(address addr) public view returns(uint256) {
+        return uintStorage[keccak256(abi.encodePacked("feeDeposits", addr))];
+    }
+
     function onExecuteMessage(address, uint256) internal returns(bool);
 
     function setRelayedMessages(bytes32 _txHash, bool _status) internal {

contracts/upgradeable_contracts/native_to_erc20/ForeignBridgeNativeToErc.sol

@@ -12,6 +12,7 @@ contract ForeignBridgeNativeToErc is ERC677Receiver, BasicBridge, BasicForeignBr
 
     /// Event created on money withdraw.
     event UserRequestForAffirmation(address recipient, uint256 value);
+    event FeeWithdrawal(uint256 amount);
 
     function initialize(
         address _validatorContract,
@@ -46,6 +47,34 @@ contract ForeignBridgeNativeToErc is ERC677Receiver, BasicBridge, BasicForeignBr
         return isInitialized();
     }
 
+    function executeSignaturesRecipientPays(uint8[] vs, bytes32[] rs, bytes32[] ss, bytes message) external {
+        Message.hasEnoughValidSignatures(message, vs, rs, ss, validatorContract());
+        address recipient = processMessage(message);
+
+        // check if recipient has enough deposits
+        uint256 chargedFee = tx.gasprice * 200000; // 200k gas should always be enough for this tx
+        require(uintStorage[keccak256(abi.encodePacked("feeDeposits", recipient))] >= chargedFee, "not enough fee deposits");
+        // take from fee deposits
+        uintStorage[keccak256(abi.encodePacked("feeDeposits", recipient))] -= chargedFee;
+        uintStorage[keccak256(abi.encodePacked("feeDeposits", owner()))] += chargedFee;
+
+        /*
+         * In case of an Exception in processMessage(), the recipient won't be charged for the failed tx.
+         * This is on purpose. It's the relayer's responsibility to check if the tx would succeed before broadcasting it.
+         * The recipient could game the relayer by having a fee deposit withdrawal tx race against the relay tx.
+         * However there's no economic incentive to do so (relay tx would fail), thus this risk for the relayer
+         * looks acceptable.
+         */
+    }
+
+    function withdrawCollectedFees() external onlyIfOwnerOfProxy {
+        uint256 amount = uintStorage[keccak256(abi.encodePacked("feeDeposits", owner()))];
+        require(amount > 0, "nothing to claim");
+        uintStorage[keccak256(abi.encodePacked("feeDeposits", owner()))] = 0;
+        emit FeeWithdrawal(amount);
+        msg.sender.transfer(amount); // throws on failure
+    }
+
     function getBridgeMode() public pure returns(bytes4 _data) {
         return bytes4(keccak256(abi.encodePacked("native-to-erc-core")));
     }

test/native_to_erc/foreign_bridge_test.js

@@ -72,11 +72,11 @@ contract('ForeignBridge', async (accounts) => {
     })
   })
 
-  describe('#feeDeposit', async () => {
+  describe('#user pays for tx via via fee deposit', async () => {
     const user1 = accounts[7]
     const user2 = accounts[8]
 
-    beforeEach(async () => {
+    before(async () => {
       const owner = accounts[0];
       token = await POA20.new("POA ERC20 Foundation", "POA20", 18);
       const foreignBridgeImpl = await ForeignBridge.new();
@@ -87,19 +87,19 @@ contract('ForeignBridge', async (accounts) => {
       await token.transferOwnership(foreignBridge.address)
     })
 
-    it('should allow to make and withdraw deposits for fees', async () => {
+    it('should allow to make and withdraw fee deposits', async () => {
       const user1BalanceBeforeDeposit = await web3.eth.getBalance(user1)
-      let tx = await foreignBridge.sendTransaction({from: user1, value: 1}).should.be.fulfilled
+      let tx = await foreignBridge.sendTransaction({from: user1, value: web3.toWei(1, 'ether')}).should.be.fulfilled
       let user1TxFees = new web3.BigNumber(gasPrice).mul(tx.receipt.gasUsed)
-      tx = await foreignBridge.sendTransaction({from: user1, value: 3}).should.be.fulfilled
+      tx = await foreignBridge.sendTransaction({from: user1, value: web3.toWei(3, 'ether')}).should.be.fulfilled
       user1TxFees = user1TxFees.add(new web3.BigNumber(gasPrice).mul(tx.receipt.gasUsed))
       let bridgeBalance = await web3.eth.getBalance(foreignBridge.address)
-      bridgeBalance.should.be.bignumber.equal(4)
+      bridgeBalance.should.be.bignumber.equal(web3.toWei(4, 'ether'))
 
       const user2BalanceBeforeDeposit = await web3.eth.getBalance(user2)
-      await foreignBridge.sendTransaction({from: user2, value: 2}).should.be.fulfilled
+      await foreignBridge.sendTransaction({from: user2, value: web3.toWei(2, 'ether')}).should.be.fulfilled
       bridgeBalance = await web3.eth.getBalance(foreignBridge.address)
-      bridgeBalance.should.be.bignumber.equal(6)
+      bridgeBalance.should.be.bignumber.equal(web3.toWei(6, 'ether'))
 
       // user1 withdraw all
       tx = await foreignBridge.sendTransaction({from: user1, value: 0}).should.be.fulfilled
@@ -114,6 +114,56 @@ contract('ForeignBridge', async (accounts) => {
       bridgeBalance = await web3.eth.getBalance(foreignBridge.address)
       bridgeBalance.should.be.bignumber.equal(0)
     })
+
+    it('should allow to make fee deposits on behalf of somebody else', async () => {
+      const user1BalanceBeforeWithdraw = await web3.eth.getBalance(user1)
+      await foreignBridge.addFeeDepositFor(user1, {from: user2, value: web3.toWei(1, 'ether')}).should.be.fulfilled
+      let bridgeBalance = await web3.eth.getBalance(foreignBridge.address)
+      bridgeBalance.should.be.bignumber.equal(web3.toWei(1, 'ether'))
+      // user1 withdraw all
+      let tx = await foreignBridge.sendTransaction({from: user1, value: 0}).should.be.fulfilled
+      let user1TxFees = new web3.BigNumber(gasPrice).mul(tx.receipt.gasUsed)
+      const user1BalanceAfterWithdraw = await web3.eth.getBalance(user1)
+      user1BalanceAfterWithdraw.should.be.bignumber.equal(user1BalanceBeforeWithdraw.add(web3.toWei(1, 'ether')).sub(user1TxFees))
+    })
+
+    it('should relay valid tx only if enough deposited', async () => {
+      var recipientAccount = accounts[3];
+      const balanceBefore = await token.balanceOf(recipientAccount)
+      const totalSupplyBefore = await token.totalSupply()
+      var value = web3.toBigNumber(web3.toWei(0.25, "ether"));
+      var transactionHash = "0x1045bfe274b88120a6b1e5d01b5ec00ab5d01098346e90e7c7a3c9b8f0181c80";
+      var message = createMessage(recipientAccount, value, transactionHash, foreignBridge.address);
+      var signature = await sign(authorities[0], message)
+      var vrs = signatureToVRS(signature);
+      false.should.be.equal(await foreignBridge.relayedMessages(transactionHash))
+
+      await foreignBridge.executeSignaturesRecipientPays([vrs.v], [vrs.r], [vrs.s], message).should.be.rejectedWith(ERROR_MSG)
+
+      // deposit 1 ETH for fees
+      await foreignBridge.sendTransaction({from: recipientAccount, value: web3.toWei(1, 'ether')}).should.be.fulfilled
+
+      const {logs} = await foreignBridge.executeSignaturesRecipientPays([vrs.v], [vrs.r], [vrs.s], message).should.be.fulfilled
+      logs[0].event.should.be.equal("RelayedMessage")
+      logs[0].args.recipient.should.be.equal(recipientAccount)
+      logs[0].args.value.should.be.bignumber.equal(value)
+      logs[0].args.transactionHash.should.be.equal(transactionHash);
+
+      const balanceAfter = await token.balanceOf(recipientAccount);
+      const totalSupplyAfter = await token.totalSupply();
+      balanceAfter.should.be.bignumber.equal(balanceBefore.add(value))
+      totalSupplyAfter.should.be.bignumber.equal(totalSupplyBefore.add(value))
+      true.should.be.equal(await foreignBridge.relayedMessages(transactionHash))
+    })
+
+    it('should allow owner only to claim collected tx fees', async () => {
+      const ownerBalanceBefore = await web3.eth.getBalance(owner);
+      await foreignBridge.withdrawCollectedFees({from: accounts[1]}).should.be.rejected
+      await foreignBridge.withdrawCollectedFees({from: owner}).should.be.fulfilled
+      const ownerBalanceAfter = await web3.eth.getBalance(owner);
+      ownerBalanceAfter.should.be.bignumber.above(ownerBalanceBefore)
+      await foreignBridge.withdrawCollectedFees({from: owner}).should.be.rejected // nothing left to withdraw
+    })
   })
 
   describe('#executeSignatures', async () => {