From 4d028922e60ab5d01bb4fcbe3396b24317b7be62 Mon Sep 17 00:00:00 2001 From: Brian Li Date: Tue, 20 Apr 2021 20:50:39 -0700 Subject: [PATCH] Address audit comments - operator -> owner - Add VerifierChanged events --- contracts/ERC20Tornado.sol | 6 +- contracts/FeeManager.sol | 12 +-- contracts/SafeMath.sol | 157 ------------------------------------- contracts/Tornado.sol | 34 ++++---- package-lock.json | 2 +- test/FeeManager.test.js | 12 +-- 6 files changed, 34 insertions(+), 189 deletions(-) delete mode 100644 contracts/SafeMath.sol diff --git a/contracts/ERC20Tornado.sol b/contracts/ERC20Tornado.sol index 52c4597..452692e 100644 --- a/contracts/ERC20Tornado.sol +++ b/contracts/ERC20Tornado.sol @@ -11,8 +11,8 @@ pragma solidity 0.5.17; +import "@openzeppelin/contracts/math/SafeMath.sol"; import "./Tornado.sol"; -import "./SafeMath.sol"; contract ERC20Tornado is Tornado { address public token; @@ -22,9 +22,9 @@ contract ERC20Tornado is Tornado { IFeeManager _feeManager, uint256 _denomination, uint32 _merkleTreeHeight, - address _operator, + address _owner, address _token - ) Tornado(_verifier, _feeManager, _denomination, _merkleTreeHeight, _operator) public { + ) Tornado(_verifier, _feeManager, _denomination, _merkleTreeHeight, _owner) public { token = _token; } diff --git a/contracts/FeeManager.sol b/contracts/FeeManager.sol index 31a5ff8..5576d3b 100644 --- a/contracts/FeeManager.sol +++ b/contracts/FeeManager.sol @@ -13,25 +13,25 @@ contract FeeManager { } function setFeeTo(address _feeTo) external { - require(msg.sender == feeToSetter, 'FeeManager: FORBIDDEN'); - require(_feeTo != address(0), 'FeeManager: new feeTo is the zero address'); + require(msg.sender == feeToSetter, 'FeeManager: Sender not authorized to change feeTo'); + require(_feeTo != address(0), 'FeeManager: New feeTo is the zero address'); feeTo = _feeTo; } function setFeeToSetter(address _feeToSetter) external { - require(msg.sender == feeToSetter, 'FeeManager: FORBIDDEN'); - require(_feeToSetter != address(0), 'FeeManager: new feeToSetter is the zero address'); + require(msg.sender == feeToSetter, 'FeeManager: Sender not authorized to change feeToSetter '); + require(_feeToSetter != address(0), 'FeeManager: New feeToSetter is the zero address'); feeToSetter = _feeToSetter; } function setProtocolFeeDivisor(uint256 _protocolFeeDivisor) external { - require(msg.sender == feeToSetter, 'FeeManager: FORBIDDEN'); + require(msg.sender == feeToSetter, 'FeeManager: Sender not authorized to change protocolFeeDivisor'); require(_protocolFeeDivisor >= MIN_PROTOCOL_FEE_DIVISOR, 'FeeManager: Protocol fee too high'); protocolFeeDivisor = _protocolFeeDivisor; } function clearFee() external { - require(msg.sender == feeToSetter, 'FeeManager: FORBIDDEN'); + require(msg.sender == feeToSetter, 'FeeManager: Sender not authorized to clear protocolFeeDivisor'); protocolFeeDivisor = 0; } } diff --git a/contracts/SafeMath.sol b/contracts/SafeMath.sol deleted file mode 100644 index e46f5b8..0000000 --- a/contracts/SafeMath.sol +++ /dev/null @@ -1,157 +0,0 @@ -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; - } -} diff --git a/contracts/Tornado.sol b/contracts/Tornado.sol index 8cb5f90..3b1f5f2 100644 --- a/contracts/Tornado.sol +++ b/contracts/Tornado.sol @@ -1,7 +1,7 @@ pragma solidity 0.5.17; -import "./MerkleTreeWithHistory.sol"; import "@openzeppelin/contracts/utils/ReentrancyGuard.sol"; +import "./MerkleTreeWithHistory.sol"; contract IVerifier { function verifyProof(bytes memory _proof, uint256[6] memory _input) public returns(bool); @@ -20,17 +20,18 @@ contract Tornado is MerkleTreeWithHistory, ReentrancyGuard { IVerifier public verifier; IFeeManager public feeManager; - // operator can update snark verification key - // after the final trusted setup ceremony operator rights are supposed to be transferred to zero address - address public operator; - modifier onlyOperator { - require(msg.sender == operator, "Only operator can call this function."); + // owner can update snark verification key + // after the final trusted setup ceremony owner rights are supposed to be transferred to zero address + address public owner; + modifier onlyOwner { + require(msg.sender == owner, "Only owner can call this function."); _; } 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); + event VerifierChanged(address indexed previousVerifier, address indexed newVerifier); event EncryptedNote(address indexed sender, bytes encryptedNote); /** @@ -38,19 +39,19 @@ contract Tornado is MerkleTreeWithHistory, ReentrancyGuard { @param _verifier the address of SNARK verifier for this contract @param _denomination transfer amount for each deposit @param _merkleTreeHeight the height of deposits' Merkle Tree - @param _operator operator address (see operator comment above) + @param _owner owner address (see owner comment above) */ constructor( IVerifier _verifier, IFeeManager _feeManager, uint256 _denomination, uint32 _merkleTreeHeight, - address _operator + address _owner ) MerkleTreeWithHistory(_merkleTreeHeight) public { require(_denomination > 0, "denomination should be greater than 0"); verifier = _verifier; feeManager = _feeManager; - operator = _operator; + owner = _owner; denomination = _denomination; } @@ -110,16 +111,17 @@ contract Tornado is MerkleTreeWithHistory, ReentrancyGuard { } /** - @dev allow operator to update SNARK verification keys. This is needed to update keys after the final trusted setup ceremony is held. - After that operator rights are supposed to be transferred to zero address + @dev allow owner to update SNARK verification keys. This is needed to update keys after the final trusted setup ceremony is held. + After that owner rights are supposed to be transferred to zero address */ - function updateVerifier(address _newVerifier) external onlyOperator { + function updateVerifier(address _newVerifier) external onlyOwner { + emit VerifierChanged(address(verifier), _newVerifier); verifier = IVerifier(_newVerifier); } - /** @dev operator can change his address */ - function changeOperator(address _newOperator) external onlyOperator { - emit OwnershipTransferred(operator, _newOperator); - operator = _newOperator; + /** @dev owner can change his address */ + function changeOwner(address _newOwner) external onlyOwner { + emit OwnershipTransferred(owner, _newOwner); + owner = _newOwner; } } diff --git a/package-lock.json b/package-lock.json index f361b55..07f3cd7 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,5 +1,5 @@ { - "name": "circuits", + "name": "@poofcash/poof-core", "version": "1.0.0", "lockfileVersion": 1, "requires": true, diff --git a/test/FeeManager.test.js b/test/FeeManager.test.js index 551905b..c4654a1 100644 --- a/test/FeeManager.test.js +++ b/test/FeeManager.test.js @@ -17,7 +17,7 @@ contract('FeeManager', accounts => { try { await feeManager.setFeeTo(accounts[1], {from: accounts[1]}) } catch (e) { - expectErrorMessage(e, "FeeManager: FORBIDDEN") + expectErrorMessage(e, "FeeManager: Sender not authorized to change feeTo") } const feeTo = await feeManager.feeTo() @@ -34,7 +34,7 @@ contract('FeeManager', accounts => { try { await feeManager.setFeeTo(ZERO_ADDRESS, {from: accounts[0]}) } catch (e) { - expectErrorMessage(e, "FeeManager: new feeTo is the zero address") + expectErrorMessage(e, "FeeManager: New feeTo is the zero address") } const feeTo = await feeManager.feeTo() @@ -47,7 +47,7 @@ contract('FeeManager', accounts => { try { await feeManager.setFeeToSetter(accounts[1], {from: accounts[1]}) } catch (e) { - expectErrorMessage(e, "FeeManager: FORBIDDEN") + expectErrorMessage(e, "FeeManager: Sender not authorized to change feeToSetter ") } const feeToSetter = await feeManager.feeToSetter() @@ -68,7 +68,7 @@ contract('FeeManager', accounts => { try { await feeManager.setFeeToSetter(ZERO_ADDRESS, {from: accounts[0]}) } catch (e) { - expectErrorMessage(e, "FeeManager: new feeToSetter is the zero address") + expectErrorMessage(e, "FeeManager: New feeToSetter is the zero address") } const feeToSetter = await feeManager.feeToSetter() @@ -81,7 +81,7 @@ contract('FeeManager', accounts => { try { await feeManager.setProtocolFeeDivisor(200, {from: accounts[1]}) } catch (e) { - expectErrorMessage(e, "FeeManager: FORBIDDEN") + expectErrorMessage(e, "FeeManager: Sender not authorized to change protocolFeeDivisor") } const protocolFeeDivisor = await feeManager.protocolFeeDivisor() @@ -119,7 +119,7 @@ contract('FeeManager', accounts => { try { await feeManager.clearFee({from: accounts[1]}) } catch (e) { - expectErrorMessage(e, "FeeManager: FORBIDDEN") + expectErrorMessage(e, "FeeManager: Sender not authorized to clear protocolFeeDivisor") } const protocolFeeDivisor = await feeManager.protocolFeeDivisor()