From 6035255a490c7cbe993fbefe4d2cec2fd1cf3868 Mon Sep 17 00:00:00 2001 From: poma Date: Fri, 4 Oct 2019 17:27:47 +0300 Subject: [PATCH] tests, more docs, minor refactoring --- contracts/ERC20Mixer.sol | 8 ++-- contracts/ETHMixer.sol | 8 ++-- contracts/Mixer.sol | 92 +++++++++++++++++++++++----------------- test/ETHMixer.test.js | 63 +++++++++++++++++++++++++-- 4 files changed, 121 insertions(+), 50 deletions(-) diff --git a/contracts/ERC20Mixer.sol b/contracts/ERC20Mixer.sol index 8a815c5..b796c7f 100644 --- a/contracts/ERC20Mixer.sol +++ b/contracts/ERC20Mixer.sol @@ -25,21 +25,21 @@ contract ERC20Mixer is Mixer { uint256 _emptyElement, address payable _operator, address _token, - uint256 _mixDenomination - ) Mixer(_verifier, _mixDenomination, _merkleTreeHeight, _emptyElement, _operator) public { + uint256 _denomination + ) Mixer(_verifier, _denomination, _merkleTreeHeight, _emptyElement, _operator) public { token = _token; userEther = _userEther; } function _processDeposit() internal { require(msg.value == userEther, "Please send `userEther` ETH along with transaction"); - safeErc20TransferFrom(msg.sender, address(this), mixDenomination); + safeErc20TransferFrom(msg.sender, address(this), denomination); } function _processWithdraw(address payable _receiver, address payable _relayer, uint256 _fee) internal { _receiver.transfer(userEther); - safeErc20Transfer(_receiver, mixDenomination - _fee); + safeErc20Transfer(_receiver, denomination - _fee); if (_fee > 0) { safeErc20Transfer(_relayer, _fee); } diff --git a/contracts/ETHMixer.sol b/contracts/ETHMixer.sol index 107fe6c..67902eb 100644 --- a/contracts/ETHMixer.sol +++ b/contracts/ETHMixer.sol @@ -16,21 +16,21 @@ import "./Mixer.sol"; contract ETHMixer is Mixer { constructor( address _verifier, - uint256 _mixDenomination, + uint256 _denomination, uint8 _merkleTreeHeight, uint256 _emptyElement, address payable _operator - ) Mixer(_verifier, _mixDenomination, _merkleTreeHeight, _emptyElement, _operator) public { + ) Mixer(_verifier, _denomination, _merkleTreeHeight, _emptyElement, _operator) public { } function _processWithdraw(address payable _receiver, address payable _relayer, uint256 _fee) internal { - _receiver.transfer(mixDenomination - _fee); + _receiver.transfer(denomination - _fee); if (_fee > 0) { _relayer.transfer(_fee); } } function _processDeposit() internal { - require(msg.value == mixDenomination, "Please send `mixDenomination` ETH along with transaction"); + require(msg.value == denomination, "Please send `mixDenomination` ETH along with transaction"); } } diff --git a/contracts/Mixer.sol b/contracts/Mixer.sol index f4b5431..dc15cf9 100644 --- a/contracts/Mixer.sol +++ b/contracts/Mixer.sol @@ -18,16 +18,23 @@ contract IVerifier { } contract Mixer is MerkleTreeWithHistory { - bool public isDepositsEnabled = true; - bool public isVerifierUpdateAllowed = true; - // operator can disable new deposits in case of emergency - // it also receives a relayer fee - address payable public operator; + uint256 public denomination; mapping(uint256 => bool) public nullifierHashes; // we store all commitments just to prevent accidental deposits with the same commitment mapping(uint256 => bool) public commitments; IVerifier public verifier; - uint256 public mixDenomination; + + // operator can + // - receive a relayer fee + // - disable new deposits in case of emergency + // - update snark verification key until this ability is permanently disabled + address payable public operator; + bool public isDepositsEnabled = true; + bool public isVerifierUpdateAllowed = true; + modifier onlyOperator { + require(msg.sender == operator, "Only operator can call this function."); + _; + } event Deposit(uint256 indexed commitment, uint256 leafIndex, uint256 timestamp); event Withdraw(address to, uint256 nullifierHash, address indexed relayer, uint256 fee); @@ -41,26 +48,22 @@ contract Mixer is MerkleTreeWithHistory { */ constructor( address _verifier, - uint256 _mixDenomination, + uint256 _denomination, uint8 _merkleTreeHeight, uint256 _emptyElement, address payable _operator ) MerkleTreeWithHistory(_merkleTreeHeight, _emptyElement) public { verifier = IVerifier(_verifier); operator = _operator; - mixDenomination = _mixDenomination; + denomination = _denomination; } + /** - @dev Deposit funds into mixer. The caller must send value equal to `mixDenomination` of this mixer. - @param commitment the note commitment, which is PedersenHash(nullifier + secret) - */ - /** - @dev Deposit funds into the mixer. The caller must send ETH value equal to `userEther` of this mixer. - The caller also has to have at least `mixDenomination` amount approved for the mixer. + @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(uint256 commitment) public payable { - require(isDepositsEnabled, "deposits disabled"); + require(isDepositsEnabled, "deposits are disabled"); require(!commitments[commitment], "The commitment has been submitted"); _processDeposit(); _insert(commitment); @@ -68,6 +71,10 @@ contract Mixer is MerkleTreeWithHistory { emit Deposit(commitment, next_index - 1, block.timestamp); } + + /** @dev this function is defined in a child contract */ + function _processDeposit() internal {} + /** @dev Withdraw deposit from the mixer. `a`, `b`, and `c` are zkSNARK proof data, and input is an array of circuit public inputs `input` array consists of: @@ -82,7 +89,7 @@ contract Mixer is MerkleTreeWithHistory { address payable receiver = address(input[2]); address payable relayer = address(input[3]); uint256 fee = input[4]; - require(fee < mixDenomination, "Fee exceeds transfer value"); + 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 @@ -92,32 +99,41 @@ contract Mixer is MerkleTreeWithHistory { emit Withdraw(receiver, nullifierHash, relayer, fee); } - function toggleDeposits() external { - require(msg.sender == operator, "unauthorized"); - isDepositsEnabled = !isDepositsEnabled; - } - - function updateVerifier(address newVerifier) external { - require(isVerifierUpdateAllowed, "verifier updates are disabled"); - require(msg.sender == operator, "unauthorized"); - verifier = IVerifier(newVerifier); - } - - function disableVerifierUpdate() external { - require(msg.sender == operator, "unauthorized"); - isVerifierUpdateAllowed = false; - } - - function changeOperator(address payable _newAccount) external { - require(msg.sender == operator, "unauthorized"); - operator = _newAccount; - } + /** @dev this function is defined in a child contract */ + function _processWithdraw(address payable _receiver, address payable _relayer, uint256 _fee) internal {} + /** @dev whether a note is already spent */ function isSpent(uint256 nullifier) public view returns(bool) { return nullifierHashes[nullifier]; } - function _processDeposit() internal {} - function _processWithdraw(address payable _receiver, address payable _relayer, uint256 _fee) internal {} + /** + @dev Allow operator to temporarily disable new deposits. This is needed to protect users funds in case a vulnerability is discovered. + It does not affect existing deposits. + */ + function toggleDeposits() external onlyOperator { + isDepositsEnabled = !isDepositsEnabled; + } + /** + @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 is supposed to permanently disable this ability. + */ + function updateVerifier(address newVerifier) external onlyOperator { + require(isVerifierUpdateAllowed, "Verifier updates have been disabled."); + verifier = IVerifier(newVerifier); + } + + /** + @dev an option for operator to permanently disable verification keys update ability. + This is supposed to be called after the final trusted setup ceremony is held. + */ + function disableVerifierUpdate() external onlyOperator { + isVerifierUpdateAllowed = false; + } + + /** @dev operator can change his address */ + function changeOperator(address payable _newAccount) external onlyOperator { + operator = _newAccount; + } } diff --git a/test/ETHMixer.test.js b/test/ETHMixer.test.js index cd6753b..b5bc81c 100644 --- a/test/ETHMixer.test.js +++ b/test/ETHMixer.test.js @@ -90,7 +90,7 @@ contract('ETHMixer', accounts => { describe('#constructor', () => { it('should initialize', async () => { - const etherDenomination = await mixer.mixDenomination() + const etherDenomination = await mixer.denomination() etherDenomination.should.be.eq.BN(toBN(value)) }) }) @@ -116,11 +116,11 @@ contract('ETHMixer', accounts => { let commitment = 42; (await mixer.isDepositsEnabled()).should.be.equal(true) const err = await mixer.toggleDeposits({ from: accounts[1] }).should.be.rejected - err.reason.should.be.equal('unauthorized') + err.reason.should.be.equal('Only operator can call this function.') await mixer.toggleDeposits({ from: sender }); (await mixer.isDepositsEnabled()).should.be.equal(false) let error = await mixer.deposit(commitment, { value, from: sender }).should.be.rejected - error.reason.should.be.equal('deposits disabled') + error.reason.should.be.equal('deposits are disabled') }) it('should throw if there is a such commitment', async () => { @@ -416,11 +416,66 @@ contract('ETHMixer', accounts => { const newOperator = accounts[7] const error = await mixer.changeOperator(newOperator, { from: accounts[7] }).should.be.rejected - error.reason.should.be.equal('unauthorized') + error.reason.should.be.equal('Only operator can call this function.') }) }) + describe('#updateVerifier', () => { + it('should work', async () => { + let operator = await mixer.operator() + operator.should.be.equal(sender) + + const newVerifier = accounts[7] + await mixer.updateVerifier(newVerifier).should.be.fulfilled + + verifier = await mixer.verifier() + verifier.should.be.equal(newVerifier) + }) + + it('cannot change from different address', async () => { + let operator = await mixer.operator() + operator.should.be.equal(sender) + + const newVerifier = accounts[7] + const error = await mixer.updateVerifier(newVerifier, { from: accounts[7] }).should.be.rejected + error.reason.should.be.equal('Only operator can call this function.') + + }) + }) + + describe('#disableVerifierUpdate', () => { + it('should work', async () => { + let operator = await mixer.operator() + operator.should.be.equal(sender) + + let isVerifierUpdateAllowed = await mixer.isVerifierUpdateAllowed() + isVerifierUpdateAllowed.should.be.equal(true) + + await mixer.disableVerifierUpdate().should.be.fulfilled + + newValue = await mixer.isVerifierUpdateAllowed() + newValue.should.be.equal(false) + }) + + it('cannot update verifier after this function is called', async () => { + let operator = await mixer.operator() + operator.should.be.equal(sender) + + let isVerifierUpdateAllowed = await mixer.isVerifierUpdateAllowed() + isVerifierUpdateAllowed.should.be.equal(true) + + await mixer.disableVerifierUpdate().should.be.fulfilled + + newValue = await mixer.isVerifierUpdateAllowed() + newValue.should.be.equal(false) + + const newVerifier = accounts[7] + const error = await mixer.updateVerifier(newVerifier).should.be.rejected + error.reason.should.be.equal('Verifier updates have been disabled.') + }) + }) + afterEach(async () => { await revertSnapshot(snapshotId.result) // eslint-disable-next-line require-atomic-updates