Arbitrum Security Council Election System - 0xnev's results

Findings

[L-01] Implement a proposal cap for security council member removal proposals

Impact

SecurityCouncilMemberRemovalGovernor.sol#L106-L143

In SecurityCouncilMemberRemovalGovernor.propose(), There is no proposal cap, so anybody who meets proposal threshold can spam and propose member removal proposals as long as member to remove is currently in a existing cohort. This differs from nominee and member election proposals, where a proposal can only be proposed bi-anually.

While such proposals still need to go through the standard governance voting process, allowing uncapped proposing of proposals can affect the UI/UX of Arbitrum DAO voting site.

Recommendation

Implement a minimum proposal cap for each proposer. Many DAOS only allow one active proposal per proposer.

[L-02] Sudden Swing of Votes for member removal proposal

Impact

SecurityCouncilMemberRemovalGovernor.sol#L163-L174

When casting votes for member removal proposals, against/for voters can wait till the last minute to cast votes, resulting in no time for voters of the other support group to react. This leads to preventing proposal execution/causing immediate execution of proposals. Given there is no queuing and vetoeing mechanism, this scenario could be abused to prevent execution of proposal or cause immediate execution of proposal.

Recommendation

Consider implementing similar decreasing voting power to disincentvize late voting.

If not, add a objection period where a sudden swing in votes from Succeeded to Defeated will allow for voters to vote and react to sudden change in state of proposal.

[L-03] SecurityCouncilNomineeElectionGovernor.includeNominee(): Missing onlyVettingPeriod() modifier

Impact

SecurityCouncilNomineeElectionGovernor.sol#L290-L317

In the nominee election phase, the appointed nominee vetter can manually include nominees via SecurityCouncilNomineeElectionGovernor.includeNominee() without having to go through a proposal.

However, the onlyVettingPeriod() modifier is missing, and as such the vetting deadline is not checked, meaning the nominee vetter can still include nominees even after vetting deadline has passed.

While it is acknowledged in test file SecurityCouncilNomineeElectionGovernor.t.sol that includeNominee should succeed even after vetting period, ARB DAO should consider only allowing inclusion of nominees before vetting period ends to ensure fairness, given exclusion can only be performed within vetting period.

Recommendation

- function includeNominee(uint256 proposalId, address account) external onlyNomineeVetter {
+ function includeNominee(uint256 proposalId, address account) external onlyNomineeVetter onlyVettingPeriod(proposalId) {    
    ProposalState state_ = state(proposalId);
    if (state_ != ProposalState.Succeeded) {
        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 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(), account)) {
        revert AccountInOtherCohort(otherCohort(), account);
    }

    _addNominee(proposalId, account);
}

[L-04] SecurityCouncilNomineeElectionGovernor.includeNominee(): Missing check adhering to constitution

Impact

SecurityCouncilNomineeElectionGovernor.sol#L290-L317

function includeNominee(uint256 proposalId, address account) external onlyNomineeVetter {
    ProposalState state_ = state(proposalId);
    if (state_ != ProposalState.Succeeded) {
        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 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(), account)) {
        revert AccountInOtherCohort(otherCohort(), account);
    }

    _addNominee(proposalId, account);
}

Based on consitution, nominee vetter can only add member of the outgoing security council randomly selected as evident by the code comments and docs:

In the event that fewer than six candidates are supported by pledged votes representing at least 0.2% of all Votable Tokens, the current Security Council members whose seats are up for election may become candidates (as randomly selected out of their Cohort) until there are 6 candidates.

While nominee vetter is a trusted role and additional nominee added still need to undergo standard governance voting procedure, an additional check could be made to track the current cohort members to ensure that the nominee added to supplement insufficient cohort must be from the current cohort which adheres to the constitution rules. It also prevents malicious nominee vetters from adding other nominees.

Proof of Concept

Add the following test in SecurityCouncilNomineeElectionGovernor.t.sol and run forge test --match-test testIncludeNomineeNotFromCurrentCohort

function testIncludeNomineeNotFromCurrentCohort() public {
    // Create nominee election proposal
    uint256 proposalId = _propose();

    // According to ARB constitution, following call to includeNominee
    // should fail if nominee to add is not part of current cohort but passes
    _mockCohortIncludes(Cohort.SECOND, _contender(1), false);
    _mockCohortIncludes(Cohort.FIRST, _contender(1), false);
    vm.prank(_contender(1));
    governor.addContender(proposalId);
    vm.roll(governor.proposalVettingDeadline(proposalId) + 1);
    vm.prank(initParams.nomineeVetter);
    governor.includeNominee(proposalId, _contender(1)); 

    // Check state
    assertEq(governor.isNominee(proposalId, _contender(1)), true);
}

