From 63f984b2d580bda451a18a7ddc4306e3d8d3d637 Mon Sep 17 00:00:00 2001 From: Alexey Date: Thu, 25 Jul 2019 15:16:09 +0300 Subject: [PATCH 1/7] isSpent fix --- cli.js | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/cli.js b/cli.js index 8e4d915..3cd5de2 100755 --- a/cli.js +++ b/cli.js @@ -48,24 +48,32 @@ async function withdraw(note, receiver) { console.log('Getting current state from mixer contract') const events = await mixer.getPastEvents('Deposit', { fromBlock: mixer.deployedBlock, toBlock: 'latest' }) + let leafIndex + + const commitment = deposit.commitment.toString(16).padStart('66', '0x000000') const leaves = events .sort((a, b) => a.returnValues.leafIndex.sub(b.returnValues.leafIndex)) - .map(e => e.returnValues.commitment) + .map(e => { + if (e.returnValues.commitment.eq(commitment)) { + leafIndex = e.returnValues.leafIndex.toNumber() + } + return e.returnValues.commitment + }) const tree = new merkleTree(MERKLE_TREE_HEIGHT, EMPTY_ELEMENT, leaves) const validRoot = await mixer.methods.isKnownRoot(await tree.root()).call() - // todo make sure that function input is 32 bytes long - const isSpent = await mixer.methods.isSpent('0x' + deposit.nullifier.toString(16)).call() + const nullifierHash = pedersenHash(deposit.nullifier.leInt2Buff(32)) + const nullifierHashToCheck = nullifierHash.toString(16).padStart('66', '0x000000') + const isSpent = await mixer.methods.isSpent(nullifierHashToCheck).call() assert(validRoot === true) assert(isSpent === false) - const leafIndex = leaves.map(el => el.toString()).indexOf(deposit.commitment.toString()) assert(leafIndex >= 0) const { root, path_elements, path_index } = await tree.path(leafIndex) // Circuit input const input = { // public root: root, - nullifierHash: pedersenHash(deposit.nullifier.leInt2Buff(32)), + nullifierHash, receiver: bigInt(receiver), fee: bigInt(0), From fc5eb51c3b21b7e6235b982f987cc4a30713309b Mon Sep 17 00:00:00 2001 From: Alexey Date: Thu, 25 Jul 2019 16:58:21 +0300 Subject: [PATCH 2/7] nullifier hash refactoring --- contracts/Mixer.sol | 16 ++++++++-------- test/Mixer.test.js | 7 ++++++- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/contracts/Mixer.sol b/contracts/Mixer.sol index a295377..4cf7ba4 100644 --- a/contracts/Mixer.sol +++ b/contracts/Mixer.sol @@ -8,13 +8,13 @@ contract IVerifier { contract Mixer is MerkleTreeWithHistory { uint256 public transferValue; - mapping(uint256 => bool) public nullifiers; + mapping(uint256 => bool) public nullifierHashes; // we store all commitments just to prevent accidental deposits with the same commitment mapping(uint256 => bool) public commitments; IVerifier verifier; event Deposit(uint256 indexed commitment, uint256 leafIndex, uint256 timestamp); - event Withdraw(address to, uint256 nullifier, uint256 fee); + event Withdraw(address to, uint256 nullifierHash, uint256 fee); /** @dev The constructor @@ -47,30 +47,30 @@ contract Mixer is MerkleTreeWithHistory { @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: - merkle root of all deposits in the mixer - - unique deposit nullifier to prevent double spends + - hash of unique deposit nullifier to prevent double spends - the receiver of funds - optional fee that goes to the transaction sender (usually a relay) */ function withdraw(uint256[2] memory a, uint256[2][2] memory b, uint256[2] memory c, uint256[4] memory input) public { uint256 root = input[0]; - uint256 nullifier = input[1]; + uint256 nullifierHash = input[1]; address payable receiver = address(input[2]); uint256 fee = input[3]; require(fee < transferValue, "Fee exceeds transfer value"); - require(!nullifiers[nullifier], "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(verifier.verifyProof(a, b, c, input), "Invalid withdraw proof"); - nullifiers[nullifier] = true; + nullifierHashes[nullifierHash] = true; receiver.transfer(transferValue - fee); if (fee > 0) { msg.sender.transfer(fee); } - emit Withdraw(receiver, nullifier, fee); + emit Withdraw(receiver, nullifierHash, fee); } function isSpent(uint256 nullifier) public view returns(bool) { - return nullifiers[nullifier]; + return nullifierHashes[nullifier]; } } diff --git a/test/Mixer.test.js b/test/Mixer.test.js index 61585e0..1ae0cf4 100644 --- a/test/Mixer.test.js +++ b/test/Mixer.test.js @@ -198,6 +198,8 @@ contract('Mixer', accounts => { const balanceMixerBefore = await web3.eth.getBalance(mixer.address) const balanceRelayerBefore = await web3.eth.getBalance(relayer) const balanceRecieverBefore = await web3.eth.getBalance(toHex(receiver.toString())) + let isSpent = await mixer.isSpent(input.nullifierHash.toString(16).padStart(66, '0x00000')) + isSpent.should.be.equal(false) const { logs } = await mixer.withdraw(pi_a, pi_b, pi_c, publicSignals, { from: relayer, gasPrice: '0' }) @@ -209,9 +211,12 @@ contract('Mixer', accounts => { balanceRelayerAfter.should.be.eq.BN(toBN(balanceRelayerBefore).add(feeBN)) balanceRecieverAfter.should.be.eq.BN(toBN(balanceRecieverBefore).add(toBN(value)).sub(feeBN)) + logs[0].event.should.be.equal('Withdraw') - logs[0].args.nullifier.should.be.eq.BN(toBN(input.nullifierHash.toString())) + logs[0].args.nullifierHash.should.be.eq.BN(toBN(input.nullifierHash.toString())) logs[0].args.fee.should.be.eq.BN(feeBN) + isSpent = await mixer.isSpent(input.nullifierHash.toString(16).padStart(66, '0x00000')) + isSpent.should.be.equal(true) }) it('should prevent double spend', async () => { From 447d0a86f47f45f0bc2e4562528ac8ed4422a5b2 Mon Sep 17 00:00:00 2001 From: Roman Semenov Date: Thu, 25 Jul 2019 21:29:09 +0300 Subject: [PATCH 3/7] Update README.md --- README.md | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 281e3b3..8c133f7 100644 --- a/README.md +++ b/README.md @@ -15,12 +15,10 @@ * Relayer is frontrunnable. When relayer submits a transaction someone can see it in tx pool and frontrun it with higher gas price to get the fee and drain relayer funds. * Workaround: we can set high gas price so that (almost) all fee is used on gas. The relayer will not receive profit this way, but this approach is acceptable until we develop more sophisticated system that prevents frontrunning * Bugs in contract. Even though we have an extensive experience in smart contract security audits, we can still make mistakes. An external audit is needed to reduce probablility of bugs -* Nullifier griefing. when you submit a withdraw transaction you reveal the nullifier for your note. If someone manages to +* ~~Nullifier griefing. when you submit a withdraw transaction you reveal the nullifier for your note. If someone manages to make a deposit with the same nullifier and withdraw it while your transaction is still in tx pool, your note will be considered -spent since it has the same nullifier and it will prevent you from withdrawing your funds - * This attack doesnt't provide any profit for the attacker - * This can be solved by storing block number for merkle root history, and only allowing to withdraw using merkle roots that are older than N ~10-20 blocks. - It will slightly reduce anonymity set (by not counting users that deposited in last N blocks), but provide a safe period for mining your withdrawal transactions. +spent since it has the same nullifier and it will prevent you from withdrawing your funds~~ + * Fixed by sending nullifier hash instead of plain nullifier ## Requirements 1. `node v11.15.0` From 787d1cc5d0d0d93e3d458e5a8da75470973fcb2e Mon Sep 17 00:00:00 2001 From: Roman Semenov Date: Thu, 25 Jul 2019 21:30:50 +0300 Subject: [PATCH 4/7] Update README.md --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 8c133f7..e48873e 100644 --- a/README.md +++ b/README.md @@ -11,7 +11,7 @@ ## Security risks * Cryptographic tools used by mixer (zkSNARKS, Pedersen commitment, MiMC hash) are not yet extensively audited by cryptographic experts and may be vulnerable - * Note: we use MiMC hash only for merkle tree, so even if a preimage attack on MiMC is discovered, it will not allow to deanonymize users or drain mixer funds + * Note: we use MiMC hash only for merkle tree, so even if a preimage attack on MiMC is discovered, it will not allow to deanonymize users. To drain funds attacker needs to be able to generate arbitrary hash collisions, which is a pretty strong assumption. * Relayer is frontrunnable. When relayer submits a transaction someone can see it in tx pool and frontrun it with higher gas price to get the fee and drain relayer funds. * Workaround: we can set high gas price so that (almost) all fee is used on gas. The relayer will not receive profit this way, but this approach is acceptable until we develop more sophisticated system that prevents frontrunning * Bugs in contract. Even though we have an extensive experience in smart contract security audits, we can still make mistakes. An external audit is needed to reduce probablility of bugs From 791875ddc53ebdf8ddf73147a92d54cdd7f60904 Mon Sep 17 00:00:00 2001 From: Roman Storm Date: Thu, 1 Aug 2019 00:33:12 -0700 Subject: [PATCH 5/7] fix overflow --- package-lock.json | 4 ++-- package.json | 4 ++-- test/Mixer.test.js | 39 +++++++++++++++++++++++++++++++++++++-- 3 files changed, 41 insertions(+), 6 deletions(-) diff --git a/package-lock.json b/package-lock.json index c36dc44..3572957 100644 --- a/package-lock.json +++ b/package-lock.json @@ -6403,8 +6403,8 @@ } }, "snarkjs": { - "version": "git+https://github.com/iden3/snarkjs.git#5fe2bd4642ec567c75ad5ac3f73687999c412e73", - "from": "git+https://github.com/iden3/snarkjs.git#5fe2bd4642ec567c75ad5ac3f73687999c412e73", + "version": "git+https://github.com/iden3/snarkjs.git#c428706ef69930e378c31199ff8d66ee13fada85", + "from": "git+https://github.com/iden3/snarkjs.git#c428706ef69930e378c31199ff8d66ee13fada85", "requires": { "big-integer": "^1.6.43", "chai": "^4.2.0", diff --git a/package.json b/package.json index d35d025..80e9ce0 100644 --- a/package.json +++ b/package.json @@ -27,10 +27,10 @@ "circom": "0.0.30", "circomlib": "^0.0.10", "dotenv": "^8.0.0", - "express": "^4.17.1", "eslint": "^6.0.1", + "express": "^4.17.1", "ganache-cli": "^6.4.5", - "snarkjs": "git+https://github.com/iden3/snarkjs.git#5fe2bd4642ec567c75ad5ac3f73687999c412e73", + "snarkjs": "git+https://github.com/iden3/snarkjs.git#c428706ef69930e378c31199ff8d66ee13fada85", "truffle": "^5.0.27", "truffle-artifactor": "^4.0.23", "truffle-contract": "^4.0.24", diff --git a/test/Mixer.test.js b/test/Mixer.test.js index 1ae0cf4..687f859 100644 --- a/test/Mixer.test.js +++ b/test/Mixer.test.js @@ -220,10 +220,16 @@ contract('Mixer', accounts => { }) it('should prevent double spend', async () => { + const deposit = generateDeposit() await tree.insert(deposit.commitment) await mixer.deposit(toBN(deposit.commitment.toString()), { value, from: sender }) + const deposit2 = generateDeposit() + await tree.insert(deposit2.commitment) + await mixer.deposit(toBN(deposit2.commitment.toString()), { value, from: sender }) + + const { root, path_elements, path_index } = await tree.path(0) const input = stringifyBigInts({ @@ -236,14 +242,44 @@ contract('Mixer', accounts => { pathElements: path_elements, pathIndex: path_index, }) - const proof = await websnarkUtils.genWitnessAndProve(groth16, input, circuit, proving_key) const { pi_a, pi_b, pi_c, publicSignals } = websnarkUtils.toSolidityInput(proof) + // publicSignals[1] ='0x' + toBN(publicSignals[1]).add(toBN('21888242871839275222246405745257275088548364400416034343698204186575808495617')).toString('hex') await mixer.withdraw(pi_a, pi_b, pi_c, publicSignals, { from: relayer }).should.be.fulfilled const error = await mixer.withdraw(pi_a, pi_b, pi_c, publicSignals, { from: relayer }).should.be.rejected error.reason.should.be.equal('The note has been already spent') }) + it('should prevent double spend with overflow', async () => { + + const deposit = generateDeposit() + await tree.insert(deposit.commitment) + await mixer.deposit(toBN(deposit.commitment.toString()), { value, from: sender }) + + const deposit2 = generateDeposit() + await tree.insert(deposit2.commitment) + await mixer.deposit(toBN(deposit2.commitment.toString()), { value, from: sender }) + + + const { root, path_elements, path_index } = await tree.path(0) + + const input = stringifyBigInts({ + root, + nullifierHash: pedersenHash(deposit.nullifier.leInt2Buff(32)), + nullifier: deposit.nullifier, + receiver, + fee, + secret: deposit.secret, + pathElements: path_elements, + pathIndex: path_index, + }) + const proof = await websnarkUtils.genWitnessAndProve(groth16, input, circuit, proving_key) + const { pi_a, pi_b, pi_c, publicSignals } = websnarkUtils.toSolidityInput(proof) + publicSignals[1] ='0x' + toBN(publicSignals[1]).add(toBN('21888242871839275222246405745257275088548364400416034343698204186575808495617')).toString('hex') + const error = await mixer.withdraw(pi_a, pi_b, pi_c, publicSignals, { from: relayer }).should.be.rejected + error.reason.should.be.equal('verifier-gte-snark-scalar-field') + }) + it('fee should be less or equal transfer value', async () => { const deposit = generateDeposit() await tree.insert(deposit.commitment) @@ -312,7 +348,6 @@ contract('Mixer', accounts => { pathElements: path_elements, pathIndex: path_index, }) - const proof = await websnarkUtils.genWitnessAndProve(groth16, input, circuit, proving_key) let { pi_a, pi_b, pi_c, publicSignals } = websnarkUtils.toSolidityInput(proof) const originalPublicSignals = publicSignals.slice() From 242a87569baaab6be6918d9d32c368bcc0c412ee Mon Sep 17 00:00:00 2001 From: Roman Storm Date: Thu, 1 Aug 2019 00:34:22 -0700 Subject: [PATCH 6/7] clean up tests --- test/Mixer.test.js | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/test/Mixer.test.js b/test/Mixer.test.js index 687f859..16a0b39 100644 --- a/test/Mixer.test.js +++ b/test/Mixer.test.js @@ -220,16 +220,10 @@ contract('Mixer', accounts => { }) it('should prevent double spend', async () => { - const deposit = generateDeposit() await tree.insert(deposit.commitment) await mixer.deposit(toBN(deposit.commitment.toString()), { value, from: sender }) - const deposit2 = generateDeposit() - await tree.insert(deposit2.commitment) - await mixer.deposit(toBN(deposit2.commitment.toString()), { value, from: sender }) - - const { root, path_elements, path_index } = await tree.path(0) const input = stringifyBigInts({ @@ -251,16 +245,10 @@ contract('Mixer', accounts => { }) it('should prevent double spend with overflow', async () => { - const deposit = generateDeposit() await tree.insert(deposit.commitment) await mixer.deposit(toBN(deposit.commitment.toString()), { value, from: sender }) - const deposit2 = generateDeposit() - await tree.insert(deposit2.commitment) - await mixer.deposit(toBN(deposit2.commitment.toString()), { value, from: sender }) - - const { root, path_elements, path_index } = await tree.path(0) const input = stringifyBigInts({ From 9b14a22b0d731aa65f6e43f585bad86d8fc332fc Mon Sep 17 00:00:00 2001 From: Roman Storm Date: Thu, 1 Aug 2019 00:34:59 -0700 Subject: [PATCH 7/7] remove comment --- test/Mixer.test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/Mixer.test.js b/test/Mixer.test.js index 16a0b39..4a12fdb 100644 --- a/test/Mixer.test.js +++ b/test/Mixer.test.js @@ -238,7 +238,6 @@ contract('Mixer', accounts => { }) const proof = await websnarkUtils.genWitnessAndProve(groth16, input, circuit, proving_key) const { pi_a, pi_b, pi_c, publicSignals } = websnarkUtils.toSolidityInput(proof) - // publicSignals[1] ='0x' + toBN(publicSignals[1]).add(toBN('21888242871839275222246405745257275088548364400416034343698204186575808495617')).toString('hex') await mixer.withdraw(pi_a, pi_b, pi_c, publicSignals, { from: relayer }).should.be.fulfilled const error = await mixer.withdraw(pi_a, pi_b, pi_c, publicSignals, { from: relayer }).should.be.rejected error.reason.should.be.equal('The note has been already spent')