Cleanup Verifier.sol and make error handling consistent

contracts/base/SemaphoreCore.sol

@@ -14,33 +14,32 @@ contract SemaphoreCore is ISemaphoreCore {
   /// It is used to prevent double-signaling.
   mapping(uint256 => bool) internal nullifierHashes;
 
-  /// @dev Returns true if no nullifier already exists and if the zero-knowledge proof is valid.
-  /// Otherwise it returns false.
+  /// @dev Asserts that no nullifier already exists and if the zero-knowledge proof is valid.
+  /// Otherwise it reverts.
   /// @param signal: Semaphore signal.
   /// @param root: Root of the Merkle tree.
   /// @param nullifierHash: Nullifier hash.
   /// @param externalNullifier: External nullifier.
   /// @param proof: Zero-knowledge proof.
   /// @param verifier: Verifier address.
-  function _isValidProof(
+  function _verifyProof(
     bytes32 signal,
     uint256 root,
     uint256 nullifierHash,
     uint256 externalNullifier,
     uint256[8] calldata proof,
     IVerifier verifier
-  ) internal view returns (bool) {
+  ) internal view {
     require(!nullifierHashes[nullifierHash], "SemaphoreCore: you cannot use the same nullifier twice");
 
     uint256 signalHash = _hashSignal(signal);
 
-    return
-      verifier.verifyProof(
-        [proof[0], proof[1]],
-        [[proof[2], proof[3]], [proof[4], proof[5]]],
-        [proof[6], proof[7]],
-        [root, nullifierHash, signalHash, externalNullifier]
-      );
+    verifier.verifyProof(
+      [proof[0], proof[1]],
+      [[proof[2], proof[3]], [proof[4], proof[5]]],
+      [proof[6], proof[7]],
+      [root, nullifierHash, signalHash, externalNullifier]
+    );
   }
 
   /// @dev Stores the nullifier hash to prevent double-signaling.

contracts/base/Verifier.sol

@@ -9,15 +9,29 @@
 //      fixed linter warnings
 //      added requiere error messages
 //
+// 2021 Remco Bloemen
+//       cleaned up code
+//       added InvalidProve() error
+//       always revert with InvalidProof() on invalid proof
+//       make Pairing strict
 //
 // SPDX-License-Identifier: GPL-3.0
 pragma solidity ^0.8.4;
 
 library Pairing {
+  error InvalidProof();
+
+  // The prime q in the base field F_q for G1
+  uint256 constant BASE_MODULUS = 21888242871839275222246405745257275088696311157297823662689037894645226208583;
+
+  // The prime moludus of the scalar field of G1.
+  uint256 constant SCALAR_MODULUS = 21888242871839275222246405745257275088548364400416034343698204186575808495617;
+
   struct G1Point {
     uint256 X;
     uint256 Y;
   }
+
   // Encoding of field elements is: X[0] * z + X[1]
   struct G2Point {
     uint256[2] X;
@@ -31,7 +45,6 @@ library Pairing {
 
   /// @return the generator of G2
   function P2() internal pure returns (G2Point memory) {
-    // Original code point
     return
       G2Point(
         [
@@ -43,28 +56,21 @@ library Pairing {
           8495653923123431417604973247489272438418190587263600148770280649306958101930
         ]
       );
-
-    /*
-        // Changed by Jordi point
-        return G2Point(
-            [10857046999023057135944570762232829481370756359578518086990519993285655852781,
-             11559732032986387107991004021392285783925812861821192530917403151452391805634],
-            [8495653923123431417604973247489272438418190587263600148770280649306958101930,
-             4082367875863433681332203403145435568316851327593401208105741076214120093531]
-        );
-*/
   }
 
   /// @return r the negation of p, i.e. p.addition(p.negate()) should be zero.
   function negate(G1Point memory p) internal pure returns (G1Point memory r) {
-    // The prime q in the base field F_q for G1
-    uint256 q = 21888242871839275222246405745257275088696311157297823662689037894645226208583;
     if (p.X == 0 && p.Y == 0) return G1Point(0, 0);
-    return G1Point(p.X, q - (p.Y % q));
+    // Validate input or revert
+    if (p.X >= BASE_MODULUS || p.Y >= BASE_MODULUS) revert InvalidProof();
+    // We know p.Y > 0 and p.Y < BASE_MODULUS.
+    return G1Point(p.X, BASE_MODULUS - p.Y);
   }
 
   /// @return r the sum of two points of G1
   function addition(G1Point memory p1, G1Point memory p2) internal view returns (G1Point memory r) {
+    // By EIP-196 all input is validated to be less than the BASE_MODULUS and form points
+    // on the curve.
     uint256[4] memory input;
     input[0] = p1.X;
     input[1] = p1.Y;
@@ -74,18 +80,16 @@ library Pairing {
     // solium-disable-next-line security/no-inline-assembly
     assembly {
       success := staticcall(sub(gas(), 2000), 6, input, 0xc0, r, 0x60)
-      // Use "invalid" to make gas estimation work
-      switch success
-      case 0 {
-        invalid()
-      }
     }
-    require(success, "pairing-add-failed");
+    if (!success) revert InvalidProof();
   }
 
   /// @return r the product of a point on G1 and a scalar, i.e.
   /// p == p.scalar_mul(1) and p.addition(p) == p.scalar_mul(2) for all points p.
   function scalar_mul(G1Point memory p, uint256 s) internal view returns (G1Point memory r) {
+    // By EIP-196 the values p.X and p.Y are verified to less than the BASE_MODULUS and
+    // form a valid point on the curve. But the scalar is not verified, so we do that explicitelly.
+    if (s >= SCALAR_MODULUS) revert InvalidProof();
     uint256[3] memory input;
     input[0] = p.X;
     input[1] = p.Y;
@@ -94,21 +98,17 @@ library Pairing {
     // solium-disable-next-line security/no-inline-assembly
     assembly {
       success := staticcall(sub(gas(), 2000), 7, input, 0x80, r, 0x60)
-      // Use "invalid" to make gas estimation work
-      switch success
-      case 0 {
-        invalid()
-      }
     }
-    require(success, "pairing-mul-failed");
+    if (!success) revert InvalidProof();
   }
 
-  /// @return the result of computing the pairing check
+  /// Asserts the pairing check
   /// e(p1[0], p2[0]) *  .... * e(p1[n], p2[n]) == 1
-  /// For example pairing([P1(), P1().negate()], [P2(), P2()]) should
-  /// return true.
-  function pairing(G1Point[] memory p1, G2Point[] memory p2) internal view returns (bool) {
-    require(p1.length == p2.length, "pairing-lengths-failed");
+  /// For example pairing([P1(), P1().negate()], [P2(), P2()]) should succeed
+  function pairingCheck(G1Point[] memory p1, G2Point[] memory p2) internal view {
+    // By EIP-197 all input is verified to be less than the BASE_MODULUS and form elements in their
+    // respective groups of the right order.
+    if (p1.length != p2.length) revert InvalidProof();
     uint256 elements = p1.length;
     uint256 inputSize = elements * 6;
     uint256[] memory input = new uint256[](inputSize);
@@ -125,86 +125,22 @@ library Pairing {
     // solium-disable-next-line security/no-inline-assembly
     assembly {
       success := staticcall(sub(gas(), 2000), 8, add(input, 0x20), mul(inputSize, 0x20), out, 0x20)
-      // Use "invalid" to make gas estimation work
-      switch success
-      case 0 {
-        invalid()
-      }
     }
-    require(success, "pairing-opcode-failed");
-    return out[0] != 0;
-  }
-
-  /// Convenience method for a pairing check for two pairs.
-  function pairingProd2(
-    G1Point memory a1,
-    G2Point memory a2,
-    G1Point memory b1,
-    G2Point memory b2
-  ) internal view returns (bool) {
-    G1Point[] memory p1 = new G1Point[](2);
-    G2Point[] memory p2 = new G2Point[](2);
-    p1[0] = a1;
-    p1[1] = b1;
-    p2[0] = a2;
-    p2[1] = b2;
-    return pairing(p1, p2);
-  }
-
-  /// Convenience method for a pairing check for three pairs.
-  function pairingProd3(
-    G1Point memory a1,
-    G2Point memory a2,
-    G1Point memory b1,
-    G2Point memory b2,
-    G1Point memory c1,
-    G2Point memory c2
-  ) internal view returns (bool) {
-    G1Point[] memory p1 = new G1Point[](3);
-    G2Point[] memory p2 = new G2Point[](3);
-    p1[0] = a1;
-    p1[1] = b1;
-    p1[2] = c1;
-    p2[0] = a2;
-    p2[1] = b2;
-    p2[2] = c2;
-    return pairing(p1, p2);
-  }
-
-  /// Convenience method for a pairing check for four pairs.
-  function pairingProd4(
-    G1Point memory a1,
-    G2Point memory a2,
-    G1Point memory b1,
-    G2Point memory b2,
-    G1Point memory c1,
-    G2Point memory c2,
-    G1Point memory d1,
-    G2Point memory d2
-  ) internal view returns (bool) {
-    G1Point[] memory p1 = new G1Point[](4);
-    G2Point[] memory p2 = new G2Point[](4);
-    p1[0] = a1;
-    p1[1] = b1;
-    p1[2] = c1;
-    p1[3] = d1;
-    p2[0] = a2;
-    p2[1] = b2;
-    p2[2] = c2;
-    p2[3] = d2;
-    return pairing(p1, p2);
+    if (!success || out[0] != 1) revert InvalidProof();
   }
 }
 
 contract Verifier {
   using Pairing for *;
+
   struct VerifyingKey {
     Pairing.G1Point alfa1;
     Pairing.G2Point beta2;
     Pairing.G2Point gamma2;
     Pairing.G2Point delta2;
     Pairing.G1Point[] IC;
   }
+
   struct Proof {
     Pairing.G1Point A;
     Pairing.G2Point B;
@@ -275,42 +211,40 @@ contract Verifier {
     );
   }
 
-  function verify(uint256[] memory input, Proof memory proof) internal view returns (uint256) {
-    uint256 snark_scalar_field = 21888242871839275222246405745257275088548364400416034343698204186575808495617;
-    VerifyingKey memory vk = verifyingKey();
-    require(input.length + 1 == vk.IC.length, "verifier-bad-input");
-    // Compute the linear combination vk_x
-    Pairing.G1Point memory vk_x = Pairing.G1Point(0, 0);
-    for (uint256 i = 0; i < input.length; i++) {
-      require(input[i] < snark_scalar_field, "verifier-gte-snark-scalar-field");
-      vk_x = Pairing.addition(vk_x, Pairing.scalar_mul(vk.IC[i + 1], input[i]));
-    }
-    vk_x = Pairing.addition(vk_x, vk.IC[0]);
-    if (
-      !Pairing.pairingProd4(Pairing.negate(proof.A), proof.B, vk.alfa1, vk.beta2, vk_x, vk.gamma2, proof.C, vk.delta2)
-    ) return 1;
-    return 0;
-  }
-
-  /// @return r  bool true if proof is valid
+  /// @dev Verifies a Semaphore proof. Reverts with InvalidProof if the proof is invalid.
   function verifyProof(
     uint256[2] memory a,
     uint256[2][2] memory b,
     uint256[2] memory c,
     uint256[4] memory input
-  ) public view returns (bool r) {
+  ) public view {
+    // If the values are not in the correct range, the Pairing contract will revert.
     Proof memory proof;
     proof.A = Pairing.G1Point(a[0], a[1]);
     proof.B = Pairing.G2Point([b[0][0], b[0][1]], [b[1][0], b[1][1]]);
     proof.C = Pairing.G1Point(c[0], c[1]);
-    uint256[] memory inputValues = new uint256[](input.length);
-    for (uint256 i = 0; i < input.length; i++) {
-      inputValues[i] = input[i];
-    }
-    if (verify(inputValues, proof) == 0) {
-      return true;
-    } else {
-      return false;
-    }
+
+    VerifyingKey memory vk = verifyingKey();
+
+    // Compute the linear combination vk_x of inputs times IC
+    if (input.length + 1 != vk.IC.length) revert Pairing.InvalidProof();
+    Pairing.G1Point memory vk_x = vk.IC[0];
+    vk_x = Pairing.addition(vk_x, Pairing.scalar_mul(vk.IC[1], input[0]));
+    vk_x = Pairing.addition(vk_x, Pairing.scalar_mul(vk.IC[2], input[1]));
+    vk_x = Pairing.addition(vk_x, Pairing.scalar_mul(vk.IC[3], input[2]));
+    vk_x = Pairing.addition(vk_x, Pairing.scalar_mul(vk.IC[4], input[3]));
+
+    // Check pairing
+    Pairing.G1Point[] memory p1 = new Pairing.G1Point[](4);
+    Pairing.G2Point[] memory p2 = new Pairing.G2Point[](4);
+    p1[0] = Pairing.negate(proof.A);
+    p2[0] = proof.B;
+    p1[1] = vk.alfa1;
+    p2[1] = vk.beta2;
+    p1[2] = vk_x;
+    p2[2] = vk.gamma2;
+    p1[3] = proof.C;
+    p2[3] = vk.delta2;
+    Pairing.pairingCheck(p1, p2);
   }
 }

contracts/extensions/SemaphoreVoting.sol

@@ -88,10 +88,7 @@ contract SemaphoreVoting is ISemaphoreVoting, SemaphoreCore, SemaphoreGroups {
     uint256 root = getRoot(pollId);
     IVerifier verifier = verifiers[depth];
 
-    require(
-      _isValidProof(vote, root, nullifierHash, pollId, proof, verifier),
-      "SemaphoreVoting: the proof is not valid"
-    );
+    _verifyProof(vote, root, nullifierHash, pollId, proof, verifier);
 
     // Prevent double-voting (nullifierHash = hash(pollId + identityNullifier)).
     _saveNullifierHash(nullifierHash);

contracts/extensions/SemaphoreWhistleblowing.sol

@@ -81,10 +81,7 @@ contract SemaphoreWhistleblowing is ISemaphoreWhistleblowing, SemaphoreCore, Sem
     uint256 root = getRoot(entityId);
     IVerifier verifier = verifiers[depth];
 
-    require(
-      _isValidProof(leak, root, nullifierHash, entityId, proof, verifier),
-      "SemaphoreWhistleblowing: the proof is not valid"
-    );
+    _verifyProof(leak, root, nullifierHash, entityId, proof, verifier);
 
     emit LeakPublished(entityId, leak);
   }

contracts/interfaces/IVerifier.sol

@@ -9,5 +9,5 @@ interface IVerifier {
     uint256[2][2] memory b,
     uint256[2] memory c,
     uint256[4] memory input
-  ) external view returns (bool);
+  ) external view;
 }

test/SemaphoreVoting.ts

@@ -163,7 +163,7 @@ describe("SemaphoreVoting", () => {
 
       const transaction = contract.connect(accounts[1]).castVote(bytes32Vote, nullifierHash, pollIds[1], solidityProof)
 
-      await expect(transaction).to.be.revertedWith("SemaphoreVoting: the proof is not valid")
+      await expect(transaction).to.be.revertedWith("InvalidProof()")
     })
 
     it("Should cast a vote", async () => {

test/SemaphoreWhistleblowing.ts

@@ -158,7 +158,7 @@ describe("SemaphoreWhistleblowing", () => {
         .connect(accounts[1])
         .publishLeak(bytes32Leak, nullifierHash, entityIds[1], solidityProof)
 
-      await expect(transaction).to.be.revertedWith("SemaphoreWhistleblowing: the proof is not valid")
+      await expect(transaction).to.be.revertedWith("InvalidProof()")
     })
 
     it("Should publish a leak", async () => {