Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Manager Role Proposal Data #290

Open
wants to merge 31 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
7c7b46f
fix overflow when aliasing
godzillaba Jun 5, 2024
17c676a
remove commented
godzillaba Jun 5, 2024
4e5e6d4
remove unused offset var
godzillaba Jun 5, 2024
99f69f1
update forge-std to v1.8.2
godzillaba Jun 5, 2024
d06b79a
take advantage of skip cheatcode for fork test
godzillaba Jun 5, 2024
115b737
Merge branch 'fix-test-overflow' into update-forge-std
godzillaba Jun 5, 2024
af13939
GrantRotatorRoleToNonEmergencyCouncil
godzillaba Jun 5, 2024
bdab94b
use l1 timelock alias in test
godzillaba Jun 5, 2024
adfc064
rename and cleanup
godzillaba Jun 5, 2024
3580208
all roles
godzillaba Jun 5, 2024
39b5dc2
rename
godzillaba Jun 5, 2024
4edff8d
fix test
godzillaba Jun 5, 2024
208fd9c
update diagram
godzillaba Jun 5, 2024
6eb3867
update docs
godzillaba Jun 5, 2024
fd26592
gas snapshot
godzillaba Jun 5, 2024
704d543
Merge branch 'update-forge-std' into manager-role-action
godzillaba Jun 5, 2024
31402a4
Revert "gas snapshot"
godzillaba Jun 5, 2024
92a940b
Merge branch 'update-forge-std' into manager-role-action
godzillaba Jun 5, 2024
1d1c684
use previous forge-std version
godzillaba Jun 5, 2024
43431a7
use old fork check
godzillaba Jun 5, 2024
fb72034
gas snapshot
godzillaba Jun 5, 2024
2cb8230
Merge branch 'main' into manager-role-action
godzillaba Jun 7, 2024
199e4f1
update docs
godzillaba Jun 7, 2024
275b6a8
Merge branch 'main' into manager-role-action
godzillaba Jun 7, 2024
d7bf332
prop data
godzillaba Jun 11, 2024
7d76dec
add checks in perform
godzillaba Jun 12, 2024
8a2ba80
update gotchas
godzillaba Jun 12, 2024
3240a71
Merge branch 'manager-role-action' into manager-role-action-data
godzillaba Jun 12, 2024
9561a5f
update proposal data
godzillaba Jun 12, 2024
b3b8fee
add bash script
godzillaba Jun 19, 2024
dc64493
chmod +x
godzillaba Jun 20, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions .gas-snapshot
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
7 changes: 5 additions & 2 deletions docs/gotchas.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Binary file removed docs/security-council-colors.png
Binary file not shown.
12 changes: 6 additions & 6 deletions docs/security-council-manager.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Binary file added docs/security-council-mgmt-flow.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
12 changes: 7 additions & 5 deletions docs/security-council-mgmt.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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.
Expand Down
12 changes: 12 additions & 0 deletions scripts/proposals/SwitchManagerRoles/SwitchManagerRoles.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"actionChainIds": [
42161
],
"actionAddresses": [
"0x29f3c6b8c98488FBAE0677AB3d2Eb29c77D6aD8a"
],
"l2TimelockScheduleArgs": {
"target": "0x0000000000000000000000000000000000000064",
"calldata": "0x928c169a000000000000000000000000e6841d92b0c345144506576ec13ecf5103ac7f49000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000003248f2a0bb000000000000000000000000000000000000000000000000000000000000000c0000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000001400000000000000000000000000000000000000000000000000000000000000000598d392a3d2df7781f2c97ff39416b8d1518474afc65984ff12187050ddbfbae000000000000000000000000000000000000000000000000000000000003f4800000000000000000000000000000000000000000000000000000000000000001000000000000000000000000a723c008e76e379c55599d2e4d93879beafda79c000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000001800000000000000000000000004dbd4fc535ac27206064b68ffcf827b0a60bab3f000000000000000000000000cf57572261c7c2bcf21ffd220ea7d1a27d40a82700000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000c000000000000000000000000000000000000000000000000000000000000000841cff79cd00000000000000000000000029f3c6b8c98488fbae0677ab3d2eb29c77d6ad8a00000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000000000000000000004b147f40c000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000"
}
}
7 changes: 7 additions & 0 deletions scripts/proposals/SwitchManagerRoles/generate.bash
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
#!/bin/bash

yarn gen:proposalData --govChainProviderRPC https://arb1.arbitrum.io/rpc \
--actionChainIds 42161 \
--actionAddresses 0x29f3c6b8c98488FBAE0677AB3d2Eb29c77D6aD8a \
--writeToJsonPath ./scripts/proposals/SwitchManagerRoles/SwitchManagerRoles.json \
--nonEmergencySCproposal true
44 changes: 44 additions & 0 deletions src/gov-action-contracts/nonemergency/SwitchManagerRolesAction.sol
Original file line number Diff line number Diff line change
@@ -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");
}
}
36 changes: 36 additions & 0 deletions test/gov-actions/SwitchManagerRolesAction.t.sol
Original file line number Diff line number Diff line change
@@ -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));
}
}
Loading