diff --git a/.gas-snapshot b/.gas-snapshot index 59be04bce..b610a96a1 100644 --- a/.gas-snapshot +++ b/.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) diff --git a/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol b/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol index e611f3bf9..f805ff0e6 100644 --- a/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol +++ b/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); diff --git a/src/security-council-mgmt/governors/modules/SecurityCouncilNomineeElectionGovernorCountingUpgradeable.sol b/src/security-council-mgmt/governors/modules/SecurityCouncilNomineeElectionGovernorCountingUpgradeable.sol index 9cc6d7e3c..efbe43206 100644 --- a/src/security-council-mgmt/governors/modules/SecurityCouncilNomineeElectionGovernorCountingUpgradeable.sol +++ b/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); diff --git a/test/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.t.sol b/test/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.t.sol index 8ed5af784..672d23b02 100644 --- a/test/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.t.sol +++ b/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));