From 5c16e02c909b30b2b19a6f1bdd0ab21672cd4213 Mon Sep 17 00:00:00 2001 From: Alexey Date: Mon, 15 Jul 2019 18:04:48 +0300 Subject: [PATCH] off-by-one fix and tests --- contracts/MerkleTreeWithHistory.sol | 2 +- contracts/Mixer.sol | 4 + test/Mixer.test.js | 113 +++++++++++++++++++++++++--- 3 files changed, 107 insertions(+), 12 deletions(-) diff --git a/contracts/MerkleTreeWithHistory.sol b/contracts/MerkleTreeWithHistory.sol index 9fc2430..587d3aa 100644 --- a/contracts/MerkleTreeWithHistory.sol +++ b/contracts/MerkleTreeWithHistory.sol @@ -84,7 +84,7 @@ contract MerkleTreeWithHistory { } // search most recent first uint256 i; - for(i = current_root; i >= 0; i--) { + for(i = current_root; i < 2**256 - 1; i--) { if (root == _roots[i]) { return true; } diff --git a/contracts/Mixer.sol b/contracts/Mixer.sol index 7bc36de..3bf4027 100644 --- a/contracts/Mixer.sol +++ b/contracts/Mixer.sol @@ -9,6 +9,8 @@ contract IVerifier { contract Mixer is MerkleTreeWithHistory { uint256 public transferValue; mapping(uint256 => bool) public nullifiers; + // we store all commitments just to prevent accidental deposits with the same commitment + mapping(uint256 => bool) public commitments; IVerifier verifier; event Deposit(address from, uint256 commitment); @@ -35,7 +37,9 @@ contract Mixer is MerkleTreeWithHistory { */ function deposit(uint256 commitment) public payable { require(msg.value == transferValue, "Please send `transferValue` ETH along with transaction"); + require(!commitments[commitment], "The commitment has been submitted"); _insert(commitment); + commitments[commitment] = true; emit Deposit(msg.sender, commitment); } diff --git a/test/Mixer.test.js b/test/Mixer.test.js index 39dc6bf..c3afb5f 100644 --- a/test/Mixer.test.js +++ b/test/Mixer.test.js @@ -3,15 +3,15 @@ const should = require('chai') .use(require('chai-as-promised')) .should() -const { toWei, toBN, fromWei, toHex } = require('web3-utils') +const { toWei, toBN, fromWei, toHex, randomHex } = require('web3-utils') const { takeSnapshot, revertSnapshot, increaseTime } = require('../scripts/ganacheHelper'); const Mixer = artifacts.require('./Mixer.sol') const { AMOUNT } = process.env -const utils = require("../scripts/utils") -const stringifyBigInts = require("websnark/tools/stringifybigint").stringifyBigInts -const snarkjs = require("snarkjs"); +const utils = require('../scripts/utils') +const stringifyBigInts = require('websnark/tools/stringifybigint').stringifyBigInts +const snarkjs = require('snarkjs'); const bigInt = snarkjs.bigInt; const MerkleTree = require('../lib/MerkleTree') @@ -25,6 +25,14 @@ function generateDeposit() { return deposit; } +function BNArrayToStringArray(array) { + const arrayToPrint = [] + array.forEach(item => { + arrayToPrint.push(item.toString()) + }) + return arrayToPrint +} + contract('Mixer', async accounts => { let mixer const sender = accounts[0] @@ -34,6 +42,9 @@ contract('Mixer', async accounts => { let snapshotId let prefix = 'test' let tree + const fee = bigInt(1e17) + const receiver = utils.rbigint(20) + const relayer = accounts[1] before(async () => { tree = new MerkleTree( @@ -65,14 +76,18 @@ contract('Mixer', async accounts => { logs[1].args.from.should.be.equal(sender) logs[1].args.commitment.should.be.eq.BN(toBN(commitment)) }) + + it('should throw if there is a such commitment', async () => { + const commitment = 42 + await mixer.deposit(commitment, { value: AMOUNT, from: sender }).should.be.fulfilled + const error = await mixer.deposit(commitment, { value: AMOUNT, from: sender }).should.be.rejected + error.reason.should.be.equal('The commitment has been submitted') + }) }) describe('#withdraw', async () => { it('should work', async () => { - const receiver = utils.rbigint(20) - let fee = bigInt(1e17) const deposit = generateDeposit() - const relayer = sender const user = accounts[4] await tree.insert(deposit.commitment) @@ -110,19 +125,95 @@ contract('Mixer', async accounts => { const balanceMixerAfter = await web3.eth.getBalance(mixer.address) const balanceRelayerAfter = await web3.eth.getBalance(relayer) const balanceRecieverAfter = await web3.eth.getBalance(toHex(receiver.toString())) - fee = toBN(fee.toString()) + const feeBN = toBN(fee.toString()) balanceMixerAfter.should.be.eq.BN(toBN(balanceMixerBefore).sub(toBN(AMOUNT))) - balanceRelayerAfter.should.be.eq.BN(toBN(balanceRelayerBefore).add(fee)) - balanceRecieverAfter.should.be.eq.BN(toBN(balanceRecieverBefore).add(toBN(AMOUNT)).sub(fee)) + balanceRelayerAfter.should.be.eq.BN(toBN(balanceRelayerBefore).add(feeBN)) + balanceRecieverAfter.should.be.eq.BN(toBN(balanceRecieverBefore).add(toBN(AMOUNT)).sub(feeBN)) logs[0].event.should.be.equal('Withdraw') logs[0].args.nullifier.should.be.eq.BN(toBN(deposit.nullifier.toString())) - logs[0].args.fee.should.be.eq.BN(fee) + logs[0].args.fee.should.be.eq.BN(feeBN) + }) + + it('should prevent double spend', async () => { + const deposit = generateDeposit() + await tree.insert(deposit.commitment) + await mixer.deposit(toBN(deposit.commitment.toString()), { value: AMOUNT, from: sender }) + + const {root, path_elements, path_index} = await tree.path(0); + + const input = stringifyBigInts({ + root, + nullifier: deposit.nullifier, + receiver, + fee, + secret: deposit.secret, + pathElements: path_elements, + pathIndex: path_index, + }) + + const { pi_a, pi_b, pi_c, publicSignals } = await utils.snarkProof(input) + 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('fee should be less or equal transfer value', async () => { + const deposit = generateDeposit() + await tree.insert(deposit.commitment) + await mixer.deposit(toBN(deposit.commitment.toString()), { value: AMOUNT, from: sender }) + + const {root, path_elements, path_index} = await tree.path(0); + oneEtherFee = bigInt(1e18) // 1 ether + const input = stringifyBigInts({ + root, + nullifier: deposit.nullifier, + receiver, + fee: oneEtherFee, + secret: deposit.secret, + pathElements: path_elements, + pathIndex: path_index, + }) + + const { pi_a, pi_b, pi_c, publicSignals } = await utils.snarkProof(input) + const error = await mixer.withdraw(pi_a, pi_b, pi_c, publicSignals, { from: relayer }).should.be.rejected + error.reason.should.be.equal('Fee exceeds transfer value') + }) + + it('should throw for corrupted merkle tree root', async () => { + const deposit = generateDeposit() + await tree.insert(deposit.commitment) + await mixer.deposit(toBN(deposit.commitment.toString()), { value: AMOUNT, from: sender }) + + const {root, path_elements, path_index} = await tree.path(0) + + const input = stringifyBigInts({ + root, + nullifier: deposit.nullifier, + receiver, + fee, + secret: deposit.secret, + pathElements: path_elements, + pathIndex: path_index, + }) + + const dummyRoot = randomHex(32) + const { pi_a, pi_b, pi_c, publicSignals } = await utils.snarkProof(input) + publicSignals[0] = dummyRoot + + const error = await mixer.withdraw(pi_a, pi_b, pi_c, publicSignals, { from: relayer }).should.be.rejected + error.reason.should.be.equal('Cannot find your merkle root') }) }) afterEach(async () => { await revertSnapshot(snapshotId.result) snapshotId = await takeSnapshot() + tree = new MerkleTree( + levels, + zeroValue, + null, + prefix, + ) }) })