Arbitrum Security Council Election System - Udsen's results

1. LACK OF INPUT VALIDATION FOR ADDRESSES WHEN ASSIGNING IN THE initialize FUNCTION OF THE SecurityCouncilManager CONTRACT

The SecurityCouncilManager.initialize function initializes the firstCohort and secondCohort arrays. But there is no input address validation performed inside the initialize function. Hence it is recommended to perform address(0) check for each of the cohort member addresses before the assignment to the state array variable.

Similarly the SecurityCouncilNomineeElectionGovernor.setNomineeVetter function, there is no check to verify that the _nomineeVetter != address(0). Since the nomineeVetter is a critical role in this protocol it is recommeneded to add this check to the setNomineeVetter function.

https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/SecurityCouncilManager.sol#L100-L101 https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol#L246-250

2. USE TWO-STEP OWNERSHIP TRANSFER

The SecurityCouncilNomineeElectionGovernor contract inherits from the openzeppelin OwnableUpgradeable contract to transfer the ownership to a particular user. But the ownership transfer process of the OwnableUpgradeable is single step and prone to DoS if errorneous address is set as the owner.

Hence it is recommended to use the openzeppelin Ownable2StepUpgradeablecontract to use two step ownership transfer process for safe ownership transfers.

https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol#L23 https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol#L112

3. includeNominee FUNCTION DOES NOT IMPLEMENT THE onlyVettingPeriod MODIFIER

The SecurityCouncilNomineeElectionGovernor.includeNominee function is used to allow the nomineeVetter to explicitly include a nominee if there are fewer nominees than the target. The function has the onlyNomineeVetter modifier which limits the function to be called only by the nomineeVetter. But this function lacks the time limitation which is imposed via the onlyVettingPeriod modifier. Hence this function can be called by the nomineeVetter any time as long as the state_ == ProposalState.Succeeded condition is true. But the nomineeVetter modifier functionality is added to the SecurityCouncilNomineeElectionGovernor.excludeNominee function.

Hence it is recommended to add the nomineeVetter modifier to the includeNominee function as well to restrict the authority of the nomineeVetter and reduce the centralization risk.

https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol#L290

4. DISCREPENCY IN THE NATSPEC COMMENTS AND FUNCTION IMPLEMENTATION

In the SecurityCouncilMemberElectionGovernorCountingUpgradeable.votesToWeight function the following natspec comment is given.

///         Each vote has weight 1 until the fullWeightVotingDeadline is reached, after which each vote has linearly
///         deacreasing weight, reaching 0 at the proposalDeadline.

It mentions that at the proposalDeadline the weight will reach 0.

But it is not correctly implemented in the logic implementation as shown below. The weight is returned as 0 only after the proposal deadline has passed and not at the proposal deadline.

    uint256 endBlock = proposalDeadline(proposalId);
    if (blockNumber > endBlock) {
        return 0;
    }

Above should be corrected as follows:

    uint256 endBlock = proposalDeadline(proposalId);
    if (blockNumber => endBlock) {
        return 0;
    }

https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/modules/SecurityCouncilMemberElectionGovernorCountingUpgradeable.sol#L223-L224 https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/modules/SecurityCouncilMemberElectionGovernorCountingUpgradeable.sol#L236-L239

5. THE initialize FUNCTIONS COULD BE FRONT RUN BY A MALICIOUS USER

The critical contracts of this system are upgradeable and hence have implemented the initialize function. For example the critical contracts such as SecurityCouncilMemberElectionGovernor, SecurityCouncilNomineeElectionGovernor uses the initialize functions.

But these initialize functions do not have any access control and hence can be front run by a malicious user thus prompting a redeployment of the contracts.

Hence it is recommended to add access control to the initialize functions of these critical functions.

https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol#L103-L131 https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilMemberElectionGovernor.sol#L48-L76

6. isContract COULD RETURN false WHEN WORKING WITH CREATE2 TO DETERMINE A CONTRACT ADDRESS

The isContract function of the Openzeppeling Address.sol library is used in multiple locations of this system to verify whether the passed in address to a function is indeed a contract. For example the following code snippet of the SecurityCouncilManager._setUpgradeExecRouteBuilder function shows how the isContract is used.

