Merge pull request #1 from tornadocash/feat/audit_fixes

Feat/audit fixes

circuits/README.md

@@ -512,7 +512,7 @@ Implementation of MiMC-7 hash in Fp being...  (link to description of the hash)
 
 ### mimcsponge
 
-- `MiMCSponge(nInputs, nRounds, nOutputs)`
+- `MiMCSponge(nInputs, nOutputs)`
 
     - DESCRIPTION
     - SCHEMA

circuits/aliascheck.circom

@@ -21,7 +21,6 @@ include "compconstant.circom";
 
 
 template AliasCheck() {
-
     signal input in[254];
 
     component  compConstant = CompConstant(-1);
@@ -30,3 +29,15 @@ template AliasCheck() {
 
     compConstant.out === 0;
 }
+
+template AliasCheckBabyJub() {
+    signal input in[251];
+    signal input enabled;
+
+    component  compConstant = CompConstant(2736030358979909402780800718157159386076813972158567259200215660948447373040);
+
+    for (var i=0; i<251; i++) in[i] ==> compConstant.in[i];
+    for (var i=0; i<3; i++) 0 ==> compConstant.in[251+i];
+
+    compConstant.out*enabled === 0;
+}

circuits/babyjub.circom

@@ -81,7 +81,7 @@ template BabyCheck() {
     a*x2 + y2 === 1 + d*x2*y2;
 }
 
-// Extracts the public key from private key
+// Extracts the public key from private key, as mentioned in https://tools.ietf.org/html/rfc8032
 template BabyPbk() {
     signal private input  in;
     signal         output Ax;

circuits/binsum.circom

@@ -50,6 +50,7 @@ To waranty binary outputs:
     This function calculates the number of extra bits in the output to do the full sum.
  */
 
+/* a must be < Nq/2, where Nq is the number of elements in the scalar field */
 function nbits(a) {
     var n = 1;
     var r = 0;
@@ -61,6 +62,7 @@ function nbits(a) {
 }
 
 
+/* n must be such that (2**(n+1) -2) < Nq/ops, where Nq is the number of bits in the scalar field */
 template BinSum(n, ops) {
     var nout = nbits((2**n -1)*ops);
     signal input in[ops][n];

circuits/bitify.circom

@@ -21,6 +21,7 @@ include "comparators.circom";
 include "aliascheck.circom";
 
 
+/* This doesn't check aliasing, so for n > 253 there are multiple bit strings for each number */
 template Num2Bits(n) {
     signal input in;
     signal output out[n];
@@ -76,6 +77,7 @@ template Bits2Num_strict() {
     b2n.out ==> out;
 }
 
+/* n must not exceed 253 */  
 template Num2BitsNeg(n) {
     signal input in;
     signal output out[n];

circuits/eddsa.circom

@@ -17,7 +17,7 @@
     along with circom. If not, see <https://www.gnu.org/licenses/>.
 */
 
-include "compconstant.circom";
+include "aliascheck.circom";
 include "pointbits.circom";
 include "pedersen.circom";
 include "escalarmulany.circom";
@@ -40,12 +40,15 @@ template EdDSAVerifier(n) {
 
 // Ensure S<Subgroup Order
 
-    component  compConstant = CompConstant(2736030358979909402780800718157159386076813972158567259200215660948447373040);
+    component aliasCheck = AliasCheckBabyJub();
+    aliasCheck.enabled <== 1;
 
-    for (i=0; i<254; i++) {
+    for (i=0; i<251; i++) {
-        S[i] ==> compConstant.in[i];
+        S[i] ==> aliasCheck.in[i];
     }
-    compConstant.out === 0;
+    S[251] === 0;
+    S[252] === 0;
+    S[253] === 0;
     S[254] === 0;
     S[255] === 0;
 

circuits/eddsamimc.circom

@@ -17,7 +17,7 @@
     along with circom. If not, see <https://www.gnu.org/licenses/>.
 */
 
-include "compconstant.circom";
+include "aliascheck.circom";
 include "pointbits.circom";
 include "mimc.circom";
 include "bitify.circom";
@@ -39,16 +39,15 @@ template EdDSAMiMCVerifier() {
 
 // Ensure S<Subgroup Order
 
-    component snum2bits = Num2Bits(253);
+    component snum2bits = Num2Bits(251);
     snum2bits.in <== S;
 
-    component  compConstant = CompConstant(2736030358979909402780800718157159386076813972158567259200215660948447373040);
+    component aliasCheck = AliasCheckBabyJub();
+    aliasCheck.enabled <== 1;
 
-    for (i=0; i<253; i++) {
+    for (i=0; i<251; i++) {
-        snum2bits.out[i] ==> compConstant.in[i];
+        snum2bits.out[i] ==> aliasCheck.in[i];
     }
-    compConstant.in[253] <== 0;
-    compConstant.out === 0;
 
 // Calculate the h = H(R,A, msg)
 
@@ -104,8 +103,8 @@ template EdDSAMiMCVerifier() {
         5299619240641551281634865583518297030282874472190772894086521144482721001553,
         16950150798460657717958625567821834550301663161624707787222815936182638968203
     ];
-    component mulFix = EscalarMulFix(253, BASE8);
+    component mulFix = EscalarMulFix(251, BASE8);
-    for (i=0; i<253; i++) {
+    for (i=0; i<251; i++) {
         mulFix.e[i] <== snum2bits.out[i];
     }
 

circuits/eddsamimcsponge.circom

@@ -17,7 +17,7 @@
     along with circom. If not, see <https://www.gnu.org/licenses/>.
 */
 
-include "compconstant.circom";
+include "aliascheck.circom";
 include "pointbits.circom";
 include "mimcsponge.circom";
 include "bitify.circom";
@@ -39,20 +39,19 @@ template EdDSAMiMCSpongeVerifier() {
 
 // Ensure S<Subgroup Order
 
-    component snum2bits = Num2Bits(253);
+    component snum2bits = Num2Bits(251);
     snum2bits.in <== S;
 
-    component  compConstant = CompConstant(2736030358979909402780800718157159386076813972158567259200215660948447373040);
+    component aliasCheck = AliasCheckBabyJub();
+    aliasCheck.enabled <== 1;
 
-    for (i=0; i<253; i++) {
+    for (i=0; i<251; i++) {
-        snum2bits.out[i] ==> compConstant.in[i];
+        snum2bits.out[i] ==> aliasCheck.in[i];
     }
-    compConstant.in[253] <== 0;
-    compConstant.out === 0;
 
 // Calculate the h = H(R,A, msg)
 
-    component hash = MiMCSponge(5, 220, 1);
+    component hash = MiMCSponge(5, 1);
     hash.ins[0] <== R8x;
     hash.ins[1] <== R8y;
     hash.ins[2] <== Ax;
@@ -104,8 +103,8 @@ template EdDSAMiMCSpongeVerifier() {
         5299619240641551281634865583518297030282874472190772894086521144482721001553,
         16950150798460657717958625567821834550301663161624707787222815936182638968203
     ];
-    component mulFix = EscalarMulFix(253, BASE8);
+    component mulFix = EscalarMulFix(251, BASE8);
-    for (i=0; i<253; i++) {
+    for (i=0; i<251; i++) {
         mulFix.e[i] <== snum2bits.out[i];
     }
 

circuits/eddsaposeidon.circom

@@ -38,16 +38,15 @@ template EdDSAPoseidonVerifier() {
 
 // Ensure S<Subgroup Order
 
-    component snum2bits = Num2Bits(253);
+    component snum2bits = Num2Bits(251);
     snum2bits.in <== S;
 
-    component  compConstant = CompConstant(2736030358979909402780800718157159386076813972158567259200215660948447373040);
+    component aliasCheck = AliasCheckBabyJub();
+    aliasCheck.enabled <== enabled;
 
-    for (i=0; i<253; i++) {
+    for (i=0; i<251; i++) {
-        snum2bits.out[i] ==> compConstant.in[i];
+        snum2bits.out[i] ==> aliasCheck.in[i];
     }
-    compConstant.in[253] <== 0;
-    compConstant.out*enabled === 0;
 
 // Calculate the h = H(R,A, msg)
 
@@ -103,8 +102,8 @@ template EdDSAPoseidonVerifier() {
         5299619240641551281634865583518297030282874472190772894086521144482721001553,
         16950150798460657717958625567821834550301663161624707787222815936182638968203
     ];
-    component mulFix = EscalarMulFix(253, BASE8);
+    component mulFix = EscalarMulFix(251, BASE8);
-    for (i=0; i<253; i++) {
+    for (i=0; i<251; i++) {
         mulFix.e[i] <== snum2bits.out[i];
     }
 

circuits/escalarmulfix.circom

@@ -44,6 +44,7 @@ include "babyjub.circom";
     A good way to see it is that the accumulator input of the adder >= 2^247*B and the other input
     is the output of the windows that it's going to be <= 2^246*B
  */
+ /* base must not be the neutral element nor points of small order */
 template WindowMulFix() {
     signal input in[3];
     signal input base[2];
@@ -133,11 +134,12 @@ template WindowMulFix() {
 
 /*
     This component does a multiplication of a escalar times a fix base
+    nWindows must not exceed 82
     Signals:
         e: The scalar in bits
         base: the base point in edwards format
         out:  The result
-        dbl: Point in Edwards to be linked to the next segment.
+        dbl: Point in Montgomery to be linked to the next segment.
  */
 
 template SegmentMulFix(nWindows) {
@@ -236,7 +238,7 @@ template EscalarMulFix(n, BASE) {
     signal output out[2];           // Point (Twisted format)
 
     var nsegments = (n-1)\246 +1;       // 249 probably would work. But I'm not sure and for security I keep 246
-    var nlastsegment = n - (nsegments-1)*249;
+    var nlastsegment = n - (nsegments-1)*246;
 
     component segments[nsegments];
 
@@ -250,13 +252,13 @@ template EscalarMulFix(n, BASE) {
 
     for (s=0; s<nsegments; s++) {
 
-        nseg = (s < nsegments-1) ? 249 : nlastsegment;
+        nseg = (s < nsegments-1) ? 246 : nlastsegment;
         nWindows = ((nseg - 1)\3)+1;
 
         segments[s] = SegmentMulFix(nWindows);
 
         for (i=0; i<nseg; i++) {
-            segments[s].e[i] <== e[s*249+i];
+            segments[s].e[i] <== e[s*246+i];
         }
 
         for (i = nseg; i<nWindows*3; i++) {

circuits/mimcsponge.circom

@@ -1,11 +1,13 @@
 // implements MiMC-2n/n as hash using a sponge construction.
 // log_5(21888242871839275222246405745257275088548364400416034343698204186575808495617) ~= 110
 // => nRounds should be 220
-template MiMCSponge(nInputs, nRounds, nOutputs) {
+template MiMCSponge(nInputs, nOutputs) {
   signal input ins[nInputs];
   signal input k;
   signal output outs[nOutputs];
 
+  var nRounds = 220;
+
   // S = R||C
   component S[nInputs + nOutputs - 1];
 

circuits/montgomery.circom

@@ -85,6 +85,7 @@ template Montgomery2Edwards() {
 
  */
 
+/* in1 must be != in2 */
 template MontgomeryAdd() {
     signal input in1[2];
     signal input in2[2];

circuits/pedersen.circom

@@ -108,6 +108,7 @@ template Window4() {
 }
 
 
+/* nWindows must not exceed 50 */
 template Segment(nWindows) {
     signal input in[nWindows*4];
     signal input base[2];

src/mimcsponge_gencontract.js

@@ -20,34 +20,29 @@ function createCode(seed, n) {
     C.push("0x00");
     C.mload();
     C.div();
-    C.push("0x3f1a1187"); // MiMCSponge(uint256,uint256,uint256)
+    C.push("0xf47d33b5"); // MiMCSponge(uint256,uint256)
     C.eq();
     C.jmpi("start");
     C.invalid();
 
     C.label("start");
     C.push("0x30644e72e131a029b85045b68181585d2833e84879b9709143e1f593f0000001");  // q
-    C.push("0x44");
-    C.mload();          // k q
     C.push("0x04");
-    C.mload();          // xL k q
+    C.mload();          // xL q
-    C.dup(2);           // q xL k q
+    C.dup(1);           // q xL q
     C.push("0x24");
-    C.mload();          // xR q xL k q
+    C.mload();          // xR q xL q
-    C.dup(1);           // q xR q xL k q
+    C.dup(1);           // q xR q xL q
-    C.dup(0);           // q q xR q xL k q
+    C.dup(3);           // xL q xR q xL q
-    C.dup(4);           // xL q q xR q xL k q
+    C.dup(1);           // q xL q xR q xL q
-    C.dup(6);           // k xL q q xR q xL k q
+    C.dup(0);           // q q xL q xR q xL q
-    C.addmod();         // t=k+xL q xR q xL k q
+    C.dup(2);           // xL q q xL q xR q xL q
-    C.dup(1);           // q t q xR q xL k q
+    C.dup(0);           // xL xL q q xL q xR q xL q
-    C.dup(0);           // q q t q xR q xL k q
+    C.mulmod();         // b=xL^2 q xL q xR q xL q
-    C.dup(2);           // t q q t q xR q xL k q
+    C.dup(0);           // b b q xL q xR q xL q
-    C.dup(0);           // t t q q t q xR q xL k q
+    C.mulmod();         // c=xL^4 xL q xR q xL q
-    C.mulmod();         // b=t^2 q t q xR q xL k q
+    C.mulmod();         // d=xL^5 xR q xL q
-    C.dup(0);           // b b q t q xR q xL k q
+    C.addmod();         // e=xL^5+xR xL q (for next round: xL xR q)
-    C.mulmod();         // c=t^4 t q xR q xL k q
-    C.mulmod();         // d=t^5 xR q xL k q
-    C.addmod();         // e=t^5+xR xL k q (for next round: xL xR k q)
 
     for (let i=0; i<n-1; i++) {
         if (i < n-2) {
@@ -55,27 +50,24 @@ function createCode(seed, n) {
         } else {
           ci = "0x00";
         }
-        C.swap(1);      // xR xL k q
+        C.swap(1);      // xR xL q
-        C.dup(3);       // q xR xL k q
+        C.dup(2);       // q xR xL q
-        C.dup(3);       // k q xR xL k q
+        C.dup(2);       // xL q xR xL q
-        C.dup(1);       // q k q xR xL k q
+        C.push(ci);     // ci xL q xR xL q
-        C.dup(4);       // xL q k q xR xL k q
+        C.addmod();     // a=ci+xL xR xL q
-        C.push(ci);     // ci xL q k q xR xL k q
+        C.dup(3);       // q a xR xL q
-        C.addmod();     // a=ci+xL k q xR xL k q
+        C.swap(1);      // a q xR xL q
-        C.addmod();     // t=a+k xR xL k q
+        C.dup(1);       // q a q xR xL q
-        C.dup(4);       // q t xR xL k q
+        C.dup(0);       // q q a q xR xL q
-        C.swap(1);      // t q xR xL k q
+        C.dup(2);       // a q q a q xR xL q
-        C.dup(1);       // q t q xR xL k q
+        C.dup(0);       // a a q q a q xR xL q
-        C.dup(0);       // q q t q xR xL k q
+        C.mulmod();     // b=a^2 q a q xR xL q
-        C.dup(2);       // t q q t q xR xL k q
+        C.dup(0);       // b b q a q xR xL q
-        C.dup(0);       // t t q q t q xR xL k q
+        C.mulmod();     // c=a^4 a q xR xL q
-        C.mulmod();     // b=t^2 q t q xR xL k q
+        C.mulmod();     // d=a^5 xR xL q
-        C.dup(0);       // b b q t q xR xL k q
+        C.dup(3);       // q d xR xL q
-        C.mulmod();     // c=t^4 t q xR xL k q
+        C.swap(2);      // xR d q xL q
-        C.mulmod();     // d=t^5 xR xL k q
+        C.addmod();     // e=a^5+xR xL q (for next round: xL xR q)
-        C.dup(4);       // q d xR xL k q
-        C.swap(2);      // xR d q xL k q
-        C.addmod();     // e=t^5+xR xL k q (for next round: xL xR k q)
     }
 
     C.push("0x20");
@@ -100,10 +92,6 @@ module.exports.abi = [
             {
                 "name": "xR_in",
                 "type": "uint256"
-            },
-            {
-                "name": "k",
-                "type": "uint256"
             }
         ],
         "name": "MiMCSponge",

test/aliascheck.js

@@ -57,6 +57,7 @@ describe("Aliascheck test", () => {
             assert(false);
         } catch(err) {
             assert(/Constraint\sdoesn't\smatch(.*)1\s!=\s0/.test(err.message) );
+            assert(err.message.indexOf("1 != 0") >= 0);
         }
     });
 
@@ -68,6 +69,7 @@ describe("Aliascheck test", () => {
             assert(false);
         } catch(err) {
             assert(/Constraint\sdoesn't\smatch(.*)1\s!=\s0/.test(err.message) );
+            assert(err.message.indexOf("1 != 0") >= 0);
         }
     });
 

test/aliascheckbabyjub.js

@@ -0,0 +1,75 @@
+const chai = require("chai");
+const path = require("path");
+const snarkjs = require("snarkjs");
+const compiler = require("circom");
+
+const assert = chai.assert;
+
+const bigInt = snarkjs.bigInt;
+
+function print(circuit, w, s) {
+    console.log(s + ": " + w[circuit.getSignalIdx(s)]);
+}
+
+function getBits(v, n) {
+    const res = [];
+    for (let i=0; i<n; i++) {
+        if (v.shr(i).isOdd()) {
+            res.push(bigInt.one);
+        } else {
+            res.push(bigInt.zero);
+        }
+    }
+    return res;
+}
+
+const r = bigInt("2736030358979909402780800718157159386076813972158567259200215660948447373041");
+
+describe("Aliascheck test", () => {
+    let circuit;
+    before( async() => {
+        const cirDef = await compiler(path.join(__dirname, "circuits", "aliascheckbabyjub_test.circom"));
+
+        circuit = new snarkjs.Circuit(cirDef);
+
+        console.log("NConstrains: " + circuit.nConstraints);
+    });
+
+    it("Satisfy the aliastest 0", async () => {
+        const inp = getBits(bigInt.zero, 251);
+        circuit.calculateWitness({in: inp});
+    });
+
+    it("Satisfy the aliastest 3", async () => {
+        const inp = getBits(bigInt(3), 251);
+        circuit.calculateWitness({in: inp});
+    });
+
+    it("Satisfy the aliastest r-1", async () => {
+        const inp = getBits(r.sub(bigInt.one), 251);
+        circuit.calculateWitness({in: inp});
+    });
+
+    it("Nhot not satisfy an input of r", async () => {
+        const inp = getBits(r, 251);
+        try {
+            circuit.calculateWitness({in: inp});
+            assert(false);
+        } catch(err) {
+            assert(err.message.indexOf("Constraint doesn't match") >= 0);
+            assert(err.message.indexOf("1 != 0") >= 0);
+        }
+    });
+
+    it("Nhot not satisfy all ones", async () => {
+        const inp = getBits(bigInt(1).shl(251).sub(bigInt(1)), 251);
+        try {
+            circuit.calculateWitness({in: inp});
+            assert(false);
+        } catch(err) {
+            assert(err.message.indexOf("Constraint doesn't match") >= 0);
+            assert(err.message.indexOf("1 != 0") >= 0);
+        }
+    });
+
+});

test/babyjub.js

@@ -101,6 +101,7 @@ describe("Baby Jub test", function () {
             assert(false, "Should be a valid point");
         } catch(err) {
             assert(/Constraint\sdoesn't\smatch(.*)168700\s!=\s1/.test(err.message) );
+            assert(err.message.indexOf("168700 != 1") >= 0);
         }
     });
 

test/circuits/aliascheckbabyjub_test.circom

@@ -0,0 +1,3 @@
+include "../../circuits/aliascheck.circom";
+
+component main = AliasCheckBabyJub()

test/circuits/eddsamimcsponge_test.circom

@@ -0,0 +1,3 @@
+include "../../circuits/eddsamimcsponge.circom";
+
+component main = EdDSAMiMCSpongeVerifier();

test/circuits/mimc_sponge_hash_test.circom

@@ -1,3 +1,3 @@
 include "../../circuits/mimcsponge.circom"
 
-component main = MiMCSponge(2, 220, 3);
+component main = MiMCSponge(2, 3);

test/eddsamimcsponge.js

@@ -0,0 +1,99 @@
+const chai = require("chai");
+const path = require("path");
+const snarkjs = require("snarkjs");
+const compiler = require("circom");
+
+const eddsa = require("../src/eddsa.js");
+
+const assert = chai.assert;
+
+const bigInt = snarkjs.bigInt;
+
+describe("EdDSA MiMCSponge test", function () {
+    let circuit;
+
+    this.timeout(100000);
+
+    before( async () => {
+        const cirDef = await compiler(path.join(__dirname, "circuits", "eddsamimcsponge_test.circom"));
+
+        circuit = new snarkjs.Circuit(cirDef);
+
+        console.log("NConstrains EdDSA MiMCSponge: " + circuit.nConstraints);
+    });
+
+    it("Sign a single number", async () => {
+        const msg = bigInt(1234);
+
+        const prvKey = Buffer.from("0001020304050607080900010203040506070809000102030405060708090001", "hex");
+
+        const pubKey = eddsa.prv2pub(prvKey);
+
+        const signature = eddsa.signMiMCSponge(prvKey, msg);
+
+        assert(eddsa.verifyMiMCSponge(msg, signature, pubKey));
+
+        const w = circuit.calculateWitness({
+            enabled: 1,
+            Ax: pubKey[0],
+            Ay: pubKey[1],
+            R8x: signature.R8[0],
+            R8y: signature.R8[1],
+            S: signature.S,
+            M: msg});
+
+        assert(circuit.checkWitness(w));
+    });
+
+    it("Detect Invalid signature", async () => {
+        const msg = bigInt(1234);
+
+        const prvKey = Buffer.from("0001020304050607080900010203040506070809000102030405060708090001", "hex");
+
+        const pubKey = eddsa.prv2pub(prvKey);
+
+
+        const signature = eddsa.signMiMCSponge(prvKey, msg);
+
+        assert(eddsa.verifyMiMCSponge(msg, signature, pubKey));
+        try {
+            const w = circuit.calculateWitness({
+                enabled: 1,
+                Ax: pubKey[0],
+                Ay: pubKey[1],
+                R8x: signature.R8[0].add(bigInt(1)),
+                R8y: signature.R8[1],
+                S: signature.S,
+                M: msg});
+            assert(false);
+        } catch(err) {
+            assert(err.message.indexOf("Constraint doesn't match") >= 0);
+            assert(err.message.indexOf("1 != 0") >= 0);
+        }
+    });
+
+
+    it("Test a dissabled circuit with a bad signature", async () => {
+        const msg = bigInt(1234);
+
+        const prvKey = Buffer.from("0001020304050607080900010203040506070809000102030405060708090001", "hex");
+
+        const pubKey = eddsa.prv2pub(prvKey);
+
+
+        const signature = eddsa.signMiMCSponge(prvKey, msg);
+
+        assert(eddsa.verifyMiMCSponge(msg, signature, pubKey));
+
+        const w = circuit.calculateWitness({
+            enabled: 0,
+            Ax: pubKey[0],
+            Ay: pubKey[1],
+            R8x: signature.R8[0].add(bigInt(1)),
+            R8y: signature.R8[1],
+            S: signature.S,
+            M: msg});
+
+        assert(circuit.checkWitness(w));
+    });
+});

test/mimcspongecontract.js

@@ -33,8 +33,8 @@ describe("MiMC Sponge Smart contract test", () => {
     });
 
     it("Shold calculate the mimc correctly", async () => {
-        const res = await mimc.methods.MiMCSponge(1,2,3).call();
+        const res = await mimc.methods.MiMCSponge(1,2).call();
-        const res2 = await mimcjs.hash(1,2,3);
+        const res2 = await mimcjs.hash(1,2, 0);
 
         assert.equal(res.xL.toString(), res2.xL.toString());
         assert.equal(res.xR.toString(), res2.xR.toString());