Output:

Running 1 test for test/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.t.sol:SecurityCouncilNomineeElectionGovernorTest
[PASS] testIncludeNomineeNotFromCurrentCohort() (gas: 252877)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.76ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Recommendation

function includeNominee(uint256 proposalId, address account) external onlyNomineeVetter {
    ProposalState state_ = state(proposalId);
    if (state_ != ProposalState.Succeeded) {
        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 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(), account)) {
        revert AccountInOtherCohort(otherCohort(), account);
    }

+   if (!securityCouncilManager.cohortIncludes(currentCohort(), account)) {
+       revert AccountNotInCurrentCohort(otherCohort(), account);
+   }

    _addNominee(proposalId, account);
}

[NC-01] SecurityCouncilNomineeElectionGovernor._requireLastMemberElectionHasExecuted(): Cannot create another election unless previous election is executed

Impact and Recommendation

SecurityCouncilNomineeElectionGovernor.sol#L163

To avoid having to undergo a contract upgrade in the event of previously blocked elections due to a lack of nominees, consider creating another type of proposal specifically for governance to resolve previously blocked proposals by extending proposal vetting deadlines

[NC-02] No queuing and vetoer mechanism for proposals

Impact and Recommendation

Consider adding a queuing period and vetoer role before execution as a fail-safe and last check of member election proposal instead of allowing immediate execution after voting and vetting periods.

[R-01] SecurityCouncilManager._removeMemberFromCohortArray(): No need to loop through both cohort

Impact and Recommendation

SecurityCouncilManager.sol#L161-L173

Since a member cannot be in both cohorts at the same time, use the enum flag Cohort to indicate in removeMember(), replaceMember() and rotateMember() in which cohort the member will be removed via _removeMemberFromCohortArray() instead of looping through both cohorts.

---

Consider not allowing removing of member when he is currently put up for election

1. Analysis of Codebase

1.1 Summary

In Arbritrum, there exists 2 types of actors that can propose to make changes to the Arbritrum ecosystem:

1. Arbritrum DAO: Regular delegatees/holders of ARB token
2. Arbritrum Security Council: A council of 12 members controlling a multisig that can make changes to ARB ecosystem in the event of emergencies.

This analysis focuses on the Arbritrum Security Council Election, where every 6-months, an election is held where 6 council members spots of the existing 12 council members are put up for election, giving delegatee (voters) power to decide on new members of the security council.

1.2 Actors

There are 4 main actors in the Arbritrum Security Council Ecosystem:

1.2.1 Contender

Contendor is someone who can register themselves during the nominee slection phase and receive votes from delegatee/voter

1.2.2 Nominee

Nominee is a contendor who has received the minimum 0.2% threshold of votable tokens in the nominee selection phase

1.2.2a Compliant Nominee

A compliant nominee is one who has successfully pass the compliance stage by the nominee vetter after meeting the minimum 0.2% threshold of votable tokens during the nominee selection phase

1.2.2b Noncompliant Nominee

A noncompliant nominee is one who has failed to pass the compliance stage by the nominee vetter even after meeting the minimum 0.2% threshold of votable tokens during the nominee selection phase, and is excluded by the nominee vetter to continue to the member election phase

1.2.3 Member

After the nominee compliant phase, the top 6 nominees that receive the most votes casted by delegatees/voters during the membership election phase wiill be elected as the new members of the Arbritrum Security Council

1.2.4 Delegatee/Voter

Delegatee/voters (holders of ARB token) cast votes during 3 of the 4 existing phases.

First during the nominee selection phase, where contendors are selected to become nominees for election to become members.

Second, during the member election phase, where votes casted during the nominee selection phase by delegatees/voters is resetted to be recasted during this phase to decide on the top 6 nominees to be elected into the Arbritrum Security Council.

Third, during a separate membership removal proposal phase specifically for removing an existing member.

1.3 Phases

1.3.1 Nominee Election Phase

This is the phase that spans 7 days, where the top 6 contendors with the most votes voted by delgatees/voters are selected to potentially enter the membership election phase, provided they pass the compliance checks during the nominee compliance phase conducted by the Arbitrum DAO nominee vetter commitee.