function _setUpgradeExecRouteBuilder(UpgradeExecRouteBuilder _router) internal {
    address routerAddress = address(_router);

    if (!Address.isContract(routerAddress)) {
        revert NotAContract({account: routerAddress});
    }

    router = _router;
    emit UpgradeExecRouteBuilderSet(routerAddress);
}

But the issue here is that if the routerAddress is created via the CREATE2 and passed the address to this function the contract might not yet be deployed. Hence the isContract will revert since the EXTCODESIZE opcode will return 0. But the contract to the routerAddress will be deployed at a later point in time.

Due to the transaction revert the users will be confused. The same issue could exist in other contracts such as SecurityCouncilMemberRemovalGovernor, SecurityCouncilMemberElectionGovernor etc ...

Hence it is recommended to inform the users of this protocol of the possibility of reverts when CREATE2 is used to determine the contract addresses before the contract is deployed to the blockchain. This will enhance the user experience when interacting with the function which use the isContract functionality.

https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/SecurityCouncilManager.sol#L112-L114 https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/SecurityCouncilManager.sol#L318-L320

7. TYPO'S IN THE NATSPEC COMMENTS SHOULD BE CORRECTED

Following typos in the natspec comments should be corrected for ease of understanding and readability.

    // system will not be blocked if a retryable ticket is `expires`
	

Above comment should be corrected as follows:

    // system will not be blocked if a retryable ticket is `expired`

https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/SecurityCouncilMemberSyncAction.sol#L41

    // when nonce is too `now`, we simply return, we don't revert.
	

Above comment should be corrected as follows:

    // when nonce is too `low`, we simply return, we don't revert.

https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/SecurityCouncilMemberSyncAction.sol.sol#L44

1. DEFAULT VALUE ASSIGNMENT TO VARIABLES CAN BE OMITTED TO SAVE GAS

In the SecurityCouncilManager contract there multiple occasions inside the for loop where the default value of 0 assigned to the i variable. Since i is by default initialzied to 0 it is not required explicitly assign the 0 value to i variable inside the for loops. This will save gas.

    for (uint256 i = 0; i < _newCohort.length; i++) {
        _addMemberToCohortArray(_newCohort[i], _cohort);
    }

Above can be modified as follows:

    for (uint256 i; i < _newCohort.length; i++) {
        _addMemberToCohortArray(_newCohort[i], _cohort);
    }

https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/SecurityCouncilManager.sol#L135-L137 https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/SecurityCouncilManager.sol#L118-L120 https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/SecurityCouncilManager.sol#L164 https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/SecurityCouncilManager.sol#L162 https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/SecurityCouncilManager.sol#L392

2. VARIABLES CAN BE DECLARED OUTSIDE THE for LOOPS TO SAVE GAS

In the SecurityCouncilManager contract, there are multiple occassions where the local variables are declared inside the for loops. But it is recommedded to declare these variables outside the for loops and use them inside later.

    for (uint256 i = 0; i < _securityCouncils.length; i++) {
        _addSecurityCouncil(_securityCouncils[i]);
    }

Above for loop can be modified as follows:

    uint256 i;
    for ( i; i < _securityCouncils.length; i++) {
        _addSecurityCouncil(_securityCouncils[i]);
    }

https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/SecurityCouncilManager.sol#L118-L120

3. THE ENTIRE for LOOP FOR SECURITY COUNCIL EXISTENCE CHECK CAN BE REPLACED WITH A SINGLE TWO DIMENSIONAL MAPPING

The SecurityCouncilManager._addSecurityCouncil function is used to add a new SecurityCouncilData element to the securityCouncils array. Before adding the SecurityCouncilData element there is a check performed to verify that new SecurityCouncil does no already exist in the securityCouncils array as show below:

    for (uint256 i = 0; i < securityCouncils.length; i++) { //@audit-issue - this entire for loop can be replaced with a two dimensional mapping(uint256 => mapping(address => bool))
        SecurityCouncilData storage existantSecurityCouncil = securityCouncils[i];
        if (
            existantSecurityCouncil.chainId == _securityCouncilData.chainId
                && existantSecurityCouncil.securityCouncil == _securityCouncilData.securityCouncil
        ) {
            revert SecurityCouncilAlreadyInRouter(_securityCouncilData);
        }
    }

