fix: prevent duplicate members

.gas-snapshot

@@ -14,7 +14,7 @@ ArbitrumFoundationVestingWalletTest:testOnlyOwnerCanMigrate() (gas: 16329777)
 ArbitrumFoundationVestingWalletTest:testOwnerCanSetBeneficiary() (gas: 16332196)
 ArbitrumFoundationVestingWalletTest:testProperlyInits() (gas: 16337566)
 ArbitrumFoundationVestingWalletTest:testRandomAddressCantSetBeneficiary() (gas: 16329676)
-ArbitrumFoundationVestingWalletTest:testRelease() (gas: 16451151)
+ArbitrumFoundationVestingWalletTest:testRelease() (gas: 16448651)
 ArbitrumVestingWalletFactoryTest:testDeploy() (gas: 4589688)
 ArbitrumVestingWalletFactoryTest:testOnlyOwnerCanCreateWallets() (gas: 1504286)
 ArbitrumVestingWalletTest:testCastVote() (gas: 16201599)
@@ -27,7 +27,7 @@ ArbitrumVestingWalletTest:testDoesDeploy() (gas: 15971357)
 ArbitrumVestingWalletTest:testReleaseAffordance() (gas: 16008664)
 ArbitrumVestingWalletTest:testVestedAmountStart() (gas: 16074932)
 CancelTimelockAndRemoveMemberActionTest:testAction() (gas: 8159)
-E2E:testE2E() (gas: 86859495)
+E2E:testE2E() (gas: 86806023)
 FixedDelegateErc20WalletTest:testInit() (gas: 5822585)
 FixedDelegateErc20WalletTest:testInitZeroToken() (gas: 5816815)
 FixedDelegateErc20WalletTest:testTransfer() (gas: 5932228)
@@ -95,14 +95,14 @@ L2GovernanceFactoryTest:testSanityCheckValues() (gas: 28571182)
 L2GovernanceFactoryTest:testSetMinDelay() (gas: 28519939)
 L2GovernanceFactoryTest:testSetMinDelayRevertsForCoreAddress() (gas: 28572810)
 L2GovernanceFactoryTest:testUpgraderCanCancel() (gas: 28812928)
-L2SecurityCouncilMgmtFactoryTest:testMemberElectionGovDeployment() (gas: 32322314)
-L2SecurityCouncilMgmtFactoryTest:testNomineeElectionGovDeployment() (gas: 32326589)
-L2SecurityCouncilMgmtFactoryTest:testOnlyOwnerCanDeploy() (gas: 27265404)
-L2SecurityCouncilMgmtFactoryTest:testRemovalGovDeployment() (gas: 32324545)
-L2SecurityCouncilMgmtFactoryTest:testSecurityCouncilManagerDeployment() (gas: 32345900)
+L2SecurityCouncilMgmtFactoryTest:testMemberElectionGovDeployment() (gas: 32271955)
+L2SecurityCouncilMgmtFactoryTest:testNomineeElectionGovDeployment() (gas: 32276230)
+L2SecurityCouncilMgmtFactoryTest:testOnlyOwnerCanDeploy() (gas: 27215045)
+L2SecurityCouncilMgmtFactoryTest:testRemovalGovDeployment() (gas: 32274186)
+L2SecurityCouncilMgmtFactoryTest:testSecurityCouncilManagerDeployment() (gas: 32295541)
 NomineeGovernorV2UpgradeActionTest:testAction() (gas: 8153)
 OfficeHoursActionTest:testConstructor() (gas: 9050)
-OfficeHoursActionTest:testFuzzOfficeHoursDeployment(uint256,uint256,int256,uint256,uint256,uint256) (runs: 256, μ: 317059, ~: 317184)
+OfficeHoursActionTest:testFuzzOfficeHoursDeployment(uint256,uint256,int256,uint256,uint256,uint256) (runs: 256, μ: 317090, ~: 317184)
 OfficeHoursActionTest:testInvalidConstructorParameters() (gas: 235740)
 OfficeHoursActionTest:testPerformBeforeMinimumTimestamp() (gas: 8646)
 OfficeHoursActionTest:testPerformDuringOfficeHours() (gas: 9140)
