Merge pull request #17 from peppersec/audit-1

Code style fixes for audit (batch 1)
This commit is contained in:
Roman Semenov 2019-11-06 01:09:22 +03:00 committed by GitHub
commit a0a4050211
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 75 additions and 57 deletions

View File

@ -17,17 +17,18 @@ contract ERC20Mixer is Mixer {
address public token; address public token;
constructor( constructor(
address _verifier, IVerifier _verifier,
uint256 _denomination, uint256 _denomination,
uint8 _merkleTreeHeight, uint8 _merkleTreeHeight,
uint256 _emptyElement, uint256 _emptyElement,
address payable _operator, address _operator,
address _token address _token
) Mixer(_verifier, _denomination, _merkleTreeHeight, _emptyElement, _operator) public { ) Mixer(_verifier, _denomination, _merkleTreeHeight, _emptyElement, _operator) public {
token = _token; token = _token;
} }
function _processDeposit() internal { function _processDeposit() internal {
require(msg.value == 0, "ETH value is supposed to be 0 for ETH mixer");
safeErc20TransferFrom(msg.sender, address(this), denomination); safeErc20TransferFrom(msg.sender, address(this), denomination);
} }

View File

@ -15,11 +15,11 @@ import "./Mixer.sol";
contract ETHMixer is Mixer { contract ETHMixer is Mixer {
constructor( constructor(
address _verifier, IVerifier _verifier,
uint256 _denomination, uint256 _denomination,
uint8 _merkleTreeHeight, uint8 _merkleTreeHeight,
uint256 _emptyElement, uint256 _emptyElement,
address payable _operator address _operator
) Mixer(_verifier, _denomination, _merkleTreeHeight, _emptyElement, _operator) public { ) Mixer(_verifier, _denomination, _merkleTreeHeight, _emptyElement, _operator) public {
} }

View File

@ -18,9 +18,10 @@ library Hasher {
contract MerkleTreeWithHistory { contract MerkleTreeWithHistory {
uint256 public levels; uint256 public levels;
uint8 constant ROOT_HISTORY_SIZE = 100; uint256 constant FIELD_SIZE = 21888242871839275222246405745257275088548364400416034343698204186575808495617;
uint256[] private _roots; uint256 constant ROOT_HISTORY_SIZE = 100;
uint256 public current_root = 0; uint256[ROOT_HISTORY_SIZE] public _roots;
uint256 public current_root_index = 0;
uint256[] private _filled_subtrees; uint256[] private _filled_subtrees;
uint256[] private _zeros; uint256[] private _zeros;
@ -28,35 +29,35 @@ contract MerkleTreeWithHistory {
uint32 public next_index = 0; uint32 public next_index = 0;
constructor(uint256 tree_levels, uint256 zero_value) public { constructor(uint256 tree_levels, uint256 zero_value) public {
require(tree_levels > 0, "tree_levels should be greater than zero");
levels = tree_levels; levels = tree_levels;
uint256 current_zero = zero_value;
_zeros.push(zero_value); _zeros.push(zero_value);
_filled_subtrees.push(_zeros[0]); _filled_subtrees.push(current_zero);
for (uint8 i = 1; i < levels; i++) { for (uint8 i = 1; i < levels; i++) {
_zeros.push(hashLeftRight(_zeros[i-1], _zeros[i-1])); current_zero = hashLeftRight(current_zero, current_zero);
_filled_subtrees.push(_zeros[i]); _zeros.push(current_zero);
_filled_subtrees.push(current_zero);
} }
_roots = new uint256[](ROOT_HISTORY_SIZE); _roots[0] = hashLeftRight(current_zero, current_zero);
_roots[0] = hashLeftRight(_zeros[levels - 1], _zeros[levels - 1]);
} }
function hashLeftRight(uint256 left, uint256 right) public pure returns (uint256 hash) { function hashLeftRight(uint256 left, uint256 right) public pure returns (uint256 hash) {
uint256 k = 21888242871839275222246405745257275088548364400416034343698204186575808495617; uint256 R = left; // left is already checked to be less than field_size by snark verifier
uint256 R = 0;
uint256 C = 0; uint256 C = 0;
R = addmod(R, left, k);
(R, C) = Hasher.MiMCSponge(R, C, 0); (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); (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; uint32 current_index = next_index;
require(current_index != 2**levels, "Merkle tree is full. No more leafs can be added"); require(current_index != 2**levels, "Merkle tree is full. No more leafs can be added");
next_index += 1; next_index += 1;
@ -80,8 +81,9 @@ contract MerkleTreeWithHistory {
current_index /= 2; current_index /= 2;
} }
current_root = (current_root + 1) % ROOT_HISTORY_SIZE; current_root_index = (current_root_index + 1) % ROOT_HISTORY_SIZE;
_roots[current_root] = current_level_hash; _roots[current_root_index] = current_level_hash;
return next_index - 1;
} }
function isKnownRoot(uint256 root) public view returns(bool) { function isKnownRoot(uint256 root) public view returns(bool) {
@ -90,14 +92,14 @@ contract MerkleTreeWithHistory {
} }
// search most recent first // search most recent first
uint256 i; 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]) { if (root == _roots[i]) {
return true; return true;
} }
} }
// process the rest of roots // 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]) { if (root == _roots[i]) {
return true; return true;
} }
@ -118,10 +120,10 @@ contract MerkleTreeWithHistory {
} }
function getLastRoot() public view returns(uint256) { 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; return _roots;
} }

