Update feeToSetter

- Don't modify the interface to processWithdraw
 - Add SafeMath and use it
 - Add tests for all the FeeManager methods
 - Update existing unit test for feeToSetter

README.md

@@ -1,6 +1,6 @@
 # Poof Cash 
 
-Poof Cash is a non-custodial Ethereum and ERC20 privacy solution based on zkSNARKs. It improves transaction privacy by breaking the on-chain link between the recipient and destination addresses. It uses a smart contract that accepts ERC20 deposits that can be withdrawn by a different address. Whenever ERC20 is withdrawn by the new address, there is no way to link the withdrawal to the deposit, ensuring complete privacy.
+Poof Cash is a non-custodial Celo and ERC20 privacy solution based on zkSNARKs. It improves transaction privacy by breaking the on-chain link between the recipient and destination addresses. It uses a smart contract that accepts ERC20 deposits that can be withdrawn by a different address. Whenever ERC20 is withdrawn by the new address, there is no way to link the withdrawal to the deposit, ensuring complete privacy.
 
 To make a deposit user generates a secret and sends its hash (called a commitment) along with the deposit amount to the Poof smart contract. The contract accepts the deposit and adds the commitment to its list of deposits.
 

client/contracts/Hasher.json

@@ -51,6 +51,5 @@
     }
   },
   "schemaVersion": "3.3.4",
-  "updatedAt": "2021-04-05T02:45:55.201Z",
-  "networkType": "ethereum"
+  "updatedAt": "2021-04-12T00:15:10.073Z"
 }

contracts/ERC20Tornado.sol

@@ -12,10 +12,10 @@
 pragma solidity 0.5.17;
 
 import "./Tornado.sol";