@@ -138,12 +138,12 @@ SecurityCouncilManagerTest:testReplaceMemberInFirstCohortAfterRotation() (gas: 4
 SecurityCouncilManagerTest:testReplaceMemberInSecondCohort() (gas: 479028)
 SecurityCouncilManagerTest:testReplaceMemberInSecondCohortAfterRotation() (gas: 270210)
 SecurityCouncilManagerTest:testRotateMember() (gas: 1015787)
-SecurityCouncilManagerTest:testRotateMemberNotContender() (gas: 4078679)
+SecurityCouncilManagerTest:testRotateMemberNotContender() (gas: 4080567)
 SecurityCouncilManagerTest:testSetMinRotationPeriod() (gas: 65814)
 SecurityCouncilManagerTest:testUpdateCohortAffordances() (gas: 83252)
 SecurityCouncilManagerTest:testUpdateFirstCohort() (gas: 313830)
 SecurityCouncilManagerTest:testUpdateRouter() (gas: 76407)
-SecurityCouncilManagerTest:testUpdateRouterAffordances() (gas: 112474)
+SecurityCouncilManagerTest:testUpdateRouterAffordances() (gas: 109974)
 SecurityCouncilManagerTest:testUpdateSecondCohort() (gas: 313924)
 SecurityCouncilMemberElectionGovernorTest:testCannotUseMoreVotesThanAvailable() (gas: 247018)
 SecurityCouncilMemberElectionGovernorTest:testCastBySig() (gas: 302873)
@@ -160,7 +160,7 @@ SecurityCouncilMemberElectionGovernorTest:testOnlyNomineeElectionGovernorCanProp
 SecurityCouncilMemberElectionGovernorTest:testProperInitialization() (gas: 49388)
 SecurityCouncilMemberElectionGovernorTest:testProposeReverts() (gas: 32916)
 SecurityCouncilMemberElectionGovernorTest:testRelay() (gas: 42229)
-SecurityCouncilMemberElectionGovernorTest:testSelectTopNominees(uint256) (runs: 256, μ: 340137, ~: 339953)
+SecurityCouncilMemberElectionGovernorTest:testSelectTopNominees(uint256) (runs: 256, μ: 339752, ~: 339539)
 SecurityCouncilMemberElectionGovernorTest:testSelectTopNomineesFails() (gas: 273467)
 SecurityCouncilMemberElectionGovernorTest:testSetFullWeightDuration() (gas: 34951)
 SecurityCouncilMemberElectionGovernorTest:testVotesToWeight() (gas: 152898)
@@ -194,25 +194,25 @@ SecurityCouncilMemberSyncActionTest:testRemoveOne() (gas: 8086867)
 SecurityCouncilMemberSyncActionTest:testUpdateCohort() (gas: 8328313)
 SecurityCouncilMemberSyncActionTest:testUpdateCohort() (gas: 8329174)
 SecurityCouncilMgmtUtilsTests:testIsInArray() (gas: 2102)
-SecurityCouncilNomineeElectionGovernorTest:testAddContender() (gas: 418329)
+SecurityCouncilNomineeElectionGovernorTest:testAddContender() (gas: 418681)
 SecurityCouncilNomineeElectionGovernorTest:testCadenceWithLargeValues() (gas: 52898)
-SecurityCouncilNomineeElectionGovernorTest:testCastBySig() (gas: 338828)
-SecurityCouncilNomineeElectionGovernorTest:testCastBySigTwice() (gas: 301643)
+SecurityCouncilNomineeElectionGovernorTest:testCastBySig() (gas: 338857)
+SecurityCouncilNomineeElectionGovernorTest:testCastBySigTwice() (gas: 301672)
 SecurityCouncilNomineeElectionGovernorTest:testCastVoteReverts() (gas: 35303)
-SecurityCouncilNomineeElectionGovernorTest:testCountVote() (gas: 593196)
+SecurityCouncilNomineeElectionGovernorTest:testCountVote() (gas: 593835)
 SecurityCouncilNomineeElectionGovernorTest:testCreateElection() (gas: 257942)
 SecurityCouncilNomineeElectionGovernorTest:testDefaultCadence() (gas: 14950)
 SecurityCouncilNomineeElectionGovernorTest:testElectionTimestampsWithDefaultCadence() (gas: 37625)
-SecurityCouncilNomineeElectionGovernorTest:testExcludeNominee() (gas: 461501)
-SecurityCouncilNomineeElectionGovernorTest:testExecute() (gas: 679315)
-SecurityCouncilNomineeElectionGovernorTest:testForceSupport() (gas: 199863)
-SecurityCouncilNomineeElectionGovernorTest:testIncludeNominee() (gas: 679144)
-SecurityCouncilNomineeElectionGovernorTest:testInvalidInit() (gas: 7833321)
+SecurityCouncilNomineeElectionGovernorTest:testExcludeNominee() (gas: 461806)
+SecurityCouncilNomineeElectionGovernorTest:testExecute() (gas: 679489)
+SecurityCouncilNomineeElectionGovernorTest:testForceSupport() (gas: 199892)
+SecurityCouncilNomineeElectionGovernorTest:testIncludeNominee() (gas: 678878)
+SecurityCouncilNomineeElectionGovernorTest:testInvalidInit() (gas: 7782964)
 SecurityCouncilNomineeElectionGovernorTest:testMultipleCadenceChanges() (gas: 238915)
 SecurityCouncilNomineeElectionGovernorTest:testProperInitialization() (gas: 78159)
 SecurityCouncilNomineeElectionGovernorTest:testProposeFails() (gas: 19786)
 SecurityCouncilNomineeElectionGovernorTest:testRelay() (gas: 42411)
-SecurityCouncilNomineeElectionGovernorTest:testRotateNominee() (gas: 510312)
+SecurityCouncilNomineeElectionGovernorTest:testRotateNominee() (gas: 680405)
 SecurityCouncilNomineeElectionGovernorTest:testSetCadenceAfterElections() (gas: 227636)
 SecurityCouncilNomineeElectionGovernorTest:testSetCadenceBeforeFirstElection() (gas: 42502)
 SecurityCouncilNomineeElectionGovernorTest:testSetCadenceInvalidValue() (gas: 26056)

src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol

@@ -62,7 +62,7 @@ contract SecurityCouncilNomineeElectionGovernor is
     ///         Currently this is set to 3 days, assuming 12 blocks per second.
     /// @dev    It is known that a malicious nominee can abuse rotation to avoid vetting,
     ///         but the nominee vetter would always have 3 extra days after any rotation to exclude the nominee if needed.
-    uint256 public constant ROTATION_CUT_OFF_BLOCKS = 21600;
+    uint256 public constant ROTATION_CUT_OFF_BLOCKS = 21_600;
 
     /// @notice Address responsible for blocking non compliant nominees
     address public nomineeVetter;
@@ -211,6 +211,18 @@ contract SecurityCouncilNomineeElectionGovernor is
         electionCount++;
     }
 