As it is seen above this check requires to iterate through an entire for loop for securityCouncils.length number of iteration. Hence this operation is extremly gas consuming. Further there are multiple SLOAD operations happening inside a single iteration as well.

Hence it is recommended to replace the entire for loop with a two dimensional mapping(uint256 => mapping(address => bool)). Here for each new SecurityCouncilData element added to the securityCouncils array the chainId and securityCouncil will be stored in the mapping with boolean value of true.

So when adding a new SecurityCouncilData element later, this mapping can be checked for the boolean value. If the boolean value is true, it means the new SecurityCouncilData element is already in the secuirtyCouncils array and hence the transaction should revert.

https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/SecurityCouncilManager.sol#L251-L260

4. storage VARIABLE DECLARATIONS CAN BE REPLACED WITH memory variables.

In the SecurityCouncilManager._addSecurityCouncil the existantSecurityCouncil variable is declared as a storage variable. But this variable is used only to read from and used to modify and value. Hence the existantSecurityCouncil variable can be declared as a memory variable to save gas on multiple SLOAD and SSTORE operations.

https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/SecurityCouncilManager.sol#L255-L256

5. RECOMMENDED TO CACHE THE STORAGE ARRAY LENGTH AND USE THE VALUE FROM MEMORY

The SecurityCouncilManager.getScheduleUpdateInnerData function uses the securityCouncils.length value multiple times within the function scope. This is multiple calls to the SLOAD operation. Hence it is recommended to cache the securityCouncils.length value into memory variable and use it from memory for the operations inside the getScheduleUpdateInnerData function.

This will save gas on multiple SLOAD operations.

https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/SecurityCouncilManager.sol#L388-L413

6. EXEUCTION FLOW OF THE SecurityCouncilMemberSyncAction.perform FUNCTION SHOULD BE CHANGED TO SAVE GAS

The SecurityCouncilMemberSyncAction.perform function is used to update members of security council multisig to match the provided array. There are two for loops in the function to modify the members of the security council.

    for (uint256 i = 0; i < _updatedMembers.length; i++) {
        address member = _updatedMembers[i];
        if (!securityCouncil.isOwner(member)) {
            _addMember(securityCouncil, member, threshold);
        }
    }

As shown above the first for loop is used to add the new members which are non-existent in the current owner array to the security council.

    for (uint256 i = 0; i < previousOwners.length; i++) {
        address owner = previousOwners[i];
        if (!SecurityCouncilMgmtUtils.isInArray(owner, _updatedMembers)) {
            _removeMember(securityCouncil, owner, threshold);
        }
    }

The second for loop as shown above is used to remove the current owner in the security array who are not in the updated members list.

The _removeMember function call of the second for loop calls the getPrevOwner function, which gets the current list of the owners of the security council as shown below:

    address[] memory owners = securityCouncil.getOwners(); 

But this owners list includes the newly added members through the first for loop. Hence the owner.length is higher which consumes more gas. Hence the execution flow of the two for loops should interchange. Which would remove the existing owners who are not in the updated members list first. This will make the owner.lenght comparatively smaller thus saving gas since less number of iterations to execute.

https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/SecurityCouncilMemberSyncAction.sol#L60-L65 https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/SecurityCouncilMemberSyncAction.sol#L67-L72 https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/SecurityCouncilMemberSyncAction.sol#L103-L111

7. FOR READ ONLY OPERATIONS IT IS RECOMMENDED TO USE memory VARIABLE INSTEAD OF storage VARIABLE

In the SecurityCouncilMemberElectionGovernorCountingUpgradeable.topNominees function the respective election struct from the _elections mapping is retrieved as follows:

    ElectionInfo storage election = _elections[proposalId]; 

Even though the election is a storage variable it is only readfrom within the topNominees function and not written into. Hence a memory variable can be used in place of the storage variable to save gas as shown below:

    ElectionInfo memory election = _elections[proposalId]; 

https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/modules/SecurityCouncilMemberElectionGovernorCountingUpgradeable.sol#L180