diff --git a/contracts/ERC20Mixer.sol b/contracts/ERC20Mixer.sol index 5941f0e..a7c6c58 100644 --- a/contracts/ERC20Mixer.sol +++ b/contracts/ERC20Mixer.sol @@ -17,17 +17,18 @@ contract ERC20Mixer is Mixer { address public token; constructor( - address _verifier, + IVerifier _verifier, uint256 _denomination, uint8 _merkleTreeHeight, uint256 _emptyElement, - address payable _operator, + address _operator, address _token ) Mixer(_verifier, _denomination, _merkleTreeHeight, _emptyElement, _operator) public { token = _token; } function _processDeposit() internal { + require(msg.value == 0, "ETH value is supposed to be 0 for ETH mixer"); safeErc20TransferFrom(msg.sender, address(this), denomination); } diff --git a/contracts/ETHMixer.sol b/contracts/ETHMixer.sol index 44ebc60..56a9aed 100644 --- a/contracts/ETHMixer.sol +++ b/contracts/ETHMixer.sol @@ -15,11 +15,11 @@ import "./Mixer.sol"; contract ETHMixer is Mixer { constructor( - address _verifier, + IVerifier _verifier, uint256 _denomination, uint8 _merkleTreeHeight, uint256 _emptyElement, - address payable _operator + address _operator ) Mixer(_verifier, _denomination, _merkleTreeHeight, _emptyElement, _operator) public { } diff --git a/contracts/MerkleTreeWithHistory.sol b/contracts/MerkleTreeWithHistory.sol index 9d2f189..0deb5a7 100644 --- a/contracts/MerkleTreeWithHistory.sol +++ b/contracts/MerkleTreeWithHistory.sol @@ -18,9 +18,10 @@ library Hasher { contract MerkleTreeWithHistory { uint256 public levels; - uint8 constant ROOT_HISTORY_SIZE = 100; - uint256[] private _roots; - uint256 public current_root = 0; + uint256 constant FIELD_SIZE = 21888242871839275222246405745257275088548364400416034343698204186575808495617; + uint256 constant ROOT_HISTORY_SIZE = 100; + uint256[ROOT_HISTORY_SIZE] public _roots; + uint256 public current_root_index = 0; uint256[] private _filled_subtrees; uint256[] private _zeros; @@ -28,35 +29,35 @@ contract MerkleTreeWithHistory { uint32 public next_index = 0; constructor(uint256 tree_levels, uint256 zero_value) public { + require(tree_levels > 0, "tree_levels should be greater than zero"); levels = tree_levels; + uint256 current_zero = zero_value; _zeros.push(zero_value); - _filled_subtrees.push(_zeros[0]); + _filled_subtrees.push(current_zero); for (uint8 i = 1; i < levels; i++) { - _zeros.push(hashLeftRight(_zeros[i-1], _zeros[i-1])); - _filled_subtrees.push(_zeros[i]); + current_zero = hashLeftRight(current_zero, current_zero); + _zeros.push(current_zero); + _filled_subtrees.push(current_zero); } - _roots = new uint256[](ROOT_HISTORY_SIZE); - _roots[0] = hashLeftRight(_zeros[levels - 1], _zeros[levels - 1]); + _roots[0] = hashLeftRight(current_zero, current_zero); } function hashLeftRight(uint256 left, uint256 right) public pure returns (uint256 hash) { - uint256 k = 21888242871839275222246405745257275088548364400416034343698204186575808495617; - uint256 R = 0; + uint256 R = left; // left is already checked to be less than field_size by snark verifier uint256 C = 0; - R = addmod(R, left, k); (R, C) = Hasher.MiMCSponge(R, C, 0); - R = addmod(R, right, k); + R = addmod(R, right, FIELD_SIZE); (R, C) = Hasher.MiMCSponge(R, C, 0); - hash = R; + return R; } - function _insert(uint256 leaf) internal { + function _insert(uint256 leaf) internal returns(uint256 index) { uint32 current_index = next_index; require(current_index != 2**levels, "Merkle tree is full. No more leafs can be added"); next_index += 1; @@ -80,8 +81,9 @@ contract MerkleTreeWithHistory { current_index /= 2; } - current_root = (current_root + 1) % ROOT_HISTORY_SIZE; - _roots[current_root] = current_level_hash; + current_root_index = (current_root_index + 1) % ROOT_HISTORY_SIZE; + _roots[current_root_index] = current_level_hash; + return next_index - 1; } function isKnownRoot(uint256 root) public view returns(bool) { @@ -90,14 +92,14 @@ contract MerkleTreeWithHistory { } // search most recent first uint256 i; - for(i = current_root; i < 2**256 - 1; i--) { + for(i = current_root_index; i < 2**256 - 1; i--) { if (root == _roots[i]) { return true; } } // process the rest of roots - for(i = ROOT_HISTORY_SIZE - 1; i > current_root; i--) { + for(i = ROOT_HISTORY_SIZE - 1; i > current_root_index; i--) { if (root == _roots[i]) { return true; } @@ -118,10 +120,10 @@ contract MerkleTreeWithHistory { } function getLastRoot() public view returns(uint256) { - return _roots[current_root]; + return _roots[current_root_index]; } - function roots() public view returns(uint256[] memory) { + function roots() public view returns(uint256[ROOT_HISTORY_SIZE] memory) { return _roots; } diff --git a/contracts/Mixer.sol b/contracts/Mixer.sol index 4a5b9ef..1fd9466 100644 --- a/contracts/Mixer.sol +++ b/contracts/Mixer.sol @@ -28,9 +28,9 @@ contract Mixer is MerkleTreeWithHistory { // - 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; + address public operator; + bool public isDepositsDisabled; + bool public isVerifierUpdateDisabled; modifier onlyOperator { require(msg.sender == operator, "Only operator can call this function."); _; @@ -47,13 +47,14 @@ contract Mixer is MerkleTreeWithHistory { @param _operator operator address (see operator above) */ constructor( - address _verifier, + IVerifier _verifier, uint256 _denomination, uint8 _merkleTreeHeight, uint256 _emptyElement, - address payable _operator + address _operator ) MerkleTreeWithHistory(_merkleTreeHeight, _emptyElement) public { - verifier = IVerifier(_verifier); + require(_denomination > 0, "denomination should be greater than 0"); + verifier = _verifier; operator = _operator; denomination = _denomination; } @@ -63,17 +64,17 @@ contract Mixer is MerkleTreeWithHistory { @param commitment the note commitment, which is PedersenHash(nullifier + secret) */ function deposit(uint256 commitment) public payable { - require(isDepositsEnabled, "deposits are disabled"); + require(!isDepositsDisabled, "deposits are disabled"); require(!commitments[commitment], "The commitment has been submitted"); - _processDeposit(); - _insert(commitment); + uint256 insertedIndex = _insert(commitment); commitments[commitment] = true; + _processDeposit(); - emit Deposit(commitment, next_index - 1, block.timestamp); + emit Deposit(commitment, insertedIndex, block.timestamp); } /** @dev this function is defined in a child contract */ - function _processDeposit() internal {} + function _processDeposit() internal; /** @dev Withdraw deposit from the mixer. `proof` is a zkSNARK proof data, and input is an array of circuit public inputs @@ -90,7 +91,7 @@ contract Mixer is MerkleTreeWithHistory { address payable relayer = address(input[3]); uint256 fee = input[4]; uint256 refund = input[5]; - require(fee < denomination, "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 @@ -101,19 +102,19 @@ contract Mixer is MerkleTreeWithHistory { } /** @dev this function is defined in a child contract */ - function _processWithdraw(address payable _receiver, address payable _relayer, uint256 _fee, uint256 _refund) internal {} + function _processWithdraw(address payable _receiver, address payable _relayer, uint256 _fee, uint256 _refund) internal; /** @dev whether a note is already spent */ - function isSpent(uint256 nullifier) public view returns(bool) { - return nullifierHashes[nullifier]; + function isSpent(uint256 nullifierHash) public view returns(bool) { + return nullifierHashes[nullifierHash]; } /** @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; + function toggleDeposits(bool _state) external onlyOperator { + isDepositsDisabled = _state; } /** @@ -121,7 +122,7 @@ contract Mixer is MerkleTreeWithHistory { After that operator is supposed to permanently disable this ability. */ function updateVerifier(address newVerifier) external onlyOperator { - require(isVerifierUpdateAllowed, "Verifier updates have been disabled."); + require(!isVerifierUpdateDisabled, "Verifier updates have been disabled."); verifier = IVerifier(newVerifier); } @@ -130,11 +131,11 @@ contract Mixer is MerkleTreeWithHistory { This is supposed to be called after the final trusted setup ceremony is held. */ function disableVerifierUpdate() external onlyOperator { - isVerifierUpdateAllowed = false; + isVerifierUpdateDisabled = true; } /** @dev operator can change his address */ - function changeOperator(address payable _newAccount) external onlyOperator { - operator = _newAccount; + function changeOperator(address _newOperator) external onlyOperator { + operator = _newOperator; } } diff --git a/contracts/Mocks/IUSDT.sol b/contracts/Mocks/IUSDT.sol index 8bd9ead..4425677 100644 --- a/contracts/Mocks/IUSDT.sol +++ b/contracts/Mocks/IUSDT.sol @@ -1,3 +1,5 @@ +pragma solidity ^0.5.8; + contract ERC20Basic { uint public _totalSupply; function totalSupply() public view returns (uint); diff --git a/test/ERC20Mixer.test.js b/test/ERC20Mixer.test.js index 8e0d42d..8818a79 100644 --- a/test/ERC20Mixer.test.js +++ b/test/ERC20Mixer.test.js @@ -102,6 +102,14 @@ contract('ERC20Mixer', accounts => { logs[0].args.commitment.should.be.eq.BN(toBN(commitment)) logs[0].args.leafIndex.should.be.eq.BN(toBN(0)) }) + + it('should not allow to send ether on deposit', async () => { + const commitment = 43 + await token.approve(mixer.address, tokenDenomination) + + let error = await mixer.deposit(commitment, { from: sender, value: 1e6 }).should.be.rejected + error.reason.should.be.equal('ETH value is supposed to be 0 for ETH mixer') + }) }) describe('#withdraw', () => { diff --git a/test/ETHMixer.test.js b/test/ETHMixer.test.js index 2462b23..b1c00f2 100644 --- a/test/ETHMixer.test.js +++ b/test/ETHMixer.test.js @@ -115,11 +115,15 @@ contract('ETHMixer', accounts => { it('should not deposit if disabled', async () => { let commitment = 42; - (await mixer.isDepositsEnabled()).should.be.equal(true) - const err = await mixer.toggleDeposits({ from: accounts[1] }).should.be.rejected + (await mixer.isDepositsDisabled()).should.be.equal(false) + const err = await mixer.toggleDeposits(true, { from: accounts[1] }).should.be.rejected err.reason.should.be.equal('Only operator can call this function.') - await mixer.toggleDeposits({ from: sender }); - (await mixer.isDepositsEnabled()).should.be.equal(false) + await mixer.toggleDeposits(false, { from: sender }); + (await mixer.isDepositsDisabled()).should.be.equal(false) + await mixer.toggleDeposits(true, { from: sender }); + (await mixer.isDepositsDisabled()).should.be.equal(true) + await mixer.toggleDeposits(true, { from: sender }); + (await mixer.isDepositsDisabled()).should.be.equal(true) let error = await mixer.deposit(commitment, { value, from: sender }).should.be.rejected error.reason.should.be.equal('deposits are disabled') }) @@ -484,26 +488,26 @@ contract('ETHMixer', accounts => { let operator = await mixer.operator() operator.should.be.equal(sender) - let isVerifierUpdateAllowed = await mixer.isVerifierUpdateAllowed() - isVerifierUpdateAllowed.should.be.equal(true) + let isVerifierUpdateDisabled = await mixer.isVerifierUpdateDisabled() + isVerifierUpdateDisabled.should.be.equal(false) await mixer.disableVerifierUpdate().should.be.fulfilled - const newValue = await mixer.isVerifierUpdateAllowed() - newValue.should.be.equal(false) + const newValue = await mixer.isVerifierUpdateDisabled() + newValue.should.be.equal(true) }) 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) + let isVerifierUpdateDisabled = await mixer.isVerifierUpdateDisabled() + isVerifierUpdateDisabled.should.be.equal(false) await mixer.disableVerifierUpdate().should.be.fulfilled - const newValue = await mixer.isVerifierUpdateAllowed() - newValue.should.be.equal(false) + const newValue = await mixer.isVerifierUpdateDisabled() + newValue.should.be.equal(true) const newVerifier = accounts[7] const error = await mixer.updateVerifier(newVerifier).should.be.rejected