+    function _requireNotInOtherCohort(address account) internal view {
+        // check to make sure the contender is not part of the other cohort (the cohort not currently up for election)
+        // this only checks against the current cohort membership of the security council,
+        // so changes to those will mean this check will be inconsistent.
+        // this check then is only a relevant check when the elections are running as expected - one at a time,
+        // every `cadenceInMonths` months. Updates to the sec council manager using methods other than replaceCohort can effect this check
+        // and it's expected that the entity making those updates understands this.
+        if (securityCouncilManager.cohortIncludes(otherCohort(), account)) {
+            revert AccountInOtherCohort(otherCohort(), account);
+        }
+    }
+
     /// @dev Revert if the previous member election has not executed.
     ///      Ensures that there are no unexpected behaviors from multiple elections running at the same time.
     ///      If, for some reason, the previous member election is blocked,
@@ -261,22 +273,14 @@ contract SecurityCouncilNomineeElectionGovernor is
             revert ProposalNotPending(state_);
         }
 
-        // check to make sure the contender is not part of the other cohort (the cohort not currently up for election)
-        // this only checks against the current cohort membership of the security council, 
-        // so changes to those will mean this check will be inconsistent.
-        // this check then is only a relevant check when the elections are running as expected - one at a time,
-        // every `cadenceInMonths` months. Updates to the sec council manager using methods other than replaceCohort can effect this check
-        // and it's expected that the entity making those updates understands this.
-        if (securityCouncilManager.cohortIncludes(otherCohort(), signer)) {
-            revert AccountInOtherCohort(otherCohort(), signer);
-        }
-
+        _requireNotInOtherCohort(signer);
         election.isContender[signer] = true;
 
         emit ContenderAdded(proposalId, signer);
 
         // if the signer is part of the outgoing cohort, we automatically add them as a nominee
         if (securityCouncilManager.cohortIncludes(currentCohort(), signer)) {
+            // no need to check for duplicate nominees as we already checked
             _addNominee(proposalId, signer);
         }
     }
@@ -342,26 +346,13 @@ contract SecurityCouncilNomineeElectionGovernor is
             revert ProposalNotSucceededState(state_);
         }
 
