Merge pull request #138 from semaphore-protocol/feat/custom-nullifier-hashes

cedoor · web-flow · commit 20fc3c58d7e4 · 2022-09-19T16:49:19.000+02:00
Custom nullifier hashes Former-commit-id: 506fa0a
diff --git a/packages/contracts/contracts/Semaphore.sol b/packages/contracts/contracts/Semaphore.sol
@@ -11,16 +11,13 @@ contract Semaphore is ISemaphore, SemaphoreCore, SemaphoreGroups {
     /// @dev Gets a tree depth and returns its verifier address.
     mapping(uint256 => IVerifier) public verifiers;
 
-    /// @dev Gets a group id and returns the group admin address.
-    mapping(uint256 => address) public groupAdmins;
-
-    /// @dev Gets a group id and returns data to check if a Merkle root is expired.
-    mapping(uint256 => MerkleTreeExpiry) public merkleTreeExpiries;
+    /// @dev Gets a group id and returns the group parameters.
+    mapping(uint256 => Group) public groups;
 
     /// @dev Checks if the group admin is the transaction sender.
     /// @param groupId: Id of the group.
     modifier onlyGroupAdmin(uint256 groupId) {
-        if (groupAdmins[groupId] != _msgSender()) {
+        if (groups[groupId].admin != _msgSender()) {
             revert Semaphore__CallerIsNotTheGroupAdmin();
         }
         _;
@@ -56,8 +53,8 @@ contract Semaphore is ISemaphore, SemaphoreCore, SemaphoreGroups {
     ) external override onlySupportedMerkleTreeDepth(merkleTreeDepth) {
         _createGroup(groupId, merkleTreeDepth, zeroValue);
 
-        groupAdmins[groupId] = admin;
-        merkleTreeExpiries[groupId].rootDuration = 1 hours;
+        groups[groupId].admin = admin;
+        groups[groupId].merkleRootDuration = 1 hours;
 
         emit GroupAdminUpdated(groupId, address(0), admin);
     }
@@ -72,15 +69,15 @@ contract Semaphore is ISemaphore, SemaphoreCore, SemaphoreGroups {
     ) external override onlySupportedMerkleTreeDepth(merkleTreeDepth) {
         _createGroup(groupId, merkleTreeDepth, zeroValue);
 
-        groupAdmins[groupId] = admin;
-        merkleTreeExpiries[groupId].rootDuration = merkleTreeRootDuration;
+        groups[groupId].admin = admin;
+        groups[groupId].merkleRootDuration = merkleTreeRootDuration;
 
         emit GroupAdminUpdated(groupId, address(0), admin);
     }
 
     /// @dev See {ISemaphore-updateGroupAdmin}.
     function updateGroupAdmin(uint256 groupId, address newAdmin) external override onlyGroupAdmin(groupId) {
-        groupAdmins[groupId] = newAdmin;
+        groups[groupId].admin = newAdmin;
 
         emit GroupAdminUpdated(groupId, _msgSender(), newAdmin);
     }
@@ -91,7 +88,7 @@ contract Semaphore is ISemaphore, SemaphoreCore, SemaphoreGroups {
 
         uint256 merkleTreeRoot = getMerkleTreeRoot(groupId);
 
-        merkleTreeExpiries[groupId].rootCreationDates[merkleTreeRoot] = block.timestamp;
+        groups[groupId].merkleRootCreationDates[merkleTreeRoot] = block.timestamp;
     }
 
     /// @dev See {ISemaphore-addMembers}.
@@ -110,7 +107,7 @@ contract Semaphore is ISemaphore, SemaphoreCore, SemaphoreGroups {
 
         uint256 merkleTreeRoot = getMerkleTreeRoot(groupId);
 
-        merkleTreeExpiries[groupId].rootCreationDates[merkleTreeRoot] = block.timestamp;
+        groups[groupId].merkleRootCreationDates[merkleTreeRoot] = block.timestamp;
     }
 
     /// @dev See {ISemaphore-updateMember}.
@@ -150,25 +147,29 @@ contract Semaphore is ISemaphore, SemaphoreCore, SemaphoreGroups {
         }
 
         if (merkleTreeRoot != currentMerkleTreeRoot) {
-            uint256 rootCreationDate = merkleTreeExpiries[groupId].rootCreationDates[merkleTreeRoot];
-            uint256 rootDuration = merkleTreeExpiries[groupId].rootDuration;
+            uint256 merkleRootCreationDate = groups[groupId].merkleRootCreationDates[merkleTreeRoot];
+            uint256 merkleRootDuration = groups[groupId].merkleRootDuration;
 
-            if (rootCreationDate == 0) {
+            if (merkleRootCreationDate == 0) {
                 revert Semaphore__MerkleTreeRootIsNotPartOfTheGroup();
             }
 
-            if (block.timestamp > rootCreationDate + rootDuration) {
+            if (block.timestamp > merkleRootCreationDate + merkleRootDuration) {
                 revert Semaphore__MerkleTreeRootIsExpired();
             }
         }
 
+        if (groups[groupId].nullifierHashes[nullifierHash]) {
+            revert Semaphore__YouAreUsingTheSameNillifierTwice();
+        }
+
         uint256 merkleTreeDepth = getMerkleTreeDepth(groupId);
 
         IVerifier verifier = verifiers[merkleTreeDepth];
 
         _verifyProof(signal, merkleTreeRoot, nullifierHash, externalNullifier, proof, verifier);
 
-        _saveNullifierHash(uint256(keccak256(abi.encodePacked(groupId, nullifierHash))));
+        groups[groupId].nullifierHashes[nullifierHash] = true;
 
         emit ProofVerified(groupId, merkleTreeRoot, nullifierHash, externalNullifier, signal);
     }
diff --git a/packages/contracts/contracts/base/SemaphoreCore.sol b/packages/contracts/contracts/base/SemaphoreCore.sol
@@ -10,10 +10,6 @@ import "../interfaces/IVerifier.sol";
 /// nullifier to prevent double-signaling. External nullifier and Merkle trees (i.e. groups) must be
 /// managed externally.
 contract SemaphoreCore is ISemaphoreCore {
-    /// @dev Gets a nullifier hash and returns true or false.
-    /// It is used to prevent double-signaling.
-    mapping(uint256 => bool) internal nullifierHashes;
-
     /// @dev Asserts that no nullifier already exists and if the zero-knowledge proof is valid.
     /// Otherwise it reverts.
     /// @param signal: Semaphore signal.
@@ -30,10 +26,6 @@ contract SemaphoreCore is ISemaphoreCore {
         uint256[8] calldata proof,
         IVerifier verifier
     ) internal view {
-        if (nullifierHashes[nullifierHash]) {
-            revert Semaphore__YouAreUsingTheSameNillifierTwice();
-        }
-
         uint256 signalHash = _hashSignal(signal);
 
         verifier.verifyProof(
@@ -44,14 +36,6 @@ contract SemaphoreCore is ISemaphoreCore {
         );
     }
 
-    /// @dev Stores the nullifier hash to prevent double-signaling.
-    /// Attention! Remember to call it when you verify a proof if you
-    /// need to prevent double-signaling.
-    /// @param nullifierHash: Semaphore nullifier hash.
-    function _saveNullifierHash(uint256 nullifierHash) internal {
-        nullifierHashes[nullifierHash] = true;
-    }
-
     /// @dev Creates a keccak256 hash of the signal.
     /// @param signal: Semaphore signal.
     /// @return Hash of the signal.
diff --git a/packages/contracts/contracts/base/SemaphoreGroups.sol b/packages/contracts/contracts/base/SemaphoreGroups.sol
@@ -12,8 +12,8 @@ import "@openzeppelin/contracts/utils/Context.sol";
 abstract contract SemaphoreGroups is Context, ISemaphoreGroups {
     using IncrementalBinaryTree for IncrementalTreeData;
 
-    /// @dev Gets a group id and returns the group/tree data.
-    mapping(uint256 => IncrementalTreeData) internal groups;
+    /// @dev Gets a group id and returns the tree data.
+    mapping(uint256 => IncrementalTreeData) internal merkleTree;
 
     /// @dev Creates a new group by initializing the associated tree.
     /// @param groupId: Id of the group.
@@ -32,7 +32,7 @@ abstract contract SemaphoreGroups is Context, ISemaphoreGroups {
             revert Semaphore__GroupAlreadyExists();
         }
 
-        groups[groupId].init(merkleTreeDepth, zeroValue);
+        merkleTree[groupId].init(merkleTreeDepth, zeroValue);
 
         emit GroupCreated(groupId, merkleTreeDepth, zeroValue);
     }
@@ -45,7 +45,7 @@ abstract contract SemaphoreGroups is Context, ISemaphoreGroups {
             revert Semaphore__GroupDoesNotExist();
         }
 
-        groups[groupId].insert(identityCommitment);
+        merkleTree[groupId].insert(identityCommitment);
 
         uint256 merkleTreeRoot = getMerkleTreeRoot(groupId);
         uint256 index = getNumberOfMerkleTreeLeaves(groupId) - 1;
@@ -71,7 +71,7 @@ abstract contract SemaphoreGroups is Context, ISemaphoreGroups {
             revert Semaphore__GroupDoesNotExist();
         }
 
-        groups[groupId].update(identityCommitment, newIdentityCommitment, proofSiblings, proofPathIndices);
+        merkleTree[groupId].update(identityCommitment, newIdentityCommitment, proofSiblings, proofPathIndices);
 
         uint256 merkleTreeRoot = getMerkleTreeRoot(groupId);
         uint256 index = proofPathIndicesToMemberIndex(proofPathIndices);
@@ -95,7 +95,7 @@ abstract contract SemaphoreGroups is Context, ISemaphoreGroups {
             revert Semaphore__GroupDoesNotExist();
         }
 
-        groups[groupId].remove(identityCommitment, proofSiblings, proofPathIndices);
+        merkleTree[groupId].remove(identityCommitment, proofSiblings, proofPathIndices);
 
         uint256 merkleTreeRoot = getMerkleTreeRoot(groupId);
         uint256 index = proofPathIndicesToMemberIndex(proofPathIndices);
@@ -105,17 +105,17 @@ abstract contract SemaphoreGroups is Context, ISemaphoreGroups {
 
     /// @dev See {ISemaphoreGroups-getMerkleTreeRoot}.
     function getMerkleTreeRoot(uint256 groupId) public view virtual override returns (uint256) {
-        return groups[groupId].root;
+        return merkleTree[groupId].root;
     }
 
     /// @dev See {ISemaphoreGroups-getMerkleTreeDepth}.
     function getMerkleTreeDepth(uint256 groupId) public view virtual override returns (uint256) {
-        return groups[groupId].depth;
+        return merkleTree[groupId].depth;
     }
 
     /// @dev See {ISemaphoreGroups-getNumberOfMerkleTreeLeaves}.
     function getNumberOfMerkleTreeLeaves(uint256 groupId) public view virtual override returns (uint256) {
-        return groups[groupId].numberOfLeaves;
+        return merkleTree[groupId].numberOfLeaves;
     }
 
     /// @dev Converts the path indices of a Merkle proof to the identity commitment index in the tree.
diff --git a/packages/contracts/contracts/extensions/SemaphoreVoting.sol b/packages/contracts/contracts/extensions/SemaphoreVoting.sol
@@ -14,6 +14,10 @@ contract SemaphoreVoting is ISemaphoreVoting, SemaphoreCore, SemaphoreGroups {
     /// @dev Gets a poll id and returns the poll data.
     mapping(uint256 => Poll) internal polls;
 
+    /// @dev Gets a nullifier hash and returns true or false.
+    /// It is used to prevent double-voting.
+    mapping(uint256 => bool) internal nullifierHashes;
+
     /// @dev Initializes the Semaphore verifiers used to verify the user's ZK proofs.
     /// @param _verifiers: List of Semaphore verifiers (address and related Merkle tree depth).
     constructor(Verifier[] memory _verifiers) {
@@ -90,15 +94,18 @@ contract SemaphoreVoting is ISemaphoreVoting, SemaphoreCore, SemaphoreGroups {
             revert Semaphore__PollIsNotOngoing();
         }
 
+        if (nullifierHashes[nullifierHash]) {
+            revert Semaphore__YouAreUsingTheSameNillifierTwice();
+        }
+
         uint256 merkleTreeDepth = getMerkleTreeDepth(pollId);
         uint256 merkleTreeRoot = getMerkleTreeRoot(pollId);
 
         IVerifier verifier = verifiers[merkleTreeDepth];
 
         _verifyProof(vote, merkleTreeRoot, nullifierHash, pollId, proof, verifier);
 
-        // Prevent double-voting (nullifierHash = hash(pollId + identityNullifier)).
-        _saveNullifierHash(nullifierHash);
+        nullifierHashes[nullifierHash] = true;
 
         emit VoteAdded(pollId, vote);
     }
diff --git a/packages/contracts/contracts/interfaces/ISemaphore.sol b/packages/contracts/contracts/interfaces/ISemaphore.sol
@@ -8,17 +8,19 @@ interface ISemaphore {
     error Semaphore__MerkleTreeDepthIsNotSupported();
     error Semaphore__MerkleTreeRootIsExpired();
     error Semaphore__MerkleTreeRootIsNotPartOfTheGroup();
+    error Semaphore__YouAreUsingTheSameNillifierTwice();
 
     struct Verifier {
         address contractAddress;
         uint256 merkleTreeDepth;
     }
 
-    /// It defines all the parameters needed to check whether a
-    /// zero-knowledge proof generated with a certain Merkle tree is still valid.
-    struct MerkleTreeExpiry {
-        uint256 rootDuration;
-        mapping(uint256 => uint256) rootCreationDates;
+    /// It defines all the group parameters, in addition to those in the Merkle tree.
+    struct Group {
+        address admin;
+        uint256 merkleRootDuration;
+        mapping(uint256 => uint256) merkleRootCreationDates;
+        mapping(uint256 => bool) nullifierHashes;
     }
 
     /// @dev Emitted when an admin is assigned to a group.
diff --git a/packages/contracts/contracts/interfaces/ISemaphoreCore.sol b/packages/contracts/contracts/interfaces/ISemaphoreCore.sol
@@ -4,8 +4,6 @@ pragma solidity 0.8.4;
 /// @title SemaphoreCore interface.
 /// @dev Interface of SemaphoreCore contract.
 interface ISemaphoreCore {
-    error Semaphore__YouAreUsingTheSameNillifierTwice();
-
     /// @notice Emitted when a proof is verified correctly and a new nullifier hash is added.
     /// @param nullifierHash: Hash of external and identity nullifiers.
     event NullifierHashAdded(uint256 nullifierHash);
diff --git a/packages/contracts/contracts/interfaces/ISemaphoreVoting.sol b/packages/contracts/contracts/interfaces/ISemaphoreVoting.sol
@@ -8,6 +8,7 @@ interface ISemaphoreVoting {
     error Semaphore__MerkleTreeDepthIsNotSupported();
     error Semaphore__PollHasAlreadyBeenStarted();
     error Semaphore__PollIsNotOngoing();
+    error Semaphore__YouAreUsingTheSameNillifierTwice();
 
     enum PollState {
         Created,
diff --git a/packages/contracts/test/Semaphore.ts b/packages/contracts/test/Semaphore.ts
@@ -252,6 +252,19 @@ describe("Semaphore", () => {
                 )
         })
 
+        it("Should not verify the same proof for an onchain group twice", async () => {
+            const transaction = contract.verifyProof(
+                groupId,
+                group.root,
+                signal,
+                fullProof.publicSignals.nullifierHash,
+                fullProof.publicSignals.merkleRoot,
+                solidityProof
+            )
+
+            await expect(transaction).to.be.revertedWith("Semaphore__YouAreUsingTheSameNillifierTwice()")
+        })
+
         it("Should not verify a proof if the Merkle tree root is expired", async () => {
             const groupId = 2
             const group = new Group(treeDepth)
