Merge pull request #3 from peppersec/prevent-griefing

prevent nullifier griefing

.nvmrc

@@ -0,0 +1 @@
+11

README.md

@@ -32,8 +32,8 @@ spent since it has the same nullifier and it will prevent you from withdrawing y
 1. `npm run build:circuit` - may take 10 minutes or more
 1. `npm run build:contract`
 1. `npm run browserify`
-1. `npm run test` - optionally run tests. It may fail for the first time, just run one more time.
 1. `npx ganache-cli`
+1. `npm run test` - optionally run tests. It may fail for the first time, just run one more time.
 1. `npm run migrate:dev`
 1. `./cli.js deposit`
 1. `./cli.js withdraw <note from previous step> <destination eth address>`
@@ -45,5 +45,5 @@ spent since it has the same nullifier and it will prevent you from withdrawing y
 
 ## Credits
 
-Special thanks to @barryWhiteHat and @kobigurk for valuable input, 
+Special thanks to @barryWhiteHat and @kobigurk for valuable input,
 and to @jbaylina for awesome [Circom](https://github.com/iden3/circom) & [Websnark](https://github.com/iden3/websnark) framework

circuits/withdraw.circom

@@ -4,31 +4,36 @@ include "merkleTree.circom";
 
 // computes Pedersen(nullifier + secret)
 template CommitmentHasher() {
-    signal input nullifier;
+    signal private input nullifier;
     signal private input secret;
 
-    signal output hash;
+    signal output commitment;
+    signal output nullifierHash;
 
-    component commitment = Pedersen(512);
+    component commitmentHasher = Pedersen(512);
+    component nullifierHasher = Pedersen(256);
     component nullifierBits = Num2Bits(256);
     component secretBits = Num2Bits(256);
     nullifierBits.in <== nullifier;
     secretBits.in <== secret;
     for (var i = 0; i < 256; i++) {
-        commitment.in[i] <== nullifierBits.out[i];
-        commitment.in[i + 256] <== secretBits.out[i];
+        nullifierHasher.in[i] <== nullifierBits.out[i];
+        commitmentHasher.in[i] <== nullifierBits.out[i];
+        commitmentHasher.in[i + 256] <== secretBits.out[i];
     }
 
-    hash <== commitment.out[0];
+    commitment <== commitmentHasher.out[0];
+    nullifierHash <== nullifierHasher.out[0];
 }
 
 // Verifies that commitment that corresponds to given secret and nullifier is included in the merkle tree of deposits
 template Withdraw(levels, rounds) {
     signal input root;
-    signal input nullifier;
+    signal input nullifierHash;
     // TODO: Check if we need some kind of explicit constraints or something for those 2 inputs
     signal input receiver; // not taking part in any computations
     signal input fee; // not taking part in any computations
+    signal private input nullifier;
     signal private input secret;
     signal private input pathElements[levels];
     signal private input pathIndex[levels];
@@ -37,8 +42,10 @@ template Withdraw(levels, rounds) {
     hasher.nullifier <== nullifier;
     hasher.secret <== secret;
 
+    nullifierHash === hasher.nullifierHash;
+
     component tree = MerkleTree(levels, rounds);
-    tree.leaf <== hasher.hash;
+    tree.leaf <== hasher.commitment;
     tree.root <== root;
     for (var i = 0; i < levels; i++) {
         tree.pathElements[i] <== pathElements[i];

cli.js

@@ -64,11 +64,12 @@ async function withdraw(note, receiver) {
   const input = {
     // public
     root: root,
-    nullifier: deposit.nullifier,
+    nullifierHash: pedersenHash(deposit.nullifier.leInt2Buff(32)),
     receiver: bigInt(receiver),
     fee: bigInt(0),
 
     // private
+    nullifier: deposit.nullifier,
     secret: deposit.secret,
     pathElements: path_elements,
     pathIndex: path_index,

test/Mixer.test.js

@@ -127,6 +127,7 @@ contract('Mixer', accounts => {
 
       const input = stringifyBigInts({
         root,
+        nullifierHash: pedersenHash(deposit.nullifier.leInt2Buff(32)),
         nullifier: deposit.nullifier,
         receiver,
         fee,
@@ -179,16 +180,18 @@ contract('Mixer', accounts => {
       const input = stringifyBigInts({
         // public
         root,
-        nullifier: deposit.nullifier,
+        nullifierHash: pedersenHash(deposit.nullifier.leInt2Buff(32)),
         receiver,
         fee,
 
         // private
+        nullifier: deposit.nullifier,
         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)
 
@@ -207,7 +210,7 @@ contract('Mixer', accounts => {
       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(deposit.nullifier.toString()))
+      logs[0].args.nullifier.should.be.eq.BN(toBN(input.nullifierHash.toString()))
       logs[0].args.fee.should.be.eq.BN(feeBN)
     })
 
@@ -220,6 +223,7 @@ contract('Mixer', accounts => {
 
       const input = stringifyBigInts({
         root,
+        nullifierHash: pedersenHash(deposit.nullifier.leInt2Buff(32)),
         nullifier: deposit.nullifier,
         receiver,
         fee,
@@ -244,6 +248,7 @@ contract('Mixer', accounts => {
       const oneEtherFee = bigInt(1e18) // 1 ether
       const input = stringifyBigInts({
         root,
+        nullifierHash: pedersenHash(deposit.nullifier.leInt2Buff(32)),
         nullifier: deposit.nullifier,
         receiver,
         fee: oneEtherFee,
@@ -266,6 +271,7 @@ contract('Mixer', accounts => {
       const { root, path_elements, path_index } = await tree.path(0)
 
       const input = stringifyBigInts({
+        nullifierHash: pedersenHash(deposit.nullifier.leInt2Buff(32)),
         root,
         nullifier: deposit.nullifier,
         receiver,
@@ -293,6 +299,7 @@ contract('Mixer', accounts => {
 
       const input = stringifyBigInts({
         root,
+        nullifierHash: pedersenHash(deposit.nullifier.leInt2Buff(32)),
         nullifier: deposit.nullifier,
         receiver,
         fee,