View File

@ -28,9 +28,9 @@ contract Mixer is MerkleTreeWithHistory {
// - receive a relayer fee // - receive a relayer fee
// - disable new deposits in case of emergency // - disable new deposits in case of emergency
// - update snark verification key until this ability is permanently disabled // - update snark verification key until this ability is permanently disabled
address payable public operator; address public operator;
bool public isDepositsEnabled = true; bool public isDepositsDisabled;
bool public isVerifierUpdateAllowed = true; bool public isVerifierUpdateDisabled;
modifier onlyOperator { modifier onlyOperator {
require(msg.sender == operator, "Only operator can call this function."); 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) @param _operator operator address (see operator above)
*/ */
constructor( constructor(
address _verifier, IVerifier _verifier,
uint256 _denomination, uint256 _denomination,
uint8 _merkleTreeHeight, uint8 _merkleTreeHeight,
uint256 _emptyElement, uint256 _emptyElement,
address payable _operator address _operator
) MerkleTreeWithHistory(_merkleTreeHeight, _emptyElement) public { ) MerkleTreeWithHistory(_merkleTreeHeight, _emptyElement) public {
verifier = IVerifier(_verifier); require(_denomination > 0, "denomination should be greater than 0");
verifier = _verifier;
operator = _operator; operator = _operator;
denomination = _denomination; denomination = _denomination;
} }
@ -63,17 +64,17 @@ contract Mixer is MerkleTreeWithHistory {
@param commitment the note commitment, which is PedersenHash(nullifier + secret) @param commitment the note commitment, which is PedersenHash(nullifier + secret)
*/ */
function deposit(uint256 commitment) public payable { function deposit(uint256 commitment) public payable {
require(isDepositsEnabled, "deposits are disabled"); require(!isDepositsDisabled, "deposits are disabled");
require(!commitments[commitment], "The commitment has been submitted"); require(!commitments[commitment], "The commitment has been submitted");
_processDeposit(); uint256 insertedIndex = _insert(commitment);
_insert(commitment);
commitments[commitment] = true; 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 */ /** @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 @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]); address payable relayer = address(input[3]);
uint256 fee = input[4]; uint256 fee = input[4];
uint256 refund = input[5]; 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(!nullifierHashes[nullifierHash], "The note has been already spent");
require(isKnownRoot(root), "Cannot find your merkle root"); // Make sure to use a recent one 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 */ /** @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 */ /** @dev whether a note is already spent */
function isSpent(uint256 nullifier) public view returns(bool) { function isSpent(uint256 nullifierHash) public view returns(bool) {
return nullifierHashes[nullifier]; return nullifierHashes[nullifierHash];
} }
/** /**
@dev Allow operator to temporarily disable new deposits. This is needed to protect users funds in case a vulnerability is discovered. @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. It does not affect existing deposits.
*/ */
function toggleDeposits() external onlyOperator { function toggleDeposits(bool _state) external onlyOperator {
isDepositsEnabled = !isDepositsEnabled; isDepositsDisabled = _state;
} }
/** /**
@ -121,7 +122,7 @@ contract Mixer is MerkleTreeWithHistory {
After that operator is supposed to permanently disable this ability. After that operator is supposed to permanently disable this ability.
*/ */
function updateVerifier(address newVerifier) external onlyOperator { function updateVerifier(address newVerifier) external onlyOperator {
require(isVerifierUpdateAllowed, "Verifier updates have been disabled."); require(!isVerifierUpdateDisabled, "Verifier updates have been disabled.");
verifier = IVerifier(newVerifier); 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. This is supposed to be called after the final trusted setup ceremony is held.
*/ */
function disableVerifierUpdate() external onlyOperator { function disableVerifierUpdate() external onlyOperator {
isVerifierUpdateAllowed = false; isVerifierUpdateDisabled = true;
} }
/** @dev operator can change his address */ /** @dev operator can change his address */
function changeOperator(address payable _newAccount) external onlyOperator { function changeOperator(address _newOperator) external onlyOperator {
operator = _newAccount; operator = _newOperator;
} }
} }

View File

@ -1,3 +1,5 @@
pragma solidity ^0.5.8;
contract ERC20Basic { contract ERC20Basic {
uint public _totalSupply; uint public _totalSupply;
function totalSupply() public view returns (uint); function totalSupply() public view returns (uint);

View File

@ -102,6 +102,14 @@ contract('ERC20Mixer', accounts => {
logs[0].args.commitment.should.be.eq.BN(toBN(commitment)) logs[0].args.commitment.should.be.eq.BN(toBN(commitment))
logs[0].args.leafIndex.should.be.eq.BN(toBN(0)) 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', () => { describe('#withdraw', () => {

View File

@ -115,11 +115,15 @@ contract('ETHMixer', accounts => {
it('should not deposit if disabled', async () => { it('should not deposit if disabled', async () => {
let commitment = 42; let commitment = 42;
(await mixer.isDepositsEnabled()).should.be.equal(true) (await mixer.isDepositsDisabled()).should.be.equal(false)
const err = await mixer.toggleDeposits({ from: accounts[1] }).should.be.rejected const err = await mixer.toggleDeposits(true, { from: accounts[1] }).should.be.rejected
err.reason.should.be.equal('Only operator can call this function.') err.reason.should.be.equal('Only operator can call this function.')
await mixer.toggleDeposits({ from: sender }); await mixer.toggleDeposits(false, { from: sender });
(await mixer.isDepositsEnabled()).should.be.equal(false) (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 let error = await mixer.deposit(commitment, { value, from: sender }).should.be.rejected
error.reason.should.be.equal('deposits are disabled') error.reason.should.be.equal('deposits are disabled')
}) })
@ -484,26 +488,26 @@ contract('ETHMixer', accounts => {
let operator = await mixer.operator() let operator = await mixer.operator()
operator.should.be.equal(sender) operator.should.be.equal(sender)
let isVerifierUpdateAllowed = await mixer.isVerifierUpdateAllowed() let isVerifierUpdateDisabled = await mixer.isVerifierUpdateDisabled()
isVerifierUpdateAllowed.should.be.equal(true) isVerifierUpdateDisabled.should.be.equal(false)
await mixer.disableVerifierUpdate().should.be.fulfilled await mixer.disableVerifierUpdate().should.be.fulfilled
const newValue = await mixer.isVerifierUpdateAllowed() const newValue = await mixer.isVerifierUpdateDisabled()
newValue.should.be.equal(false) newValue.should.be.equal(true)
}) })
it('cannot update verifier after this function is called', async () => { it('cannot update verifier after this function is called', async () => {
let operator = await mixer.operator() let operator = await mixer.operator()
operator.should.be.equal(sender) operator.should.be.equal(sender)
let isVerifierUpdateAllowed = await mixer.isVerifierUpdateAllowed() let isVerifierUpdateDisabled = await mixer.isVerifierUpdateDisabled()
isVerifierUpdateAllowed.should.be.equal(true) isVerifierUpdateDisabled.should.be.equal(false)
await mixer.disableVerifierUpdate().should.be.fulfilled await mixer.disableVerifierUpdate().should.be.fulfilled
const newValue = await mixer.isVerifierUpdateAllowed() const newValue = await mixer.isVerifierUpdateDisabled()
newValue.should.be.equal(false) newValue.should.be.equal(true)
const newVerifier = accounts[7] const newVerifier = accounts[7]
const error = await mixer.updateVerifier(newVerifier).should.be.rejected const error = await mixer.updateVerifier(newVerifier).should.be.rejected