+import "./SafeMath.sol";
 
 contract ERC20Tornado is Tornado {
   address public token;
-  uint256 public protocolFee;
 
   constructor(
     IVerifier _verifier,
@@ -26,8 +26,6 @@ contract ERC20Tornado is Tornado {
     address _token
   ) Tornado(_verifier, _feeManager, _denomination, _merkleTreeHeight, _operator) public {
     token = _token;
-    // 0.5% fee
-    protocolFee = _denomination / 200;
   }
 
   function _processDeposit() internal {
@@ -35,13 +33,17 @@ contract ERC20Tornado is Tornado {
     _safeErc20TransferFrom(msg.sender, address(this), denomination);
   }
 
-  function _processWithdraw(address payable _recipient, address payable _relayer, uint256 _relayer_fee, uint256 _refund, address _feeTo) internal {
+  function _processWithdraw(address payable _recipient, address payable _relayer, uint256 _relayer_fee, uint256 _refund) internal {
     require(msg.value == _refund, "Incorrect refund amount received by the contract");
 
-    bool feeOn = _feeTo != address(0);
+    address feeTo = feeManager.feeTo();
+    uint256 protocolFeeDivisor = feeManager.protocolFeeDivisor();
+
+    bool feeOn = feeTo != address(0) && protocolFeeDivisor != 0;
     if (feeOn) {
+      uint256 protocolFee = SafeMath.div(denomination, protocolFeeDivisor);
       _safeErc20Transfer(_recipient, denomination - _relayer_fee - protocolFee);
-      _safeErc20Transfer(_feeTo, protocolFee);
+      _safeErc20Transfer(feeTo, protocolFee);
     } else {
       _safeErc20Transfer(_recipient, denomination - _relayer_fee);
     }

contracts/FeeManager.sol

@@ -1,11 +1,16 @@
 pragma solidity 0.5.17;
 
 contract FeeManager {
+  // Maximum fee of 0.5%
+  uint256 public MIN_PROTOCOL_FEE_DIVISOR = 200;
+
   address public feeTo;
   address public feeToSetter;
+  uint256 public protocolFeeDivisor;
 
   constructor(address _feeToSetter) public {
     feeToSetter = _feeToSetter;
+    protocolFeeDivisor = 0;
   }
 
   function setFeeTo(address _feeTo) external {
@@ -17,4 +22,15 @@ contract FeeManager {
       require(msg.sender == feeToSetter, 'Poof: FORBIDDEN');
       feeToSetter = _feeToSetter;
   }
+
+  function setProtocolFeeDivisor(uint256 _protocolFeeDivisor) external {
+      require(msg.sender == feeToSetter, 'Poof: FORBIDDEN');
+      require(_protocolFeeDivisor >= MIN_PROTOCOL_FEE_DIVISOR, 'Poof: Protocol fee too high');
+      protocolFeeDivisor = _protocolFeeDivisor;
+  }
+
+  function clearFee() external {
+      require(msg.sender == feeToSetter, 'Poof: FORBIDDEN');
+      protocolFeeDivisor = 0;
+  }
 }

contracts/SafeMath.sol

@@ -0,0 +1,157 @@
+pragma solidity ^0.5.0;
+
+// Source: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/release-v2.5.0/contracts/math/SafeMath.sol
+/**
+ * @dev Wrappers over Solidity's arithmetic operations with added overflow
+ * checks.
+ *
+ * Arithmetic operations in Solidity wrap on overflow. This can easily result
+ * in bugs, because programmers usually assume that an overflow raises an
+ * error, which is the standard behavior in high level programming languages.
+ * `SafeMath` restores this intuition by reverting the transaction when an
+ * operation overflows.
+ *
+ * Using this library instead of the unchecked operations eliminates an entire
+ * class of bugs, so it's recommended to use it always.
+ */
+library SafeMath {
+    /**
+     * @dev Returns the addition of two unsigned integers, reverting on
+     * overflow.
+     *
+     * Counterpart to Solidity's `+` operator.
+     *
+     * Requirements:
+     * - Addition cannot overflow.
+     */
+    function add(uint256 a, uint256 b) internal pure returns (uint256) {
+        uint256 c = a + b;
+        require(c >= a, "SafeMath: addition overflow");
+
+        return c;
+    }
+
+    /**
+     * @dev Returns the subtraction of two unsigned integers, reverting on
+     * overflow (when the result is negative).
+     *
+     * Counterpart to Solidity's `-` operator.
+     *
+     * Requirements:
+     * - Subtraction cannot overflow.
+     */
+    function sub(uint256 a, uint256 b) internal pure returns (uint256) {
+        return sub(a, b, "SafeMath: subtraction overflow");
+    }
+
+    /**
+     * @dev Returns the subtraction of two unsigned integers, reverting with custom message on
+     * overflow (when the result is negative).
+     *
+     * Counterpart to Solidity's `-` operator.
+     *
+     * Requirements:
+     * - Subtraction cannot overflow.
+     *
+     * _Available since v2.4.0._
+     */
+    function sub(uint256 a, uint256 b, string memory errorMessage) internal pure returns (uint256) {
+        require(b <= a, errorMessage);
+        uint256 c = a - b;
+
+        return c;
+    }
+
+    /**
+     * @dev Returns the multiplication of two unsigned integers, reverting on
+     * overflow.
+     *
+     * Counterpart to Solidity's `*` operator.
+     *
+     * Requirements:
+     * - Multiplication cannot overflow.
+     */
+    function mul(uint256 a, uint256 b) internal pure returns (uint256) {
+        // Gas optimization: this is cheaper than requiring 'a' not being zero, but the
+        // benefit is lost if 'b' is also tested.
+        // See: https://github.com/OpenZeppelin/openzeppelin-contracts/pull/522
+        if (a == 0) {
+            return 0;
+        }
+
+        uint256 c = a * b;
+        require(c / a == b, "SafeMath: multiplication overflow");
+
+        return c;
+    }
+
+    /**
+     * @dev Returns the integer division of two unsigned integers. Reverts on
+     * division by zero. The result is rounded towards zero.
+     *
+     * Counterpart to Solidity's `/` operator. Note: this function uses a
+     * `revert` opcode (which leaves remaining gas untouched) while Solidity
+     * uses an invalid opcode to revert (consuming all remaining gas).
+     *
+     * Requirements:
+     * - The divisor cannot be zero.
+     */
+    function div(uint256 a, uint256 b) internal pure returns (uint256) {
+        return div(a, b, "SafeMath: division by zero");
+    }
+
+    /**
+     * @dev Returns the integer division of two unsigned integers. Reverts with custom message on
+     * division by zero. The result is rounded towards zero.
+     *
+     * Counterpart to Solidity's `/` operator. Note: this function uses a
+     * `revert` opcode (which leaves remaining gas untouched) while Solidity
+     * uses an invalid opcode to revert (consuming all remaining gas).
+     *
+     * Requirements:
+     * - The divisor cannot be zero.
+     *
+     * _Available since v2.4.0._
+     */
+    function div(uint256 a, uint256 b, string memory errorMessage) internal pure returns (uint256) {
+        // Solidity only automatically asserts when dividing by 0
+        require(b > 0, errorMessage);
+        uint256 c = a / b;
+        // assert(a == b * c + a % b); // There is no case in which this doesn't hold
+
+        return c;
+    }
+
+    /**
+     * @dev Returns the remainder of dividing two unsigned integers. (unsigned integer modulo),
+     * Reverts when dividing by zero.
+     *
+     * Counterpart to Solidity's `%` operator. This function uses a `revert`
+     * opcode (which leaves remaining gas untouched) while Solidity uses an
+     * invalid opcode to revert (consuming all remaining gas).
+     *
+     * Requirements:
+     * - The divisor cannot be zero.
+     */
+    function mod(uint256 a, uint256 b) internal pure returns (uint256) {
+        return mod(a, b, "SafeMath: modulo by zero");
+    }
+
+    /**
+     * @dev Returns the remainder of dividing two unsigned integers. (unsigned integer modulo),
+     * Reverts with custom message when dividing by zero.
+     *
+     * Counterpart to Solidity's `%` operator. This function uses a `revert`
+     * opcode (which leaves remaining gas untouched) while Solidity uses an
+     * invalid opcode to revert (consuming all remaining gas).
+     *
+     * Requirements:
+     * - The divisor cannot be zero.
+     *
+     * _Available since v2.4.0._
+     */
+    function mod(uint256 a, uint256 b, string memory errorMessage) internal pure returns (uint256) {
+        require(b != 0, errorMessage);
+        return a % b;
+    }
+}

contracts/Tornado.sol

@@ -9,6 +9,7 @@ contract IVerifier {
 
 contract IFeeManager {
   function feeTo() external view returns (address);
+  function protocolFeeDivisor() external view returns (uint256);
 }
 
 contract Tornado is MerkleTreeWithHistory, ReentrancyGuard {
@@ -83,12 +84,12 @@ contract Tornado is MerkleTreeWithHistory, ReentrancyGuard {
     require(verifier.verifyProof(_proof, [uint256(_root), uint256(_nullifierHash), uint256(_recipient), uint256(_relayer), _fee, _refund]), "Invalid withdraw proof");
 
     nullifierHashes[_nullifierHash] = true;
-    _processWithdraw(_recipient, _relayer, _fee, _refund, feeManager.feeTo());
+    _processWithdraw(_recipient, _relayer, _fee, _refund);
     emit Withdrawal(_recipient, _nullifierHash, _relayer, _fee);
   }
 
   /** @dev this function is defined in a child contract */
-  function _processWithdraw(address payable _recipient, address payable _relayer, uint256 _relayer_fee, uint256 _refund, address _feeTo) internal;
+  function _processWithdraw(address payable _recipient, address payable _relayer, uint256 _relayer_fee, uint256 _refund) internal;
 
   /** @dev whether a note is already spent */
   function isSpent(bytes32 _nullifierHash) public view returns(bool) {

test/ERC20Tornado.test.js

@@ -196,6 +196,7 @@ contract('ERC20Tornado', accounts => {
 
     it("should give fees when feeTo is set", async () => {
       await feeManager.setFeeTo(accounts[5]);
+      await feeManager.setProtocolFeeDivisor(200);
       const balanceFeeToBefore = await token.balanceOf(accounts[5]);
 
       const deposit = generateDeposit();

test/FeeManager.test.js

@@ -0,0 +1,107 @@
+const FeeManager = artifacts.require('./FeeManager.sol')
+const {toBN} = require('web3-utils')
+
+const ZERO_ADDRESS = "0x0000000000000000000000000000000000000000"
+
+const expectEqual = (actual, expected) => assert(expected === actual, `Expected ${expected}, got: ${actual}`)
+
+contract('FeeManager', accounts => {
+  let feeManager
+
+  before(async () => {
+    feeManager = await FeeManager.deployed()
+  })
+
+  describe("#setFeeTo", () => {
+    it('should fail when sender is not `feeToSetter`', async () => {
+      try {
+        await feeManager.setFeeTo(accounts[1], {from: accounts[1]})
+      } catch (e) {
+        assert(e.message.includes("Poof: FORBIDDEN"), `Unexpected error: ${e.message}`)
+      }
+
+      const feeTo = await feeManager.feeTo()
+      expectEqual(feeTo, ZERO_ADDRESS)
+    })
+
+    it('should work', async () => {
+      await feeManager.setFeeTo(accounts[1], {from: accounts[0]})
+      const feeTo = await feeManager.feeTo()
+      expectEqual(feeTo, accounts[1])
+    })
+  })
+
+  describe("#setFeeToSetter", () => {
+    it('should fail when sender is not `feeToSetter`', async () => {
+      try {
+        await feeManager.setFeeToSetter(accounts[1], {from: accounts[1]})
+      } catch (e) {
+        assert(e.message.includes("Poof: FORBIDDEN"), `Unexpected error: ${e.message}`)
+      }
+
+      const feeToSetter = await feeManager.feeToSetter()
+      expectEqual(feeToSetter, accounts[0])
+    })
+
+    it('should work', async () => {
+      await feeManager.setFeeToSetter(accounts[1], {from: accounts[0]})
+      const feeToSetter1 = await feeManager.feeToSetter()
+      expectEqual(feeToSetter1, accounts[1])
+
+      await feeManager.setFeeToSetter(accounts[0], {from: accounts[1]})
+      const feeToSetter2 = await feeManager.feeToSetter()
+      expectEqual(feeToSetter2, accounts[0])
+    })
+  })
+
+  describe("#setProtocolFeeDivisor", () => {
+    it('should fail when sender is not `feeToSetter`', async () => {
+      try {
+        await feeManager.setProtocolFeeDivisor(200, {from: accounts[1]})
+      } catch (e) {
+        assert(e.message.includes("Poof: FORBIDDEN"), `Unexpected error: ${e.message}`)
+      }
+
+      const protocolFeeDivisor = await feeManager.protocolFeeDivisor()
+      expectEqual(protocolFeeDivisor.toString(), "0")
+    })
+
+    it('should fail when protocolFeeDivisor < `MIN_PROTOCOL_FEE_DIVISOR`', async () => {
+      try {
+        await feeManager.setProtocolFeeDivisor(199, {from: accounts[0]})
+      } catch (e) {
+        assert(e.message.includes("Poof: Protocol fee too high"), `Unexpected error: ${e.message}`)
+      }
+
+      const protocolFeeDivisor = await feeManager.protocolFeeDivisor()
+      expectEqual(protocolFeeDivisor.toString(), "0")
+    })
+
+    it('should work', async () => {
+      await feeManager.setProtocolFeeDivisor(200, {from: accounts[0]})
+      const protocolFeeDivisor1 = await feeManager.protocolFeeDivisor()
+      expectEqual(protocolFeeDivisor1.toString(), "200")
+
+      await feeManager.setProtocolFeeDivisor(500, {from: accounts[0]})
+      const protocolFeeDivisor2 = await feeManager.protocolFeeDivisor()
+      expectEqual(protocolFeeDivisor2.toString(), "500")
+
+      await feeManager.clearFee({from: accounts[0]})
+      const protocolFeeDivisor3 = await feeManager.protocolFeeDivisor()
+      expectEqual(protocolFeeDivisor3.toString(), "0")
+    })
+  })
+
+  describe("#clearFee", () => {
+    it('should fail when sender is not `feeToSetter`', async () => {
+      try {
+        await feeManager.clearFee({from: accounts[1]})
+      } catch (e) {
+        assert(e.message.includes("Poof: FORBIDDEN"), `Unexpected error: ${e.message}`)
+      }
+
+      const protocolFeeDivisor = await feeManager.protocolFeeDivisor()
+      expectEqual(protocolFeeDivisor.toString(), "0")
+    })
+  })
+})