1.3.2 Nominee Compliance Phase

This phase spans 14 days, where compliance checks are conducted on potential contendors selected as nominees by nominee vetting commitee. If any nominee fails potential compliance check resulting in the fixed cohort size of 6 for member election phase not being met, the nominee vetting committee can manually include nominees to ensure that cohort size is met.

1.3.3 Member Election Phase

1.3.3a Full Weight Voting

After the nominee compliance phase, any votes casted during the first 7 days of the member election phase will account for full weight towards nominee selected.

1.3.3b Linearly Decreasing Weight Voting

After the first 7 days of the Member election phase has passed, any votes casted during the next 14 days of the member election phase will have a linearly decreasing weight to incentivize early voting and disincentivize late voting. This is presumably to avoid any sudden swings in votes for nominees.

1.3.4 Member Removal Phase

This is a separate phase where proposals can be proposed to call exlcusively, SecurityCouncilManager.removeMember(), to remove a member from the security council, where thereafter a spot on the council would be guranteed to be opened for election.

2. Architecture Improvements

2.1 No queuing and vetoer mechanism

Consider adding a queuing period and vetoer role before execution as a fail-safe and last check of proposal instead of allowing immediate execution after voting and vetting periods.

2.2 Implement a proposal cap for security council member removal proposals

In SecurityCouncilMemberRemovalGovernor.propose(), There is no proposal cap, so anybody who meets proposal threshold can spam and propose member removal proposals as long as member to remove is currently in a existing cohort. This differs from nominee and member election proposals, where a proposal can only be proposed bi-anually.

While such proposals still need to go through the standard governance voting process, allowing uncapped proposing of proposals can affect the UI/UX of Arbitrum DAO voting site.

2.3 SecurityCouncilNomineeElectionGovernor._requireLastMemberElectionHasExecuted(): Cannot create another election unless previous election is executed

To avoid having to undergo a contract upgrade in the event of previously blocked elections due to a lack of nominees, consider creating another type of proposal specifically for governance to resolve previously blocked proposals by extending proposal vetting deadlines

2.4 Sudden Swing of Votes for member removal proposal

When casting votes for member removal proposals, against/for voters can wait till the last minute to cast votes, resulting in no time for voters of the other support group to react. This leads to preventing proposal execution/causing immediate execution of proposals. Given there is no queuing and vetoeing mechanism, this scenario could be abused to prevent execution of proposal or cause immediate execution of proposal.

2.5 Consider adding a check to ensure that inclusion of new nominee is part of current cohort

Based on consitution, nominee vetter can only add member of the outgoing security council randomly selected as evident by the code comments and docs:

In the event that fewer than six candidates are supported by pledged votes representing at least 0.2% of all Votable Tokens, the current Security Council members whose seats are up for election may become candidates (as randomly selected out of their Cohort) until there are 6 candidates.

While nominee vetter is a trusted role and additional nominee added still need to undergo standard governance voting procedure, an additional check could be made to track the current cohort members to ensure that the nominee added to supplement insufficient cohort must be from the current cohort which adheres to the constitution rules and prevent malicious nominee vetters from adding other nominees.

3. Centralization Risks

3.1 Nominee Vetter in SecurityCouncilNomineeElectionGovernor.sol

• excludeNominee(): Appointed nominee vetter can exclude any nomineee to be put up for election even if contendor meet the minimum threshold (0.2% votes) to become nominee

<br/>

• includeNominee(): Appointed nominee vetter can include any contendor to become nominee and contest for election even if contendor do not meet the minimum threshold (0.2% votes) to qualify to be a nominee.

3.2 Security Council

The 12 member security council is in itself a centralized entity, who are trusted to make emergency changes to the overall Arbritrum system without having to go through DAO proposals.

4. Time Spent

• Day 1: Understand protocol flow and mechanisms
• Day 2: Audit contracts based on protocol flow and mechanism stated in 4.1
• Day 3: Finish up analysis and QA report

4.1 Contract Flow

1. SecurityCouncilNomineeElectionGovernor.sol: Nominee election phase and nominee compliance phase
2. SecurityCouncilMemberElectionGovernor.sol: Member election phase
3. SecurityCouncilMemberRemovalGovernor.sol: Member removal phase
4. SecurityCouncilManager.sol: Executes cross-chain changes to security councils after proposals has passed

Time spent:

16 hours