From 255bccf2ebc7a1ebfe06ebc7bb1bff2f8141fa83 Mon Sep 17 00:00:00 2001 From: cedoor Date: Thu, 21 Mar 2024 15:51:06 +0000 Subject: [PATCH] feat(contracts)!: create a two-step mechanism to update group admins The best practice to update a group admin is through a two-step update. The existing admin assigns the new potential admin, and the new admin accepts in a separate transaction. This prevents existing admins from making mistakes and assigning wrong addresses. re #690 --- packages/contracts/README.md | 2 +- packages/contracts/contracts/Semaphore.sol | 5 ++++ .../contracts/base/SemaphoreGroups.sol | 27 ++++++++++++++++--- .../contracts/interfaces/ISemaphore.sol | 3 +++ .../contracts/interfaces/ISemaphoreGroups.sol | 9 ++++++- packages/contracts/test/Semaphore.ts | 21 +++++++++++++-- 6 files changed, 60 insertions(+), 7 deletions(-) diff --git a/packages/contracts/README.md b/packages/contracts/README.md index c9f9bb70..1ee04425 120000 --- a/packages/contracts/README.md +++ b/packages/contracts/README.md @@ -1 +1 @@ -contracts/README.md +contracts/README.md \ No newline at end of file diff --git a/packages/contracts/contracts/Semaphore.sol b/packages/contracts/contracts/Semaphore.sol index bc163637..b76aa98f 100644 --- a/packages/contracts/contracts/Semaphore.sol +++ b/packages/contracts/contracts/Semaphore.sol @@ -56,6 +56,11 @@ contract Semaphore is ISemaphore, SemaphoreGroups { _updateGroupAdmin(groupId, newAdmin); } + /// @dev See {SemaphoreGroups- acceptGroupAdmin}. + function acceptGroupAdmin(uint256 groupId) external override { + _acceptGroupAdmin(groupId); + } + /// @dev See {ISemaphore-updateGroupMerkleTreeDuration}. function updateGroupMerkleTreeDuration( uint256 groupId, diff --git a/packages/contracts/contracts/base/SemaphoreGroups.sol b/packages/contracts/contracts/base/SemaphoreGroups.sol index 0cd43350..93a0faa5 100644 --- a/packages/contracts/contracts/base/SemaphoreGroups.sol +++ b/packages/contracts/contracts/base/SemaphoreGroups.sol @@ -19,6 +19,10 @@ abstract contract SemaphoreGroups is ISemaphoreGroups { /// The admin can be an Ethereum account or a smart contract. mapping(uint256 => address) internal admins; + /// @dev Gets a group id and returns any pending admin. + /// The pending admin can be an Ethereum account or a smart contract. + mapping(uint256 => address) internal pendingAdmins; + /// @dev Checks if the group admin is the transaction sender. /// @param groupId: Id of the group. modifier onlyGroupAdmin(uint256 groupId) { @@ -48,13 +52,30 @@ abstract contract SemaphoreGroups is ISemaphoreGroups { emit GroupAdminUpdated(groupId, address(0), admin); } - /// @dev Updates the group admin. + /// @dev Updates the group admin. In order for the new admin to actually be updated, + /// they must explicitly accept by calling `_acceptGroupAdmin`. /// @param groupId: Id of the group. /// @param newAdmin: New admin of the group. function _updateGroupAdmin(uint256 groupId, address newAdmin) internal virtual onlyGroupAdmin(groupId) { - admins[groupId] = newAdmin; + pendingAdmins[groupId] = newAdmin; - emit GroupAdminUpdated(groupId, msg.sender, newAdmin); + emit GroupAdminPending(groupId, msg.sender, newAdmin); + } + + /// @dev Allows the new admin to accept to update the group admin with their address. + /// @param groupId: Id of the group. + function _acceptGroupAdmin(uint256 groupId) internal virtual { + if (pendingAdmins[groupId] != msg.sender) { + revert Semaphore__CallerIsNotThePendingGroupAdmin(); + } + + address oldAdmin = admins[groupId]; + + admins[groupId] = msg.sender; + + delete pendingAdmins[groupId]; + + emit GroupAdminUpdated(groupId, oldAdmin, msg.sender); } /// @dev Adds an identity commitment to an existing group. diff --git a/packages/contracts/contracts/interfaces/ISemaphore.sol b/packages/contracts/contracts/interfaces/ISemaphore.sol index e2c425e1..b68feabc 100644 --- a/packages/contracts/contracts/interfaces/ISemaphore.sol +++ b/packages/contracts/contracts/interfaces/ISemaphore.sol @@ -70,6 +70,9 @@ interface ISemaphore { /// @dev See {SemaphoreGroups-_updateGroupAdmin}. function updateGroupAdmin(uint256 groupId, address newAdmin) external; + /// @dev See {SemaphoreGroups-_acceptGroupAdmin}. + function acceptGroupAdmin(uint256 groupId) external; + /// @dev Updates the group Merkle tree duration. /// @param groupId: Id of the group. /// @param newMerkleTreeDuration: New Merkle tree duration. diff --git a/packages/contracts/contracts/interfaces/ISemaphoreGroups.sol b/packages/contracts/contracts/interfaces/ISemaphoreGroups.sol index dd521bb8..b99bb7d6 100644 --- a/packages/contracts/contracts/interfaces/ISemaphoreGroups.sol +++ b/packages/contracts/contracts/interfaces/ISemaphoreGroups.sol @@ -5,17 +5,24 @@ pragma solidity 0.8.23; interface ISemaphoreGroups { error Semaphore__GroupDoesNotExist(); error Semaphore__CallerIsNotTheGroupAdmin(); + error Semaphore__CallerIsNotThePendingGroupAdmin(); /// @dev Event emitted when a new group is created. /// @param groupId: Id of the group. event GroupCreated(uint256 indexed groupId); - /// @dev Event emitted when an admin is assigned to a group. + /// @dev Event emitted when a new 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 Event emitted when a group admin is being updated. + /// @param groupId: Id of the group. + /// @param oldAdmin: Old admin of the group. + /// @param newAdmin: New admin of the group. + event GroupAdminPending(uint256 indexed groupId, address indexed oldAdmin, address indexed newAdmin); + /// @dev Event emitted when a new identity commitment is added. /// @param groupId: Group id of the group. /// @param index: Merkle tree leaf index. diff --git a/packages/contracts/test/Semaphore.ts b/packages/contracts/test/Semaphore.ts index 8d5607ef..9e5d0cfb 100644 --- a/packages/contracts/test/Semaphore.ts +++ b/packages/contracts/test/Semaphore.ts @@ -87,7 +87,7 @@ describe("Semaphore", () => { }) describe("# updateGroupAdmin", () => { - it("Should not update a group admin if the caller is not the group admin", async () => { + it("Should not update an admin if the caller is not the admin", async () => { const transaction = semaphoreContract.updateGroupAdmin(groupId, accountAddresses[0]) await expect(transaction).to.be.revertedWithCustomError( @@ -96,9 +96,26 @@ describe("Semaphore", () => { ) }) - it("Should update the group admin", async () => { + it("Should update the admin", async () => { const transaction = semaphoreContract.connect(accounts[1]).updateGroupAdmin(groupId, accountAddresses[0]) + await expect(transaction) + .to.emit(semaphoreContract, "GroupAdminPending") + .withArgs(groupId, accountAddresses[1], accountAddresses[0]) + }) + + it("Should not accept accept the new admin if the caller is not the new admin", async () => { + const transaction = semaphoreContract.connect(accounts[2]).acceptGroupAdmin(groupId) + + await expect(transaction).to.be.revertedWithCustomError( + semaphoreContract, + "Semaphore__CallerIsNotThePendingGroupAdmin" + ) + }) + + it("Should accept the new admin", async () => { + const transaction = semaphoreContract.acceptGroupAdmin(groupId) + await expect(transaction) .to.emit(semaphoreContract, "GroupAdminUpdated") .withArgs(groupId, accountAddresses[1], accountAddresses[0])