Address audit comments

- Emit event when Tornado ownership changes
 - Disallow setting feeToSetter to 0 address
 - Disallow setting feeTo to 0 address
 - Use more SafeMath
 - Set MIN_PROTOCOL_FEE_DIVISOR to constant
 - Add additional unit tests

contracts/ERC20Tornado.sol

@@ -42,10 +42,10 @@ contract ERC20Tornado is Tornado {
     bool feeOn = feeTo != address(0) && protocolFeeDivisor != 0;
     if (feeOn) {
       uint256 protocolFee = SafeMath.div(denomination, protocolFeeDivisor);
-      _safeErc20Transfer(_recipient, denomination - _relayer_fee - protocolFee);
+      _safeErc20Transfer(_recipient, SafeMath.sub(denomination, SafeMath.add(_relayer_fee, protocolFee)));
       _safeErc20Transfer(feeTo, protocolFee);
     } else {
-      _safeErc20Transfer(_recipient, denomination - _relayer_fee);
+      _safeErc20Transfer(_recipient, SafeMath.sub(denomination, _relayer_fee));
     }
 
     if (_relayer_fee > 0) {

contracts/FeeManager.sol

@@ -2,7 +2,7 @@ pragma solidity 0.5.17;
 
 contract FeeManager {
   // Maximum fee of 0.5%
-  uint256 public MIN_PROTOCOL_FEE_DIVISOR = 200;
+  uint256 constant public MIN_PROTOCOL_FEE_DIVISOR = 200;
 
   address public feeTo;
   address public feeToSetter;
@@ -10,27 +10,28 @@ contract FeeManager {
 
   constructor(address _feeToSetter) public {
     feeToSetter = _feeToSetter;
-    protocolFeeDivisor = 0;
   }
 
   function setFeeTo(address _feeTo) external {
-      require(msg.sender == feeToSetter, 'Poof: FORBIDDEN');
+      require(msg.sender == feeToSetter, 'FeeManager: FORBIDDEN');
+      require(_feeTo != address(0), 'FeeManager: new feeTo is the zero address');
       feeTo = _feeTo;
   }
 
   function setFeeToSetter(address _feeToSetter) external {
-      require(msg.sender == feeToSetter, 'Poof: FORBIDDEN');
+      require(msg.sender == feeToSetter, 'FeeManager: FORBIDDEN');
+      require(_feeToSetter != address(0), 'FeeManager: new feeToSetter is the zero address');
       feeToSetter = _feeToSetter;
   }
 
   function setProtocolFeeDivisor(uint256 _protocolFeeDivisor) external {
-      require(msg.sender == feeToSetter, 'Poof: FORBIDDEN');
-      require(_protocolFeeDivisor >= MIN_PROTOCOL_FEE_DIVISOR, 'Poof: Protocol fee too high');
+      require(msg.sender == feeToSetter, 'FeeManager: FORBIDDEN');
+      require(_protocolFeeDivisor >= MIN_PROTOCOL_FEE_DIVISOR, 'FeeManager: Protocol fee too high');
       protocolFeeDivisor = _protocolFeeDivisor;
   }
 
   function clearFee() external {
-      require(msg.sender == feeToSetter, 'Poof: FORBIDDEN');
+      require(msg.sender == feeToSetter, 'FeeManager: FORBIDDEN');
       protocolFeeDivisor = 0;
   }
 }

contracts/Tornado.sol

@@ -30,6 +30,7 @@ contract Tornado is MerkleTreeWithHistory, ReentrancyGuard {
 
   event Deposit(bytes32 indexed commitment, uint32 leafIndex, uint256 timestamp);
   event Withdrawal(address to, bytes32 nullifierHash, address indexed relayer, uint256 fee);
+  event OwnershipTransferred(address indexed previousOwner, address indexed newOwner);
 
   /**
     @dev The constructor
@@ -116,6 +117,7 @@ contract Tornado is MerkleTreeWithHistory, ReentrancyGuard {
 
   /** @dev operator can change his address */
   function changeOperator(address _newOperator) external onlyOperator {
+    emit OwnershipTransferred(operator, _newOperator);
     operator = _newOperator;
   }
 }

test/FeeManager.test.js

@@ -1,9 +1,9 @@
 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}`)
+const expectErrorMessage = (error, expectedMessage) => assert(error.message.includes(expectedMessage), `Unexpected error: ${error.message}`)
 
 contract('FeeManager', accounts => {
   let feeManager
@@ -17,7 +17,7 @@ contract('FeeManager', accounts => {
       try {
         await feeManager.setFeeTo(accounts[1], {from: accounts[1]})
       } catch (e) {
-        assert(e.message.includes("Poof: FORBIDDEN"), `Unexpected error: ${e.message}`)
+        expectErrorMessage(e, "FeeManager: FORBIDDEN")
       }
 
       const feeTo = await feeManager.feeTo()
@@ -29,6 +29,17 @@ contract('FeeManager', accounts => {
       const feeTo = await feeManager.feeTo()
       expectEqual(feeTo, accounts[1])
     })
+
+    it('should fail when `feeTo` is the zero address', async () => {
+      try {
+        await feeManager.setFeeTo(ZERO_ADDRESS, {from: accounts[0]})
+      } catch (e) {
+        expectErrorMessage(e, "FeeManager: new feeTo is the zero address")
+      }
+
+      const feeTo = await feeManager.feeTo()
+      expectEqual(feeTo, accounts[1])
+    })
   })
 
   describe("#setFeeToSetter", () => {
@@ -36,7 +47,7 @@ contract('FeeManager', accounts => {
       try {
         await feeManager.setFeeToSetter(accounts[1], {from: accounts[1]})
       } catch (e) {
-        assert(e.message.includes("Poof: FORBIDDEN"), `Unexpected error: ${e.message}`)
+        expectErrorMessage(e, "FeeManager: FORBIDDEN")
       }
 
       const feeToSetter = await feeManager.feeToSetter()
@@ -52,6 +63,17 @@ contract('FeeManager', accounts => {
       const feeToSetter2 = await feeManager.feeToSetter()
       expectEqual(feeToSetter2, accounts[0])
     })
+
+    it('should fail when `feeToSetter` is the zero address', async () => {
+      try {
+        await feeManager.setFeeToSetter(ZERO_ADDRESS, {from: accounts[0]})
+      } catch (e) {
+        expectErrorMessage(e, "FeeManager: new feeToSetter is the zero address")
+      }
+
+      const feeToSetter = await feeManager.feeToSetter()
+      expectEqual(feeToSetter, accounts[0])
+    })
   })
 
   describe("#setProtocolFeeDivisor", () => {
@@ -59,7 +81,7 @@ contract('FeeManager', accounts => {
       try {
         await feeManager.setProtocolFeeDivisor(200, {from: accounts[1]})
       } catch (e) {
-        assert(e.message.includes("Poof: FORBIDDEN"), `Unexpected error: ${e.message}`)
+        expectErrorMessage(e, "FeeManager: FORBIDDEN")
       }
 
       const protocolFeeDivisor = await feeManager.protocolFeeDivisor()
@@ -70,7 +92,7 @@ contract('FeeManager', accounts => {
       try {
         await feeManager.setProtocolFeeDivisor(199, {from: accounts[0]})
       } catch (e) {
-        assert(e.message.includes("Poof: Protocol fee too high"), `Unexpected error: ${e.message}`)
+        expectErrorMessage(e, "FeeManager: Protocol fee too high")
       }
 
       const protocolFeeDivisor = await feeManager.protocolFeeDivisor()
@@ -97,7 +119,7 @@ contract('FeeManager', accounts => {
       try {
         await feeManager.clearFee({from: accounts[1]})
       } catch (e) {
-        assert(e.message.includes("Poof: FORBIDDEN"), `Unexpected error: ${e.message}`)
+        expectErrorMessage(e, "FeeManager: FORBIDDEN")
       }
 
       const protocolFeeDivisor = await feeManager.protocolFeeDivisor()