Merge pull request #23 from peppersec/audit-5

Audit 5

contracts/ERC20Mixer.sol

@@ -26,20 +26,25 @@ contract ERC20Mixer is Mixer {
     token = _token;
   }
 
-  function _processDeposit() internal nonReentrant {
+  function _processDeposit() internal {
     require(msg.value == 0, "ETH value is supposed to be 0 for ERC20 mixer");
     _safeErc20TransferFrom(msg.sender, address(this), denomination);
   }
 
-  function _processWithdraw(address payable _recipient, address payable _relayer, uint256 _fee, uint256 _refund) internal nonReentrant {
+  function _processWithdraw(address payable _recipient, address payable _relayer, uint256 _fee, uint256 _refund) internal {
     require(msg.value == _refund, "Incorrect refund amount received by the contract");
 
     _safeErc20Transfer(_recipient, denomination - _fee);
     if (_fee > 0) {
       _safeErc20Transfer(_relayer, _fee);
     }
+
     if (_refund > 0) {
-      _recipient.call.value(_refund)("");
+      (bool success, ) = _recipient.call.value(_refund)("");
+      if (!success) {
+        // let's return _refund back to the relayer
+        _relayer.transfer(_refund);
+      }
     }
   }
 

contracts/ETHMixer.sol

@@ -22,11 +22,11 @@ contract ETHMixer is Mixer {
   ) Mixer(_verifier, _denomination, _merkleTreeHeight, _operator) public {
   }
 
-  function _processDeposit() internal nonReentrant {
+  function _processDeposit() internal {
     require(msg.value == denomination, "Please send `mixDenomination` ETH along with transaction");
   }
 
-  function _processWithdraw(address payable _recipient, address payable _relayer, uint256 _fee, uint256 _refund) internal nonReentrant {
+  function _processWithdraw(address payable _recipient, address payable _relayer, uint256 _fee, uint256 _refund) internal {
     // sanity checks
     require(msg.value == 0, "Message value is supposed to be zero for ETH mixer");
     require(_refund == 0, "Refund value is supposed to be zero for ETH mixer");

contracts/Mixer.sol

@@ -59,7 +59,7 @@ contract Mixer is MerkleTreeWithHistory, ReentrancyGuard {
     @dev Deposit funds into mixer. The caller must send (for ETH) or approve (for ERC20) value equal to or `denomination` of this mixer.
     @param _commitment the note commitment, which is PedersenHash(nullifier + secret)
   */
-  function deposit(bytes32 _commitment) external payable {
+  function deposit(bytes32 _commitment) external payable nonReentrant {
     require(!commitments[_commitment], "The commitment has been submitted");
 
     uint32 insertedIndex = _insert(_commitment);
@@ -80,7 +80,7 @@ contract Mixer is MerkleTreeWithHistory, ReentrancyGuard {
       - the recipient of funds
       - optional fee that goes to the transaction sender (usually a relay)
   */
-  function withdraw(bytes calldata _proof, bytes32 _root, bytes32 _nullifierHash, address payable _recipient, address payable _relayer, uint256 _fee, uint256 _refund) external payable {
+  function withdraw(bytes calldata _proof, bytes32 _root, bytes32 _nullifierHash, address payable _recipient, address payable _relayer, uint256 _fee, uint256 _refund) external payable nonReentrant {
     require(_fee <= denomination, "Fee exceeds transfer value");
     require(!nullifierHashes[_nullifierHash], "The note has been already spent");
     require(isKnownRoot(_root), "Cannot find your merkle root"); // Make sure to use a recent one

contracts/Mocks/BadRecipient.sol

@@ -0,0 +1,7 @@
+pragma solidity ^0.5.0;
+
+contract BadRecipient {
+  function() external {
+    require(false, "this contract does not accept ETH");
+  }
+}

test/ERC20Mixer.test.js

@@ -9,6 +9,7 @@ const { toBN, toHex } = require('web3-utils')
 const { takeSnapshot, revertSnapshot } = require('../lib/ganacheHelper')
 
 const Mixer = artifacts.require('./ERC20Mixer.sol')
+const BadRecipient = artifacts.require('./BadRecipient.sol')
 const Token = artifacts.require('./ERC20Mock.sol')
 const USDTToken = artifacts.require('./IUSDT.sol')
 const { ETH_AMOUNT, TOKEN_AMOUNT, MERKLE_TREE_HEIGHT, ERC20_TOKEN } = process.env
@@ -54,6 +55,7 @@ contract('ERC20Mixer', accounts => {
   let mixer
   let token
   let usdtToken
+  let badRecipient
   const sender = accounts[0]
   const operator = accounts[0]
   const levels = MERKLE_TREE_HEIGHT || 16
@@ -63,7 +65,7 @@ contract('ERC20Mixer', accounts => {
   let tree
   const fee = bigInt(ETH_AMOUNT).shr(1) || bigInt(1e17)
   const refund = ETH_AMOUNT || '1000000000000000000' // 1 ether
-  const recipient = getRandomRecipient()
+  let recipient = getRandomRecipient()
   const relayer = accounts[1]
   let groth16
   let circuit
@@ -83,6 +85,7 @@ contract('ERC20Mixer', accounts => {
       token = await Token.deployed()
       await token.mint(sender, tokenDenomination)
     }
+    badRecipient = await BadRecipient.new()
     snapshotId = await takeSnapshot()
     groth16 = await buildGroth16()
     circuit = require('../build/circuits/withdraw.json')
@@ -201,6 +204,85 @@ contract('ERC20Mixer', accounts => {
       isSpent.should.be.equal(true)
     })
 
+    it('should return refund to the relayer is case of fail', async () => {
+      const deposit = generateDeposit()
+      const user = accounts[4]
+      recipient = bigInt(badRecipient.address)
+      await tree.insert(deposit.commitment)
+      await token.mint(user, tokenDenomination)
+
+      const balanceUserBefore = await token.balanceOf(user)
+      await token.approve(mixer.address, tokenDenomination, { from: user })
+      await mixer.deposit(toFixedHex(deposit.commitment), { from: user, gasPrice: '0' })
+
+      const balanceUserAfter = await token.balanceOf(user)
+      balanceUserAfter.should.be.eq.BN(toBN(balanceUserBefore).sub(toBN(tokenDenomination)))
+
+      const { root, path_elements, path_index } = await tree.path(0)
+      // Circuit input
+      const input = stringifyBigInts({
+        // public
+        root,
+        nullifierHash: pedersenHash(deposit.nullifier.leInt2Buff(31)),
+        relayer,
+        recipient,
+        fee,
+        refund,
+
+        // private
+        nullifier: deposit.nullifier,
+        secret: deposit.secret,
+        pathElements: path_elements,
+        pathIndices: path_index,
+      })
+
+
+      const proofData = await websnarkUtils.genWitnessAndProve(groth16, input, circuit, proving_key)
+      const { proof } = websnarkUtils.toSolidityInput(proofData)
+
+      const balanceMixerBefore = await token.balanceOf(mixer.address)
+      const balanceRelayerBefore = await token.balanceOf(relayer)
+      const balanceRecieverBefore = await token.balanceOf(toHex(recipient.toString()))
+
+      const ethBalanceOperatorBefore = await web3.eth.getBalance(operator)
+      const ethBalanceRecieverBefore = await web3.eth.getBalance(toHex(recipient.toString()))
+      const ethBalanceRelayerBefore = await web3.eth.getBalance(relayer)
+      let isSpent = await mixer.isSpent(toFixedHex(input.nullifierHash))
+      isSpent.should.be.equal(false)
+
+      const args = [
+        toFixedHex(input.root),
+        toFixedHex(input.nullifierHash),
+        toFixedHex(input.recipient, 20),
+        toFixedHex(input.relayer, 20),
+        toFixedHex(input.fee),
+        toFixedHex(input.refund)
+      ]
+      const { logs } = await mixer.withdraw(proof, ...args, { value: refund, from: relayer, gasPrice: '0' })
+
+      const balanceMixerAfter = await token.balanceOf(mixer.address)
+      const balanceRelayerAfter = await token.balanceOf(relayer)
+      const ethBalanceOperatorAfter = await web3.eth.getBalance(operator)
+      const balanceRecieverAfter = await token.balanceOf(toHex(recipient.toString()))
+      const ethBalanceRecieverAfter = await web3.eth.getBalance(toHex(recipient.toString()))
+      const ethBalanceRelayerAfter = await web3.eth.getBalance(relayer)
+      const feeBN = toBN(fee.toString())
+      balanceMixerAfter.should.be.eq.BN(toBN(balanceMixerBefore).sub(toBN(tokenDenomination)))
+      balanceRelayerAfter.should.be.eq.BN(toBN(balanceRelayerBefore).add(feeBN))
+      balanceRecieverAfter.should.be.eq.BN(toBN(balanceRecieverBefore).add(toBN(tokenDenomination).sub(feeBN)))
+
+      ethBalanceOperatorAfter.should.be.eq.BN(toBN(ethBalanceOperatorBefore))
+      ethBalanceRecieverAfter.should.be.eq.BN(toBN(ethBalanceRecieverBefore))
+      ethBalanceRelayerAfter.should.be.eq.BN(toBN(ethBalanceRelayerBefore))
+
+      logs[0].event.should.be.equal('Withdrawal')
+      logs[0].args.nullifierHash.should.be.equal(toFixedHex(input.nullifierHash))
+      logs[0].args.relayer.should.be.eq.BN(relayer)
+      logs[0].args.fee.should.be.eq.BN(feeBN)
+      isSpent = await mixer.isSpent(toFixedHex(input.nullifierHash))
+      isSpent.should.be.equal(true)
+    })
+
     it('should reject with wrong refund value', async () => {
       const deposit = generateDeposit()
       const user = accounts[4]