From 9fc3dd53c695dc03aab809872696270116f0d99c Mon Sep 17 00:00:00 2001 From: Chris Buckland Date: Mon, 7 Oct 2024 18:11:16 +0100 Subject: [PATCH 1/2] Updates from code review --- src/security-council-mgmt/SecurityCouncilManager.sol | 12 ++++++------ .../SecurityCouncilManager.t.sol | 6 ++++++ 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/security-council-mgmt/SecurityCouncilManager.sol b/src/security-council-mgmt/SecurityCouncilManager.sol index 1aefab02..f50a67bc 100644 --- a/src/security-council-mgmt/SecurityCouncilManager.sol +++ b/src/security-council-mgmt/SecurityCouncilManager.sol @@ -160,7 +160,7 @@ contract SecurityCouncilManager is _addSecurityCouncil(_securityCouncils[i]); } - setMinRotationPeriodImpl(_minRotationPeriod); + _setMinRotationPeriod(_minRotationPeriod); // we do our own 712 functionality because inheriting the OZ version // would change our storage layout @@ -171,11 +171,11 @@ contract SecurityCouncilManager is function postUpgradeInit(uint256 _minRotationPeriod, address minRotationPeriodSetter) external { - address proxyAdmin = ProxyUtil.getProxyAdmin(); - require(msg.sender == proxyAdmin, "NOT_FROM_ADMIN"); + require(msg.sender == ProxyUtil.getProxyAdmin(), "NOT_FROM_ADMIN"); + require(minRotationPeriod == 0, "MIN_ROTATION_ALREADY_SET"); _grantRole(MIN_ROTATION_PERIOD_SETTER_ROLE, minRotationPeriodSetter); - setMinRotationPeriodImpl(_minRotationPeriod); + _setMinRotationPeriod(_minRotationPeriod); NAME_HASH = keccak256(bytes("SecurityCouncilManager")); VERSION_HASH = keccak256(bytes("1")); @@ -192,10 +192,10 @@ contract SecurityCouncilManager is external onlyRole(MIN_ROTATION_PERIOD_SETTER_ROLE) { - setMinRotationPeriodImpl(_minRotationPeriod); + _setMinRotationPeriod(_minRotationPeriod); } - function setMinRotationPeriodImpl(uint256 _minRotationPeriod) internal { + function _setMinRotationPeriod(uint256 _minRotationPeriod) internal { minRotationPeriod = _minRotationPeriod; emit MinRotationPeriodSet(_minRotationPeriod); } diff --git a/test/security-council-mgmt/SecurityCouncilManager.t.sol b/test/security-council-mgmt/SecurityCouncilManager.t.sol index 6f0aca4a..40b19403 100644 --- a/test/security-council-mgmt/SecurityCouncilManager.t.sol +++ b/test/security-council-mgmt/SecurityCouncilManager.t.sol @@ -631,6 +631,12 @@ contract SecurityCouncilManagerTest is Test { ); assertEq(s.NAME_HASH(), keccak256(bytes("SecurityCouncilManager"))); assertEq(s.VERSION_HASH(), keccak256(bytes("1"))); + + vm.expectRevert("MIN_ROTATION_ALREADY_SET"); + vm.prank(address(pa)); + TransparentUpgradeableProxy(payable(address(s))).upgradeToAndCall( + address(logic), abi.encodeCall(s.postUpgradeInit, (mr, mrs)) + ); } function testSetMinRotationPeriod() public { From 73593e4c1a390671b22dfea7b2eca69de1eeccef Mon Sep 17 00:00:00 2001 From: Chris Buckland Date: Mon, 7 Oct 2024 18:14:14 +0100 Subject: [PATCH 2/2] Updated snapshot --- .gas-snapshot | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/.gas-snapshot b/.gas-snapshot index d93a90a8..be20914f 100644 --- a/.gas-snapshot +++ b/.gas-snapshot @@ -26,7 +26,7 @@ ArbitrumVestingWalletTest:testDelegateFailsForNonBeneficiary() (gas: 16008435) ArbitrumVestingWalletTest:testDoesDeploy() (gas: 15971342) ArbitrumVestingWalletTest:testReleaseAffordance() (gas: 16008649) ArbitrumVestingWalletTest:testVestedAmountStart() (gas: 16074917) -E2E:testE2E() (gas: 85825323) +E2E:testE2E() (gas: 85848560) FixedDelegateErc20WalletTest:testInit() (gas: 5822575) FixedDelegateErc20WalletTest:testInitZeroToken() (gas: 5816805) FixedDelegateErc20WalletTest:testTransfer() (gas: 5932218) @@ -94,14 +94,14 @@ L2GovernanceFactoryTest:testSanityCheckValues() (gas: 28415658) L2GovernanceFactoryTest:testSetMinDelay() (gas: 28364371) L2GovernanceFactoryTest:testSetMinDelayRevertsForCoreAddress() (gas: 28417242) L2GovernanceFactoryTest:testUpgraderCanCancel() (gas: 28657360) -L2SecurityCouncilMgmtFactoryTest:testMemberElectionGovDeployment() (gas: 31669215) -L2SecurityCouncilMgmtFactoryTest:testNomineeElectionGovDeployment() (gas: 31673446) -L2SecurityCouncilMgmtFactoryTest:testOnlyOwnerCanDeploy() (gas: 26712481) -L2SecurityCouncilMgmtFactoryTest:testRemovalGovDeployment() (gas: 31671446) -L2SecurityCouncilMgmtFactoryTest:testSecurityCouncilManagerDeployment() (gas: 31692326) +L2SecurityCouncilMgmtFactoryTest:testMemberElectionGovDeployment() (gas: 31692452) +L2SecurityCouncilMgmtFactoryTest:testNomineeElectionGovDeployment() (gas: 31696683) +L2SecurityCouncilMgmtFactoryTest:testOnlyOwnerCanDeploy() (gas: 26735718) +L2SecurityCouncilMgmtFactoryTest:testRemovalGovDeployment() (gas: 31694683) +L2SecurityCouncilMgmtFactoryTest:testSecurityCouncilManagerDeployment() (gas: 31715563) NomineeGovernorV2UpgradeActionTest:testAction() (gas: 8153) OfficeHoursActionTest:testConstructor() (gas: 9050) -OfficeHoursActionTest:testFuzzOfficeHoursDeployment(uint256,uint256,int256,uint256,uint256,uint256) (runs: 256, μ: 317072, ~: 317184) +OfficeHoursActionTest:testFuzzOfficeHoursDeployment(uint256,uint256,int256,uint256,uint256,uint256) (runs: 256, μ: 317070, ~: 317184) OfficeHoursActionTest:testInvalidConstructorParameters() (gas: 235740) OfficeHoursActionTest:testPerformBeforeMinimumTimestamp() (gas: 8646) OfficeHoursActionTest:testPerformDuringOfficeHours() (gas: 9140) @@ -125,7 +125,7 @@ SecurityCouncilManagerTest:testAddSCAffordances() (gas: 112133) SecurityCouncilManagerTest:testCantUpdateCohortWithADup() (gas: 123130) SecurityCouncilManagerTest:testCohortMethods() (gas: 136182) SecurityCouncilManagerTest:testInitialization() (gas: 209054) -SecurityCouncilManagerTest:testPostUpgradeInit() (gas: 5162749) +SecurityCouncilManagerTest:testPostUpgradeInit() (gas: 5191924) SecurityCouncilManagerTest:testRemoveMember() (gas: 213164) SecurityCouncilManagerTest:testRemoveMemberAffordances() (gas: 99124) SecurityCouncilManagerTest:testRemoveSCAffordances() (gas: 81287) @@ -156,7 +156,7 @@ SecurityCouncilMemberElectionGovernorTest:testOnlyNomineeElectionGovernorCanProp SecurityCouncilMemberElectionGovernorTest:testProperInitialization() (gas: 49388) SecurityCouncilMemberElectionGovernorTest:testProposeReverts() (gas: 32916) SecurityCouncilMemberElectionGovernorTest:testRelay() (gas: 42229) -SecurityCouncilMemberElectionGovernorTest:testSelectTopNominees(uint256) (runs: 256, μ: 340091, ~: 339846) +SecurityCouncilMemberElectionGovernorTest:testSelectTopNominees(uint256) (runs: 256, μ: 340178, ~: 340008) SecurityCouncilMemberElectionGovernorTest:testSelectTopNomineesFails() (gas: 273335) SecurityCouncilMemberElectionGovernorTest:testSetFullWeightDuration() (gas: 34951) SecurityCouncilMemberElectionGovernorTest:testVotesToWeight() (gas: 152898)