Arbitrum Security Council Election System - berlin-101's results

L-01 SecurityCouncilManager.sol "_addSecurityCouncil" only checks for chain id and security council address

When adding a security council only chain id and security council address are checked to detect whether adding the council is allowed: https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/SecurityCouncilManager.sol#L255-L256

When removing a security council also the address of the update action is checked to make the match: https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/SecurityCouncilManager.sol#L255-L256.

This can mean two things.

1. When removing a security council the check for the update action address is not necessary and can be removed.
2. Not checking for the address of the update action when adding a security council prevents adding more granular associations between a security council and upgrade actions.

When in discussion with the team the question whether this is explicitly documented as a reference for implementation could not be clarified.

L-02 SelectTopNominees() function on SecurityCouncilMemberElectionGovernorCountingUpgradeable.sol favors later entries in the nominees array that have the same weight

I am not sure if that is intended but this line favors later entries in the nominees array passed to the function as the index is added to the weight: https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/modules/SecurityCouncilMemberElectionGovernorCountingUpgradeable.sol#L206

L-03 If the vetter makes a mistake after the vetting deadline, a nominee cannot be removed any more

The "excludeNominee" function of "SecurityCouncilNomineeElectionGovernor.sol" enforces that it can only be called during the vetting period via the "onlyVettingPeriod" modifier: https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol#L269.

However, the "inludeNominee" function does not enforce it as there is no option to extend the vetting period implemented: https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol#L290. So no additional nominees could be added to achieve the candidates after the vetting period ended. This may be implemented as expected.

But if the vetter makes a mistake after the vetting period ended, no nominee added through failure could be removed and the added candidate could be voted for. It is unclear if this is intended. If so, the vetter must be trusted to not make mistakes.

L-04 createActionRouteDataWithDefaults() function in UpgradeExecRouteBuilder is only used in tests

