Merge pull request iden3#1 from tornadocash/feat/audit_fixes

Feat/audit fixes
jwasinger · Apr 6, 2020 · ce5dbe8 · ce5dbe8
2 parents 3c2b566 + 3478226
commit ce5dbe8
Show file tree

Hide file tree

Showing 22 changed files with 280 additions and 88 deletions.
diff --git a/circuits/README.md b/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

diff --git a/circuits/aliascheck.circom b/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;
+}
diff --git a/circuits/babyjub.circom b/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;

diff --git a/circuits/binsum.circom b/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];

diff --git a/circuits/bitify.circom b/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];

diff --git a/circuits/eddsa.circom b/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++) {
-        S[i] ==> compConstant.in[i];
+    for (i=0; i<251; i++) {
+        S[i] ==> aliasCheck.in[i];
     }
-    compConstant.out === 0;
+    S[251] === 0;
+    S[252] === 0;
+    S[253] === 0;
     S[254] === 0;
     S[255] === 0;
 

diff --git a/circuits/eddsamimc.circom b/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++) {
-        snum2bits.out[i] ==> compConstant.in[i];
+    for (i=0; i<251; 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);
-    for (i=0; i<253; i++) {
+    component mulFix = EscalarMulFix(251, BASE8);
+    for (i=0; i<251; i++) {
         mulFix.e[i] <== snum2bits.out[i];
     }
 

diff --git a/circuits/eddsamimcsponge.circom b/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++) {
-        snum2bits.out[i] ==> compConstant.in[i];
+    for (i=0; i<251; 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);
-    for (i=0; i<253; i++) {
+    component mulFix = EscalarMulFix(251, BASE8);
+    for (i=0; i<251; i++) {
         mulFix.e[i] <== snum2bits.out[i];
     }
 

diff --git a/circuits/eddsaposeidon.circom b/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++) {
-        snum2bits.out[i] ==> compConstant.in[i];
+    for (i=0; i<251; 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);
-    for (i=0; i<253; i++) {
+    component mulFix = EscalarMulFix(251, BASE8);
+    for (i=0; i<251; i++) {
         mulFix.e[i] <== snum2bits.out[i];
     }
 

diff --git a/circuits/escalarmulfix.circom b/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++) {

diff --git a/circuits/mimcsponge.circom b/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];
 

diff --git a/circuits/montgomery.circom b/circuits/montgomery.circom
@@ -85,6 +85,7 @@ template Montgomery2Edwards() {
 
  */
 
+/* in1 must be != in2 */
 template MontgomeryAdd() {
     signal input in1[2];
     signal input in2[2];

diff --git a/circuits/pedersen.circom b/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];