-        if (isNominee(proposalId, account)) {
-            revert NomineeAlreadyAdded(account);
-        }
-
         uint256 cnCount = compliantNomineeCount(proposalId);
         uint256 cohortSize = securityCouncilManager.cohortSize();
         if (cnCount >= cohortSize) {
             revert CompliantNomineeTargetHit(cnCount, cohortSize);
         }
 
-        // can't include nominees from the other cohort (the cohort not currently up for election)
-        // this only checks against the current the current other cohort, and against the current cohort membership
-        // in the security council, so changes to those will mean this check will be inconsistent.
-        // this check then is only a relevant check when the elections are running as expected - one at a time,
-        // every `cadenceInMonths` months. Updates to the sec council manager using methods other than replaceCohort can effect this check
-        // and it's expected that the entity making those updates understands this.
-        if (securityCouncilManager.cohortIncludes(otherCohort(), account)) {
-            revert AccountInOtherCohort(otherCohort(), account);
-        }
-
+        _requireNotInOtherCohort(account);
         _addNominee(proposalId, account);
     }
 
@@ -383,24 +374,19 @@ contract SecurityCouncilNomineeElectionGovernor is
             revert ProposalNotInRotationPeriod(block.number, rotationDeadline);
         }
 
+        if (election.isExcluded[newNomineeAddress]) {
+            revert NomineeAlreadyExcluded(newNomineeAddress);
+        }
+
         address signer = recoverRotateNomineeMessage(proposalId, signature, msg.sender);
         if (signer != newNomineeAddress) {
             revert InvalidSignature();
         }
 
-        // check to make sure the new nominee is not part of the other cohort (the cohort not currently up for election)
-        // this only checks against the current the current other cohort, and against the current cohort membership
-        // in the security council, so changes to those will mean this check will be inconsistent.
-        // this check then is only a relevant check when the elections are running as expected - one at a time,
-        // every 6 months. Updates to the sec council manager using methods other than replaceCohort can effect this check
-        // and it's expected that the entity making those updates understands this.
-        if (securityCouncilManager.cohortIncludes(otherCohort(), newNomineeAddress)) {
-            revert AccountInOtherCohort(otherCohort(), newNomineeAddress);
-        }
-
         // rotation by first excluding the nominee and then adding the new nominee
         election.isExcluded[msg.sender] = true;
         election.excludedNomineeCount++;
+        _requireNotInOtherCohort(newNomineeAddress);
         _addNominee(proposalId, newNomineeAddress);
         emit NomineeExcluded(proposalId, msg.sender);
         emit NomineeRotated(proposalId, msg.sender, newNomineeAddress);

src/security-council-mgmt/governors/modules/SecurityCouncilNomineeElectionGovernorCountingUpgradeable.sol

@@ -121,6 +121,9 @@ abstract contract SecurityCouncilNomineeElectionGovernorCountingUpgradeable is
 
     /// @dev Transitions an account to being a nominee
     function _addNominee(uint256 proposalId, address account) internal {
+        if (isNominee(proposalId, account)) {
+            revert NomineeAlreadyAdded(account);
+        }
         _elections[proposalId].nominees.push(account);
         _elections[proposalId].isNominee[account] = true;
         emit NewNominee(proposalId, account);

test/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.t.sol

@@ -1231,6 +1231,24 @@ contract SecurityCouncilNomineeElectionGovernorTest is Test {
         );
         governor.rotateNominee(proposalId, _contender(1), sig);
 
+        // cannot rotate to existing nominee
+        bytes memory sig2 =
+            sigUtils.signRotateNomineeMessage(proposalId, _contenderPrivKey(2), _contender(0));
+        vm.prank(initParams.nomineeVetter);
+        _mockCohortIncludes(Cohort.SECOND, _contender(2), false);
+        governor.includeNominee(proposalId, _contender(2));
+        _mockCohortIncludes(Cohort.SECOND, _contender(2), false);
+        vm.prank(_contender(0));
+        vm.expectRevert(
+            abi.encodeWithSelector(
+                SecurityCouncilNomineeElectionGovernorCountingUpgradeable
+                    .NomineeAlreadyAdded
+                    .selector,
+                _contender(2)
+            )
+        );
+        governor.rotateNominee(proposalId, _contender(2), sig2);
+
         // rotate the nominee
         _mockCohortIncludes(Cohort.SECOND, _contender(1), false);
         vm.prank(_contender(0));