Updates from code review

ArbitrumFoundation · Oct 7, 2024 · 9fc3dd5 · 9fc3dd5
1 parent b057634
commit 9fc3dd5
Show file tree

Hide file tree

Showing 2 changed files with 12 additions and 6 deletions.
diff --git 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
@@ -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 {