Security Council Manager Affordance Change Action (ArbitrumFoundation…

…#284)

* fix overflow when aliasing

* remove commented

* remove unused offset var

* update forge-std to v1.8.2

* take advantage of skip cheatcode for fork test

* GrantRotatorRoleToNonEmergencyCouncil

* use l1 timelock alias in test

* rename and cleanup

* all roles

* rename

* fix test

* update diagram

* update docs

* gas snapshot

* Revert "gas snapshot"

This reverts commit fd26592.

* use previous forge-std version

* use old fork check

* gas snapshot

* update docs

* add checks in perform

* update gotchas

* update remappings

.gas-snapshot

@@ -13,7 +13,7 @@ ArbitrumFoundationVestingWalletTest:testOnlyOwnerCanMigrate() (gas: 16329757)
 ArbitrumFoundationVestingWalletTest:testOwnerCanSetBeneficiary() (gas: 16332176)
 ArbitrumFoundationVestingWalletTest:testProperlyInits() (gas: 16337546)
 ArbitrumFoundationVestingWalletTest:testRandomAddressCantSetBeneficiary() (gas: 16329656)
-ArbitrumFoundationVestingWalletTest:testRelease() (gas: 16448631)
+ArbitrumFoundationVestingWalletTest:testRelease() (gas: 16451131)
 ArbitrumVestingWalletFactoryTest:testDeploy() (gas: 4589688)
 ArbitrumVestingWalletFactoryTest:testOnlyOwnerCanCreateWallets() (gas: 1504286)
 ArbitrumVestingWalletTest:testCastVote() (gas: 16201584)
@@ -25,7 +25,7 @@ ArbitrumVestingWalletTest:testDelegateFailsForNonBeneficiary() (gas: 16008435)
 ArbitrumVestingWalletTest:testDoesDeploy() (gas: 15971342)
 ArbitrumVestingWalletTest:testReleaseAffordance() (gas: 16008649)
 ArbitrumVestingWalletTest:testVestedAmountStart() (gas: 16074917)
-E2E:testE2E() (gas: 84675625)
+E2E:testE2E() (gas: 84680100)
 FixedDelegateErc20WalletTest:testInit() (gas: 5822575)
 FixedDelegateErc20WalletTest:testInitZeroToken() (gas: 5816805)
 FixedDelegateErc20WalletTest:testTransfer() (gas: 5932218)
@@ -129,8 +129,8 @@ SecurityCouncilManagerTest:testUpdateRouter() (gas: 76296)
 SecurityCouncilManagerTest:testUpdateRouterAffordacnes() (gas: 112379)
 SecurityCouncilManagerTest:testUpdateSecondCohort() (gas: 295316)
 SecurityCouncilMemberElectionGovernorTest:testCannotUseMoreVotesThanAvailable() (gas: 246997)
-SecurityCouncilMemberElectionGovernorTest:testCastBySig() (gas: 302832)
-SecurityCouncilMemberElectionGovernorTest:testCastBySigTwice() (gas: 266284)
+SecurityCouncilMemberElectionGovernorTest:testCastBySig() (gas: 302852)
+SecurityCouncilMemberElectionGovernorTest:testCastBySigTwice() (gas: 266244)
 SecurityCouncilMemberElectionGovernorTest:testCastVoteReverts() (gas: 35277)
 SecurityCouncilMemberElectionGovernorTest:testExecute() (gas: 665450)
 SecurityCouncilMemberElectionGovernorTest:testForceSupport() (gas: 165349)
@@ -143,7 +143,7 @@ SecurityCouncilMemberElectionGovernorTest:testOnlyNomineeElectionGovernorCanProp
 SecurityCouncilMemberElectionGovernorTest:testProperInitialization() (gas: 49388)
 SecurityCouncilMemberElectionGovernorTest:testProposeReverts() (gas: 32916)
 SecurityCouncilMemberElectionGovernorTest:testRelay() (gas: 42229)
-SecurityCouncilMemberElectionGovernorTest:testSelectTopNominees(uint256) (runs: 256, μ: 339956, ~: 339703)
+SecurityCouncilMemberElectionGovernorTest:testSelectTopNominees(uint256) (runs: 256, μ: 340009, ~: 339543)
 SecurityCouncilMemberElectionGovernorTest:testSelectTopNomineesFails() (gas: 273335)
 SecurityCouncilMemberElectionGovernorTest:testSetFullWeightDuration() (gas: 34951)
 SecurityCouncilMemberElectionGovernorTest:testVotesToWeight() (gas: 152898)
@@ -197,11 +197,12 @@ SequencerActionsTest:testCantAddZeroAddress() (gas: 235614)
 SetInitialGovParamsActionTest:testL1() (gas: 259904)
 SetInitialGovParamsActionTest:testL2() (gas: 688888)
 SetSequencerInboxMaxTimeVariationAction:testSetMaxTimeVariation() (gas: 374262)
+SwitchManagerRolesActionTest:testAction() (gas: 6313)
 TokenDistributorTest:testClaim() (gas: 5742744)
 TokenDistributorTest:testClaimAndDelegate() (gas: 5850827)
 TokenDistributorTest:testClaimAndDelegateFailsForExpired() (gas: 5748244)
 TokenDistributorTest:testClaimAndDelegateFailsForWrongSender() (gas: 5803385)
-TokenDistributorTest:testClaimAndDelegateFailsWrongNonce() (gas: 5803366)
+TokenDistributorTest:testClaimAndDelegateFailsWrongNonce() (gas: 5803386)
 TokenDistributorTest:testClaimFailsAfterEnd() (gas: 5704035)
 TokenDistributorTest:testClaimFailsBeforeStart() (gas: 5703530)
 TokenDistributorTest:testClaimFailsForFalseTransfer() (gas: 5686246)

docs/gotchas.md

@@ -29,6 +29,9 @@ Typically, for a timelocked OZ governror, the `onlyGovernance` modifier ensures
 
 - **UpgradeExecutor Affordance** 
 
-Affordances are always given to the DAO via an UpgradeExecutor contract, which grants affordance to both the core governor proposal path and the Security Council. This includes abilities that are intended only for the Security Council; for example, proposal cancellation, practically speaking, could/would only ever be preformed by the Security Council (since the DAO wouldn't have time to vote on and execute a cancellation). Still, for this case, the affordance is given to the UpgradeExecutor; this is done for clarity, consistency, and to ensure that the UpgradeExecutor is the single source of truth for execution rights.
+Affordances are always given to the DAO via an UpgradeExecutor contract, which grants affordance to both the core governor proposal path and the Emergency Security Councils. This includes abilities that are intended only for the Security Council; for example, proposal cancellation, practically speaking, could/would only ever be preformed by the Security Council (since the DAO wouldn't have time to vote on and execute a cancellation). Still, for this case, the affordance is given to the UpgradeExecutor; this is done for clarity, consistency, and to ensure that the UpgradeExecutor is the single source of truth for execution rights.
 
-The only affordances granted directly to the Security Council (and not to its corresponding UpradeExecutor) are the "MEMBER_ADDER", "MEMBER_REPLACER", "MEMBER_ROTATOR", and "MEMBER_REMOVER" roles on the SecurityCouncilManager contract. If the emergency Security Council on Arbitrum One is ever either removed or deployed to a new address, these roles should be modified accordingly.
+- **Non Emergency Security Council Affordances**
+In addition to proposing to the L2 Timlock, the only affordances granted directly to the Non Emergency Security Council are the "MEMBER_ADDER", "MEMBER_REPLACER", "MEMBER_ROTATOR", and "MEMBER_REMOVER" roles on the SecurityCouncilManager contract. If the Non Emergency Security Council on Arbitrum One is ever either removed or deployed to a new address, these roles should be modified accordingly.
+
+Additionally, if the signing threshold of the Non Emergency Security Council is reduced to be less than the Emergency Council, these roles should be revoked and instead granted to the Emergency Council to remain consistent with the Constitution.

docs/security-council-manager.md

@@ -20,27 +20,27 @@ Can only be called by the Member Election Governor. It is used to replace a whol
 
 ### Remove member
 
-Can be called by the Emergency Security Council and the Removal Governor. Is used to remove a member. The Constitution allows members to be removed if 9 of 12 members wish them to be, or if the DAO votes to remove a member and 10% of votable tokens cast a vote, with 5/6 of voted tokens are in favour of removal.
+Can be called by the Non-Emergency Security Council and the Removal Governor. Is used to remove a member. The Constitution allows members to be removed if 9 of 12 members wish them to be, or if the DAO votes to remove a member and 10% of votable tokens cast a vote, with 5/6 of voted tokens are in favour of removal.
 
 ### Add member
 
-Can only be called by the Emergency Security Council. Can only be called if the there are less than 12 members in the Security Council because at least one has been removed.
+Can only be called by the Non-Emergency Security Council. Can only be called if the there are less than 12 members in the Security Council because at least one has been removed.
 
 ### Replace member
 
-Can only be called by the Emergency Security Council. This is a utility function to allow the council to call Remove and Add in the same transaction. Semantically this means that an entity has been removed from the council, and a different one has been added.
+Can only be called by the Non-Emergency Security Council. This is a utility function to allow the council to call Remove and Add in the same transaction. Semantically this means that an entity has been removed from the council, and a different one has been added.
 
 ### Rotate member
 
-Can only be called by the Emergency Security Council. Functionally this is the same as Replace, however semantically it infers different intent. Rotate should be called when a entity wish to replace their address, but it is the same entity that controls the newly added address.
+Can only be called by the Non-Emergency Security Council. Functionally this is the same as Replace, however semantically it infers different intent. Rotate should be called when a entity wish to replace their address, but it is the same entity that controls the newly added address.
 
 ### Add Security Council
 
-Can be called by DAO and the emergency security council. Adds an additional security council whose members will be updated by the election system. 
+Can be called by DAO and the Emergency Security Council. Adds an additional security council whose members will be updated by the election system. 
 
 ### Remove Security Council
 
-Can be called by DAO and the emergency security council. Removes a security council from being updated by the election system.
+Can be called by DAO and the Emergency Security Council. Removes a security council from being updated by the election system.
 
 
 ## Race conditions

docs/security-council-mgmt.md

@@ -163,7 +163,7 @@ The old cohort of members will be removed, and the new cohort will replace them.
 
 ### Implementation details
 
-To do this the existing [Upgrade Executor contracts](https://github.com/ArbitrumFoundation/governance/blob/main/docs/overview.md#l1-upgrade-executor) on each chain will be installed as Gnosis Safe modules into the Security Council safes. A custom [Governance Action Contract](https://github.com/ArbitrumFoundation/governance/blob/main/src/gov-action-contracts/README.md) will be used to call the specific `OwnerManager` `addOwnerWithThreshold` and `removeOwner` methods on the Gnosis safes.
+To do this the existing [Upgrade Executor contracts](https://github.com/ArbitrumFoundation/governance/blob/main/docs/overview.md#l1-upgrade-executor) on each chain will be installed as Gnosis Safe modules into the Security Council Safes. A custom [Governance Action Contract](https://github.com/ArbitrumFoundation/governance/blob/main/src/gov-action-contracts/README.md) will be used to call the specific `OwnerManager` `addOwnerWithThreshold` and `removeOwner` methods on the Gnosis Safes.
 
 ## Additional affordances
 
@@ -176,15 +176,17 @@ It accepts proposals in the same format that normal governors do, however it ove
 
 Voting and proposing can occur using the standard governance UIs.
 
-### 9/12 Security Council
+### Non-Emergency Security Council
 
-The Security Council can remove a member prior to the end of their term, if 9 of 12 members agree. The 9 of 12 council has the rights to call `removeMember` on the `SecurityCouncilManager`.
+The Security Council can remove a member prior to the end of their term, if 9 of 12 members agree. The non-emergency council has the right to call `removeMember` on the `SecurityCouncilManager`.
 
-The Security Council can also add a member once one has been removed if 9 of 12 members agree and if there are less than 12 members currently on the council. The 9 of 12 council is be given the rights to call `addMember` on the `SecurityCouncilManager`.
+The Security Council can add a member if there are less than 12 members currently on the council. The non-emergency council has the right to call `addMember` on the `SecurityCouncilManager`.
+
+The Security Council can replace members or rotate keys at any time. The non-emergency council has the right to call `replaceMember` and `rotateMember` on the `SecurityCouncilManager`. These 2 functions are functionally identical, although they imply different intent/purpose
 
 ### Overall diagram
 Below is a diagram showing the interaction between the different components described above:
-![](./security-council-colors.png)
+![](./security-council-mgmt-flow.png)
 
 ### Block periods
 The constitution specifies time periods in days and weeks, however in the implementation block numbers are used as a proxy for this. In the event of an L1 block time change the contracts here, and in general governance, would need to be updated to reflect the time periods again.

remappings.txt

@@ -1,9 +1,8 @@
 forge-std/=lib/forge-std/src/
 solady/=lib/solady/src/
-
-@arbitrum/token-bridge-contracts/=node_modules/@arbitrum/token-bridge-contracts
+@arbitrum/token-bridge-contracts/=node_modules/@arbitrum/token-bridge-contracts/
 @arbitrum/nitro-contracts/=node_modules/@arbitrum/nitro-contracts/
-
 @openzeppelin/contracts-upgradeable/=node_modules/@openzeppelin/contracts-upgradeable/
 @openzeppelin/contracts/=node_modules/@openzeppelin/contracts/
-@gnosis.pm/safe-contracts/=node_modules/@gnosis.pm/safe-contracts/
+@gnosis.pm/safe-contracts/=node_modules/@gnosis.pm/safe-contracts/
+ds-test/=lib/forge-std/lib/ds-test/src/

src/gov-action-contracts/nonemergency/SwitchManagerRolesAction.sol

@@ -0,0 +1,44 @@
+// SPDX-License-Identifier: Apache-2.0
+pragma solidity 0.8.16;
+
+import "../../security-council-mgmt/SecurityCouncilManager.sol";
+
+/// @notice Grant the non emergency council the MEMBER_ADDER_ROLE, MEMBER_REPLACER_ROLE, MEMBER_ROTATOR_ROLE and MEMBER_REMOVER_ROLE on the SecurityCouncilManager.
+///         Revoke those same roles from the emergency council.
+contract SwitchManagerRolesAction {
+    SecurityCouncilManager public constant securityCouncilManager =
+        SecurityCouncilManager(0xD509E5f5aEe2A205F554f36E8a7d56094494eDFC);
+
+    address public constant nonEmergencyCouncil = 0xADd68bCb0f66878aB9D37a447C7b9067C5dfa941;
+    address public constant emergencyCouncil = 0x423552c0F05baCCac5Bfa91C6dCF1dc53a0A1641;
+
+    bytes32 public immutable MEMBER_ADDER_ROLE = securityCouncilManager.MEMBER_ADDER_ROLE();
+    bytes32 public immutable MEMBER_REPLACER_ROLE = securityCouncilManager.MEMBER_REPLACER_ROLE();
+    bytes32 public immutable MEMBER_ROTATOR_ROLE = securityCouncilManager.MEMBER_ROTATOR_ROLE();
+    bytes32 public immutable MEMBER_REMOVER_ROLE = securityCouncilManager.MEMBER_REMOVER_ROLE();
+
+    function perform() public {
+        // grant roles to non emergency council
+        securityCouncilManager.grantRole(MEMBER_ADDER_ROLE, nonEmergencyCouncil);
+        securityCouncilManager.grantRole(MEMBER_REPLACER_ROLE, nonEmergencyCouncil);
+        securityCouncilManager.grantRole(MEMBER_ROTATOR_ROLE, nonEmergencyCouncil);
+        securityCouncilManager.grantRole(MEMBER_REMOVER_ROLE, nonEmergencyCouncil);
+
+        // revoke roles from emergency council
+        securityCouncilManager.revokeRole(MEMBER_ADDER_ROLE, emergencyCouncil);
+        securityCouncilManager.revokeRole(MEMBER_REPLACER_ROLE, emergencyCouncil);
+        securityCouncilManager.revokeRole(MEMBER_ROTATOR_ROLE, emergencyCouncil);
+        securityCouncilManager.revokeRole(MEMBER_REMOVER_ROLE, emergencyCouncil);
+
+        // ensure roles were changed
+        require(securityCouncilManager.hasRole(MEMBER_ADDER_ROLE, nonEmergencyCouncil), "Adder role not granted");
+        require(securityCouncilManager.hasRole(MEMBER_REPLACER_ROLE, nonEmergencyCouncil), "Replacer role not granted");
+        require(securityCouncilManager.hasRole(MEMBER_ROTATOR_ROLE, nonEmergencyCouncil), "Rotator role not granted");
+        require(securityCouncilManager.hasRole(MEMBER_REMOVER_ROLE, nonEmergencyCouncil), "Remover role not granted");
+
+        require(!securityCouncilManager.hasRole(MEMBER_ADDER_ROLE, emergencyCouncil), "Adder role not revoked");
+        require(!securityCouncilManager.hasRole(MEMBER_REPLACER_ROLE, emergencyCouncil), "Replacer role not revoked");
+        require(!securityCouncilManager.hasRole(MEMBER_ROTATOR_ROLE, emergencyCouncil), "Rotator role not revoked");
+        require(!securityCouncilManager.hasRole(MEMBER_REMOVER_ROLE, emergencyCouncil), "Remover role not revoked");
+    }
+}

test/gov-actions/SwitchManagerRolesAction.t.sol

@@ -0,0 +1,36 @@
+// SPDX-License-Identifier: Apache-2.0
+pragma solidity 0.8.16;
+
+import "forge-std/Test.sol";
+
+import "../../src/gov-action-contracts/nonemergency/SwitchManagerRolesAction.sol";
+
+contract SwitchManagerRolesActionTest is Test {
+    UpgradeExecutor arbOneUe = UpgradeExecutor(0xCF57572261c7c2BCF21ffD220ea7d1a27D40A827);
+
+    function testAction() external {
+        if (!isFork()) {
+            console.log("not fork test, skipping SwitchManagerRolesActionTest");
+            return;
+        }
+
+        SwitchManagerRolesAction gac = new SwitchManagerRolesAction();
+
+        address emergencyCouncil = gac.emergencyCouncil();
+        address nonEmergencyCouncil = gac.nonEmergencyCouncil();
+        SecurityCouncilManager manager = gac.securityCouncilManager();
+
+        vm.prank(0xf7951D92B0C345144506576eC13Ecf5103aC905a); // L1 Timelock Alias
+        arbOneUe.execute(address(gac), abi.encodeWithSignature("perform()"));
+
+        assertTrue(manager.hasRole(manager.MEMBER_ADDER_ROLE(), nonEmergencyCouncil));
+        assertTrue(manager.hasRole(manager.MEMBER_REPLACER_ROLE(), nonEmergencyCouncil));
+        assertTrue(manager.hasRole(manager.MEMBER_ROTATOR_ROLE(), nonEmergencyCouncil));
+        assertTrue(manager.hasRole(manager.MEMBER_REMOVER_ROLE(), nonEmergencyCouncil));
+
+        assertFalse(manager.hasRole(manager.MEMBER_ADDER_ROLE(), emergencyCouncil));
+        assertFalse(manager.hasRole(manager.MEMBER_REPLACER_ROLE(), emergencyCouncil));
+        assertFalse(manager.hasRole(manager.MEMBER_ROTATOR_ROLE(), emergencyCouncil));
+        assertFalse(manager.hasRole(manager.MEMBER_REMOVER_ROLE(), emergencyCouncil));
+    }
+}