From 2b05a94e15edcdbef9bee1c9495054509fde770b Mon Sep 17 00:00:00 2001 From: cedoor Date: Fri, 15 Dec 2023 14:20:21 +0100 Subject: [PATCH] refactor(contracts): move access logic to group contract --- packages/contracts/contracts/Semaphore.sol | 78 ++++------------- .../contracts/base/SemaphoreGroups.sol | 79 +++++++++++++++-- .../contracts/interfaces/ISemaphore.sol | 85 ++++++------------- .../contracts/interfaces/ISemaphoreGroups.sol | 14 +++ packages/contracts/contracts/package.json | 1 - packages/contracts/package.json | 1 - yarn.lock | 9 -- 7 files changed, 128 insertions(+), 139 deletions(-) diff --git a/packages/contracts/contracts/Semaphore.sol b/packages/contracts/contracts/Semaphore.sol index 9008a8096..118afce11 100644 --- a/packages/contracts/contracts/Semaphore.sol +++ b/packages/contracts/contracts/Semaphore.sol @@ -18,65 +18,29 @@ contract Semaphore is ISemaphore, SemaphoreGroups { /// @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 (groups[groupId].admin != _msgSender()) { - revert Semaphore__CallerIsNotTheGroupAdmin(); - } - _; - } - - /// @dev Checks if the group exists. - /// @param groupId: Id of the group. - modifier onlyExistingGroup(uint256 groupId) { - if (groups[groupId].admin == address(0)) { - revert Semaphore__GroupDoesNotExist(); - } - - _; - } - /// @dev Initializes the Semaphore verifier used to verify the user's ZK proofs. /// @param _verifier: Semaphore verifier address. constructor(ISemaphoreVerifier _verifier) { verifier = _verifier; } - /// @dev See {ISemaphore-createGroup}. + /// @dev See {SemaphoreGroups-_createGroup}. function createGroup(uint256 groupId, address admin) external override { - if (groups[groupId].admin != address(0)) { - revert Semaphore__GroupAlreadyExists(); - } + _createGroup(groupId, admin); - groups[groupId].admin = admin; groups[groupId].merkleTreeDuration = 1 hours; - - emit GroupCreated(groupId); - emit GroupAdminUpdated(groupId, address(0), admin); } /// @dev See {ISemaphore-createGroup}. function createGroup(uint256 groupId, address admin, uint256 merkleTreeDuration) external override { - if (groups[groupId].admin != address(0)) { - revert Semaphore__GroupAlreadyExists(); - } + _createGroup(groupId, admin); - groups[groupId].admin = admin; groups[groupId].merkleTreeDuration = merkleTreeDuration; - - emit GroupCreated(groupId); - emit GroupAdminUpdated(groupId, address(0), admin); } - /// @dev See {ISemaphore-updateGroupAdmin}. - function updateGroupAdmin( - uint256 groupId, - address newAdmin - ) external override onlyExistingGroup(groupId) onlyGroupAdmin(groupId) { - groups[groupId].admin = newAdmin; - - emit GroupAdminUpdated(groupId, _msgSender(), newAdmin); + /// @dev See {SemaphoreGroups-_updateGroupAdmin}. + function updateGroupAdmin(uint256 groupId, address newAdmin) external override { + _updateGroupAdmin(groupId, newAdmin); } /// @dev See {ISemaphore-updateGroupMerkleTreeDuration}. @@ -91,11 +55,8 @@ contract Semaphore is ISemaphore, SemaphoreGroups { emit GroupMerkleTreeDurationUpdated(groupId, oldMerkleTreeDuration, newMerkleTreeDuration); } - /// @dev See {ISemaphore-addMember}. - function addMember( - uint256 groupId, - uint256 identityCommitment - ) external override onlyExistingGroup(groupId) onlyGroupAdmin(groupId) { + /// @dev See {SemaphoreGroups-_addMember}. + function addMember(uint256 groupId, uint256 identityCommitment) external override { _addMember(groupId, identityCommitment); uint256 merkleTreeRoot = getMerkleTreeRoot(groupId); @@ -103,31 +64,22 @@ contract Semaphore is ISemaphore, SemaphoreGroups { groups[groupId].merkleRootCreationDates[merkleTreeRoot] = block.timestamp; } - /// @dev See {ISemaphore-addMembers}. - function addMembers( - uint256 groupId, - uint256[] calldata identityCommitments - ) external override onlyExistingGroup(groupId) onlyGroupAdmin(groupId) { - for (uint256 i = 0; i < identityCommitments.length; ) { - _addMember(groupId, identityCommitments[i]); - - unchecked { - ++i; - } - } + /// @dev See {SemaphoreGroups-_addMembers}. + function addMembers(uint256 groupId, uint256[] calldata identityCommitments) external override { + _addMembers(groupId, identityCommitments); uint256 merkleTreeRoot = getMerkleTreeRoot(groupId); groups[groupId].merkleRootCreationDates[merkleTreeRoot] = block.timestamp; } - /// @dev See {ISemaphore-updateMember}. + /// @dev See {SemaphoreGroups-_updateMember}. function updateMember( uint256 groupId, uint256 identityCommitment, uint256 newIdentityCommitment, uint256[] calldata merkleProofSiblings - ) external override onlyExistingGroup(groupId) onlyGroupAdmin(groupId) { + ) external override { _updateMember(groupId, identityCommitment, newIdentityCommitment, merkleProofSiblings); uint256 merkleTreeRoot = getMerkleTreeRoot(groupId); @@ -135,12 +87,12 @@ contract Semaphore is ISemaphore, SemaphoreGroups { groups[groupId].merkleRootCreationDates[merkleTreeRoot] = block.timestamp; } - /// @dev See {ISemaphore-removeMember}. + /// @dev See {SemaphoreGroups-_removeMember}. function removeMember( uint256 groupId, uint256 identityCommitment, uint256[] calldata merkleProofSiblings - ) external override onlyExistingGroup(groupId) onlyGroupAdmin(groupId) { + ) external override { _removeMember(groupId, identityCommitment, merkleProofSiblings); uint256 merkleTreeRoot = getMerkleTreeRoot(groupId); diff --git a/packages/contracts/contracts/base/SemaphoreGroups.sol b/packages/contracts/contracts/base/SemaphoreGroups.sol index 07defa544..f4a848522 100644 --- a/packages/contracts/contracts/base/SemaphoreGroups.sol +++ b/packages/contracts/contracts/base/SemaphoreGroups.sol @@ -3,27 +3,90 @@ pragma solidity 0.8.4; import "../interfaces/ISemaphoreGroups.sol"; import {InternalLeanIMT, LeanIMTData} from "@zk-kit/imt.sol/internal/InternalLeanIMT.sol"; -import "@openzeppelin/contracts/utils/Context.sol"; /// @title Semaphore groups contract. /// @dev This contract allows you to create groups, add, remove and update members. /// You can use getters to obtain informations about groups (root, depth, number of leaves). -abstract contract SemaphoreGroups is Context, ISemaphoreGroups { +abstract contract SemaphoreGroups is ISemaphoreGroups { using InternalLeanIMT for LeanIMTData; - /// @dev Gets a group id and returns the tree data. + /// @dev Gets a group id and returns its tree data. mapping(uint256 => LeanIMTData) internal merkleTrees; + /// @dev Gets a group id and returns its admin. + mapping(uint256 => address) internal admins; + + /// @dev Checks if the group admin is the transaction sender. + /// @param groupId: Id of the group. + modifier onlyGroupAdmin(uint256 groupId) { + if (admins[groupId] != msg.sender) { + revert Semaphore__CallerIsNotTheGroupAdmin(); + } + _; + } + + /// @dev Checks if the group exists. + /// @param groupId: Id of the group. + modifier onlyExistingGroup(uint256 groupId) { + if (admins[groupId] == address(0)) { + revert Semaphore__GroupDoesNotExist(); + } + + _; + } + + /// @dev Creates a new group. Only the admin will be able to add or remove members. + /// @param groupId: Id of the group. + /// @param admin: Admin of the group. + function _createGroup(uint256 groupId, address admin) internal virtual { + if (admins[groupId] != address(0)) { + revert Semaphore__GroupAlreadyExists(); + } + + admins[groupId] = admin; + + emit GroupCreated(groupId); + emit GroupAdminUpdated(groupId, address(0), admin); + } + + /// @dev Updates the group admin. + /// @param groupId: Id of the group. + /// @param newAdmin: New admin of the group. + function _updateGroupAdmin( + uint256 groupId, + address newAdmin + ) internal virtual onlyExistingGroup(groupId) onlyGroupAdmin(groupId) { + admins[groupId] = newAdmin; + + emit GroupAdminUpdated(groupId, msg.sender, newAdmin); + } + /// @dev Adds an identity commitment to an existing group. /// @param groupId: Id of the group. /// @param identityCommitment: New identity commitment. - function _addMember(uint256 groupId, uint256 identityCommitment) internal virtual { + function _addMember( + uint256 groupId, + uint256 identityCommitment + ) internal virtual onlyExistingGroup(groupId) onlyGroupAdmin(groupId) { uint256 merkleTreeRoot = merkleTrees[groupId]._insert(identityCommitment); uint256 leafIndex = getMerkleTreeSize(groupId) - 1; emit MemberAdded(groupId, leafIndex, identityCommitment, merkleTreeRoot); } + /// @dev Adds new members to an existing group. + /// @param groupId: Id of the group. + /// @param identityCommitments: New identity commitments. + function _addMembers(uint256 groupId, uint256[] calldata identityCommitments) internal virtual { + for (uint256 i = 0; i < identityCommitments.length; ) { + _addMember(groupId, identityCommitments[i]); + + unchecked { + ++i; + } + } + } + /// @dev Updates an identity commitment of an existing group. A proof of membership is /// needed to check if the node to be updated is part of the tree. /// @param groupId: Id of the group. @@ -35,13 +98,14 @@ abstract contract SemaphoreGroups is Context, ISemaphoreGroups { uint256 oldIdentityCommitment, uint256 newIdentityCommitment, uint256[] calldata merkleProofSiblings - ) internal virtual { + ) internal virtual onlyExistingGroup(groupId) onlyGroupAdmin(groupId) { + uint256 leafIndex = merkleTrees[groupId]._indexOf(oldIdentityCommitment); + uint256 merkleTreeRoot = merkleTrees[groupId]._update( oldIdentityCommitment, newIdentityCommitment, merkleProofSiblings ); - uint256 leafIndex = merkleTrees[groupId]._indexOf(newIdentityCommitment); emit MemberUpdated(groupId, leafIndex, oldIdentityCommitment, newIdentityCommitment, merkleTreeRoot); } @@ -55,8 +119,9 @@ abstract contract SemaphoreGroups is Context, ISemaphoreGroups { uint256 groupId, uint256 identityCommitment, uint256[] calldata merkleProofSiblings - ) internal virtual { + ) internal virtual onlyExistingGroup(groupId) onlyGroupAdmin(groupId) { uint256 leafIndex = merkleTrees[groupId]._indexOf(identityCommitment); + uint256 merkleTreeRoot = merkleTrees[groupId]._remove(identityCommitment, merkleProofSiblings); emit MemberRemoved(groupId, leafIndex, identityCommitment, merkleTreeRoot); diff --git a/packages/contracts/contracts/interfaces/ISemaphore.sol b/packages/contracts/contracts/interfaces/ISemaphore.sol index f3a2f5e02..6ffd16155 100644 --- a/packages/contracts/contracts/interfaces/ISemaphore.sol +++ b/packages/contracts/contracts/interfaces/ISemaphore.sol @@ -3,34 +3,20 @@ pragma solidity 0.8.4; /// @title Semaphore contract interface. interface ISemaphore { - error Semaphore__GroupDoesNotExist(); - error Semaphore__GroupAlreadyExists(); error Semaphore__GroupHasNoMembers(); - error Semaphore__CallerIsNotTheGroupAdmin(); error Semaphore__MerkleTreeDepthIsNotSupported(); error Semaphore__MerkleTreeRootIsExpired(); error Semaphore__MerkleTreeRootIsNotPartOfTheGroup(); error Semaphore__YouAreUsingTheSameNillifierTwice(); error Semaphore__InvalidProof(); - /// It defines all the group parameters, in addition to those in the Merkle tree. + /// It defines all the group parameters used by Semaphore.sol. struct Group { - address admin; uint256 merkleTreeDuration; mapping(uint256 => uint256) merkleRootCreationDates; mapping(uint256 => bool) nullifiers; } - /// @dev Emitted when a new group is created. - /// @param groupId: Id of the group. - event GroupCreated(uint256 indexed groupId); - - /// @dev Emitted when an admin is assigned to a group. - /// @param groupId: Id of the group. - /// @param oldAdmin: Old admin of the group. - /// @param newAdmin: New admin of the group. - event GroupAdminUpdated(uint256 indexed groupId, address indexed oldAdmin, address indexed newAdmin); - /// @dev Emitted when the Merkle tree duration of a group is updated. /// @param groupId: Id of the group. /// @param oldMerkleTreeDuration: Old Merkle tree duration of the group. @@ -57,37 +43,16 @@ interface ISemaphore { uint256[8] proof ); - /// @dev Saves the nullifier hash to avoid double signaling and emits an event - /// if the zero-knowledge proof is valid. - /// @param groupId: Id of the group. - /// @param merkleTreeRoot: Root of the Merkle tree. - /// @param nullifier: Nullifier. - /// @param message: Semaphore message. - /// @param scope: Scope. - /// @param proof: Zero-knowledge proof. - function verifyProof( - uint256 groupId, - uint256 merkleTreeRoot, - uint256 nullifier, - uint256 message, - uint256 scope, - uint256[8] calldata proof - ) external; - - /// @dev Creates a new group. Only the admin will be able to add or remove members. - /// @param groupId: Id of the group. - /// @param admin: Admin of the group. + /// @dev See {SemaphoreGroups-_createGroup}. function createGroup(uint256 groupId, address admin) external; - /// @dev Creates a new group. Only the admin will be able to add or remove members. + /// @dev It creates a group with a custom Merkle tree duration. /// @param groupId: Id of the group. /// @param admin: Admin of the group. - /// @param merkleTreeRootDuration: Time before the validity of a root expires. - function createGroup(uint256 groupId, address admin, uint256 merkleTreeRootDuration) external; + /// @param merkleTreeDuration: Merkle tree duration. + function createGroup(uint256 groupId, address admin, uint256 merkleTreeDuration) external; - /// @dev Updates the group admin. - /// @param groupId: Id of the group. - /// @param newAdmin: New admin of the group. + /// @dev See {SemaphoreGroups-_updateGroupAdmin}. function updateGroupAdmin(uint256 groupId, address newAdmin) external; /// @dev Updates the group Merkle tree duration. @@ -95,22 +60,13 @@ interface ISemaphore { /// @param newMerkleTreeDuration: New Merkle tree duration. function updateGroupMerkleTreeDuration(uint256 groupId, uint256 newMerkleTreeDuration) external; - /// @dev Adds a new member to an existing group. - /// @param groupId: Id of the group. - /// @param identityCommitment: New identity commitment. + /// @dev See {SemaphoreGroups-_addMember}. function addMember(uint256 groupId, uint256 identityCommitment) external; - /// @dev Adds new members to an existing group. - /// @param groupId: Id of the group. - /// @param identityCommitments: New identity commitments. + /// @dev See {SemaphoreGroups-_addMembers}. function addMembers(uint256 groupId, uint256[] calldata identityCommitments) external; - /// @dev Updates an identity commitment of an existing group. A proof of membership is - /// needed to check if the node to be updated is part of the tree. - /// @param groupId: Id of the group. - /// @param oldIdentityCommitment: Existing identity commitment to be updated. - /// @param newIdentityCommitment: New identity commitment. - /// @param merkleProofSiblings: Array of the sibling nodes of the proof of membership. + /// @dev See {SemaphoreGroups-_updateMember}. function updateMember( uint256 groupId, uint256 oldIdentityCommitment, @@ -118,10 +74,23 @@ interface ISemaphore { uint256[] calldata merkleProofSiblings ) external; - /// @dev Removes a member from an existing group. A proof of membership is - /// needed to check if the node to be removed is part of the tree. - /// @param groupId: Id of the group. - /// @param identityCommitment: Identity commitment to be removed. - /// @param merkleProofSiblings: Array of the sibling nodes of the proof of membership. + /// @dev See {SemaphoreGroups-_removeMember}. function removeMember(uint256 groupId, uint256 identityCommitment, uint256[] calldata merkleProofSiblings) external; + + /// @dev Saves the nullifier hash to avoid double signaling and emits an event + /// if the zero-knowledge proof is valid. + /// @param groupId: Id of the group. + /// @param merkleTreeRoot: Root of the Merkle tree. + /// @param nullifier: Nullifier. + /// @param message: Semaphore message. + /// @param scope: Scope. + /// @param proof: Zero-knowledge proof. + function verifyProof( + uint256 groupId, + uint256 merkleTreeRoot, + uint256 nullifier, + uint256 message, + uint256 scope, + uint256[8] calldata proof + ) external; } diff --git a/packages/contracts/contracts/interfaces/ISemaphoreGroups.sol b/packages/contracts/contracts/interfaces/ISemaphoreGroups.sol index eef6b50b5..32ac9785b 100644 --- a/packages/contracts/contracts/interfaces/ISemaphoreGroups.sol +++ b/packages/contracts/contracts/interfaces/ISemaphoreGroups.sol @@ -3,6 +3,20 @@ pragma solidity 0.8.4; /// @title SemaphoreGroups contract interface. interface ISemaphoreGroups { + error Semaphore__GroupDoesNotExist(); + error Semaphore__GroupAlreadyExists(); + error Semaphore__CallerIsNotTheGroupAdmin(); + + /// @dev Emitted when a new group is created. + /// @param groupId: Id of the group. + event GroupCreated(uint256 indexed groupId); + + /// @dev Emitted when an admin is assigned to a group. + /// @param groupId: Id of the group. + /// @param oldAdmin: Old admin of the group. + /// @param newAdmin: New admin of the group. + event GroupAdminUpdated(uint256 indexed groupId, address indexed oldAdmin, address indexed newAdmin); + /// @dev Emitted when a new identity commitment is added. /// @param groupId: Group id of the group. /// @param leafIndex: Merkle tree leaf index. diff --git a/packages/contracts/contracts/package.json b/packages/contracts/contracts/package.json index c41eeae19..d4173b221 100644 --- a/packages/contracts/contracts/package.json +++ b/packages/contracts/contracts/package.json @@ -30,7 +30,6 @@ "access": "public" }, "dependencies": { - "@openzeppelin/contracts": "4.7.3", "@zk-kit/imt.sol": "2.0.0-beta.3" } } diff --git a/packages/contracts/package.json b/packages/contracts/package.json index 162a2e5a6..9f5ca860d 100644 --- a/packages/contracts/package.json +++ b/packages/contracts/package.json @@ -59,7 +59,6 @@ } }, "dependencies": { - "@openzeppelin/contracts": "4.7.3", "@zk-kit/imt.sol": "2.0.0-beta.3" } } diff --git a/yarn.lock b/yarn.lock index 4a8319f27..33ec8b017 100644 --- a/yarn.lock +++ b/yarn.lock @@ -7797,13 +7797,6 @@ __metadata: languageName: node linkType: hard -"@openzeppelin/contracts@npm:4.7.3": - version: 4.7.3 - resolution: "@openzeppelin/contracts@npm:4.7.3" - checksum: 18382fcacf7cfd652f5dd0e70c08f08ea74eaa8ff11e9f9850639ada70198ae01a3f9493d89a52d724f2db394e9616bf6258017804612ba273167cf657fbb073 - languageName: node - linkType: hard - "@peculiar/asn1-android@npm:^2.3.3": version: 2.3.6 resolution: "@peculiar/asn1-android@npm:2.3.6" @@ -8422,7 +8415,6 @@ __metadata: version: 0.0.0-use.local resolution: "@semaphore-protocol/contracts@workspace:packages/contracts/contracts" dependencies: - "@openzeppelin/contracts": 4.7.3 "@zk-kit/imt.sol": 2.0.0-beta.3 languageName: unknown linkType: soft @@ -28202,7 +28194,6 @@ __metadata: "@nomicfoundation/hardhat-chai-matchers": ^1.0.5 "@nomiclabs/hardhat-ethers": ^2.0.6 "@nomiclabs/hardhat-etherscan": ^3.1.7 - "@openzeppelin/contracts": 4.7.3 "@semaphore-protocol/group": "workspace:packages/group" "@semaphore-protocol/identity": "workspace:packages/identity" "@semaphore-protocol/proof": "workspace:packages/proof"