diff --git a/packages/contracts/contracts/Semaphore.sol b/packages/contracts/contracts/Semaphore.sol index 07ca811e6..fccfa967f 100644 --- 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 index 94353d48c..e4e8da92c 100644 --- 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 index df9154477..415a67c82 100644 --- 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 index 4455d61e4..f0f7f0146 100644 --- 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,6 +94,10 @@ contract SemaphoreVoting is ISemaphoreVoting, SemaphoreCore, SemaphoreGroups { revert Semaphore__PollIsNotOngoing(); } + if (nullifierHashes[nullifierHash]) { + revert Semaphore__YouAreUsingTheSameNillifierTwice(); + } + uint256 merkleTreeDepth = getMerkleTreeDepth(pollId); uint256 merkleTreeRoot = getMerkleTreeRoot(pollId); @@ -97,8 +105,7 @@ contract SemaphoreVoting is ISemaphoreVoting, SemaphoreCore, SemaphoreGroups { _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 index ff1ab8a71..902413bd0 100644 --- 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 index 544432911..cac19d496 100644 --- 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 index b7f0cc54d..77a1d0193 100644 --- 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 index af8034824..80c622424 100644 --- 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)