The createActionRouteDataWithDefaults() function within UpgradeExecRouteBuilder.sol (https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/UpgradeExecRouteBuilder.sol#L186) is only used in tests. It should be removed from the contract and moved into the test folder.

L-05 "schedValues" variable should be called "schedActions" instead in UpgradeExecRouteBuilder.sol

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/UpgradeExecRouteBuilder.sol#L124

L-06 Inconsistency in naming between TIMELOCK_PROPOSAL_ROLE and PROPOSER_ROLE

This becomes apparent for example here: https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/GovernanceChainSCMgmtActivationAction.sol#L69. "TIMELOCK_PROPOSER_ROLE" should be used instead. For "TIMELOCK_CANCELLER_ROLE" this is done consistently: https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/GovernanceChainSCMgmtActivationAction.sol#L70.

L-07 "_removeMemberFromCohortArray" function of SecurityCouncilManager.sol could have been written cleaner: _removeMemberFromCohortArray

Suggested implementation:

 function _removeMemberFromCohortArray(address _member) internal returns (Cohort) {
    for (uint256 i = 0; i <= Cohort.SECOND; i++) {
        address[] storage cohort = i == Cohort.FIRST ? firstCohort : secondCohort;
        for (uint256 j = 0; j < cohort.length; j++) {
            if (_member == cohort[j]) {
                cohort[j] = cohort[cohort.length - 1];
                cohort.pop();
                return Cohort(i);
            }
        }
    }
    revert NotAMember(_member);
}

L-08 requireSafesEquivalent() function in SecurityCouncilMgmtUpgradeLib uses underscore prefix inconsistently for its arguments

Safe2 parameter does not use underscore: https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/SecurityCouncilMgmtUpgradeLib.sol#L30-L32C35

L-09 Inconsistency in naming due to using "remover" and "removal" terminology at the same time

Example: https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/factories/L2SecurityCouncilMgmtFactory.sol#L141. This should be harmonized. The suggestion is to use "remover".

L-10 A more compact syntax for errors can be used in some places where error take only 1 parameter

Several examples of this can be found in SecurityCouncilManager.sol: https://github.com/ArbitrumFoundation/governance/blob/security-council-mgmt--base/src/security-council-mgmt/SecurityCouncilManager.sol#L113, https://github.com/ArbitrumFoundation/governance/blob/security-council-mgmt--base/src/security-council-mgmt/SecurityCouncilManager.sol#L149, https://github.com/ArbitrumFoundation/governance/blob/security-council-mgmt--base/src/security-council-mgmt/SecurityCouncilManager.sol#L172, https://github.com/ArbitrumFoundation/governance/blob/security-council-mgmt--base/src/security-council-mgmt/SecurityCouncilManager.sol#L319,

L-11 VoteType enum could have been used consistently for encoding support instead of mixing it with integers

There is a mix of using VoteType and integers to indicate the "support" of votes. For example here the VoteType enum is used: https://github.com/ArbitrumFoundation/governance/blob/security-council-mgmt--base/src/security-council-mgmt/governors/SecurityCouncilMemberRemovalGovernor.sol#L170. Here an integer is used: https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/modules/SecurityCouncilMemberElectionGovernorCountingUpgradeable.sol#L102. Using the enum consistently is preferable.

L-12 If vetter accidentally excluded a nominee it cannot be added again for the election

Via the "excludeNominee" function in SecurityCouncilNomineeElectionGovernor.sol a nominee can be excluded by a vetter.

But because this function blacklists the nominee's address via "election.isExcluded[nominee] = true;" (https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol#L279) the vetter cannot revert this by using the "includeNominee" function as it does not reset this entry (https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol#L290).

If the implementation should be robust to cover for vetter mistakes, resetting the "election.isExcluded" state of the nominee address (and decreasing the election.excludedNomineeCount) should be implemented within the "includeNominee" function.

L-13 Inconsitent way of defining "totalUsedVotes" and "weightReceived" in emitted VoteCastForNominee in SecurityCouncilMemberElectionGovernorCountingUpgradeable.sol

See: https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/modules/SecurityCouncilMemberElectionGovernorCountingUpgradeable.sol#L136-L138

For "totalUsedVotes" the following is used:

totalUsedVotes: prevVotesUsed + votes

For "weightReceived" the following is used:

weightReceived: election.weightReceived[nominee]

which could have been the following to be consistent:

weightReceived: prevWeightReceived + weight

Approach taken in evaluating the codebase

• The documentation was read thoroughly which includes: . The Arbitrum consitution: https://docs.arbitrum.foundation/dao-constitution . The "Security Council Elections Proposed Implementation Spec" proposal: https://forum.arbitrum.foundation/t/proposal-security-council-elections-proposed-implementation-spec/15425 . The architectural overview graphic (which was very helpful): https://raw.githubusercontent.com/ArbitrumFoundation/governance/c18de53820c505fc459f766c1b224810eaeaabc5/docs/security-council-colors.png
• After that, all contracts were sorted by number of lines of code and approached from the smallest number
• During the review the code was heavily documented
• Arbitrum-related issues (e.g. block reorgs, usage of block.number in Arbitrum, etc. were inspected via https://solodit.xyz/)
• Unclear behavior of the protocol was discussed via Discord with protocol members who were very supportive
• Audit markers were left in code to indicate findings and their severity

Codebase quality analysis

• The codebase was in total deemed as mature
• Some inconsistencies in naming variables (e.g. TIMELOCK_PROPOSAL_ROLE vs. PROPOSER_ROLE) were encountered
• Enums were not consistently used throughout the code base (VoteType)
• The excessive amount of inheritance and the associated amount of initializations within a contract makes it hard to keep an overview and easy to miss required initializations (bot results already discovered some of these issues)
• Some parts of the code could use more comments to explain what happens. One good example is the packing of weights and appending the nominee index in the "selectTopNominees" function in SecurityCouncilMemberElectionGovernorCountingUpgradeable.sol (https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/modules/SecurityCouncilMemberElectionGovernorCountingUpgradeable.sol#L191). Here the appended index actually influences the ranking for same weights in nominees which is undocumented.
• The test suite seemed well organized but can be enhanced (currently 94%)
• Several typos in the codebase deminished the impression of a very mature codebase a bit
• There was no sign in the protocol that a static analyzer like slither was used. This is recommened.
• The Makefile was helpful to get started and run tests

Centralization risks

• After the contract deployer admin account the security council with the power for emergency activities (circumventing DAO vote) is the greatest centralization risk
• Some processes happen off-chain and are not transparent for the DAO members (e.g. randomization of council members if a cohort could not be filled). A list of processes that happen off-chain could help establishing trust from DAO members.

Mechanism review

• The implementation of the full election process as a state machine through multiple porposals is good to undertand and transparent
• Enforcing the time delay through a roundtrip from L1 to L2 and back to L1 is a good solution to enable users to opt out.
• Potentially a final time window of 3 days to opt out from the protocol is too low. One would always need to have an ear on what is happening on Arbitrum to not miss this timeframe.
• No precautions are implemented in code against the vetter making mistakes that he cannot correct any more (see the QA findings for details): . removing a nominee cannot be re-added . after the vetting period a nominee cannot be removed anymore These edge cases should be documented. It also implies that the vetter acts with an enourmous level of care. A vetter is also human and human make mistakes. Potentially the implementation of a mechanism to have a small amount of time after an action would allow the vetter to revert his faulty action while maintaining trust from the DAO as it is clear that this is only for mistakes in a small time window.

Learnings

• This was the auditor's first audit of a protocol where a deeper look was taken in the L1 <-> L2 communication
• This was also the auditor's first participation in a contest fully focused on governance
• The way votes are packaged was seen the first time
• Looking into previous audit contest findings around the topics of Arbitrum, Gnosis safes and governance in general helped gather some new knowledge

Time spent:

50 hours