Merge branch 'master' of github.com:poma/mixer-huyixer

README.md

@@ -11,16 +11,14 @@
 
 ## 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
-* 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`

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),
 

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];
   }
 }

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",

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",

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 () => {
@@ -231,7 +236,6 @@ 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)
       await mixer.withdraw(pi_a, pi_b, pi_c, publicSignals, { from: relayer }).should.be.fulfilled
@@ -239,6 +243,30 @@ contract('Mixer', accounts => {
       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 { 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)
@@ -307,7 +335,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()