Arbitrum Security Council Election System - ktg's results

Lines of code

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/modules/SecurityCouncilNomineeElectionGovernorTiming.sol#L75-#L94

Vulnerability details

Impact

• SecurityCouncilNomineeElectionGovernor might have to wait for more than 6 months to create election again

Proof of Concept

According to the document https://forum.arbitrum.foundation/t/proposal-security-council-elections-proposed-implementation-spec/15425/1#h-1-nominee-selection-7-days-10, security council election can be create every 6 months. Contract SecurityCouncilNomineeElectionGovernor implements this by these codes:

 function createElection() external returns (uint256 proposalId) {
        // require that the last member election has executed
        _requireLastMemberElectionHasExecuted();

        // each election has a deterministic start time
        uint256 thisElectionStartTs = electionToTimestamp(electionCount);
        if (block.timestamp < thisElectionStartTs) {
            revert CreateTooEarly(block.timestamp, thisElectionStartTs);
        }
        ...
}

    function electionToTimestamp(uint256 electionIndex) public view returns (uint256) {
        // subtract one to make month 0 indexed
        uint256 month = firstNominationStartDate.month - 1;

        month += 6 * electionIndex;
        uint256 year = firstNominationStartDate.year + month / 12;
        month = month % 12;

        // add one to make month 1 indexed
        month += 1;

        return DateTimeLib.dateTimeToTimestamp({
            year: year,
            month: month,
            day: firstNominationStartDate.day,
            hour: firstNominationStartDate.hour,
            minute: 0,
            second: 0
        });
    }

If electionIndex = 1, function createElection will call electionToTimestamp to calculate for timestamp 6 months from firstNominationStartDate. However, the code uses firstNominationStartDate.day to form the result day:

        return DateTimeLib.dateTimeToTimestamp({
            year: year,
            month: month,
            day: firstNominationStartDate.day,
            hour: firstNominationStartDate.hour,
            minute: 0,
            second: 0
        });

This could result in wrong calculation because the day in months can varies from 28-31. Therefore, the worst case is that the user has to wait for 6 months + 4 more days to create new election.

For example, if firstNominationStartDate = 2024-08-31-01:00:00 (which is the last day of August). The user might expect that they can create election again 6 months from that, which mean 1:00 AM of the last day of February 2025 (which is 2025-02-28-01:00:00), but in fact the result of electionToTimestamp would be 2025-03-03-01:00:00, 4 days from that.

Below is POC for the above example, for easy of testing, place this test case in file test/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.t.sol under contract SecurityCouncilNomineeElectionGovernorTest and run it using command: forge test --match-path test/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.t.sol --match-test testDateTime -vvvv

    function testDateTime() public {
        // Deploy a new governor
        // with first nomination start date = 2024-08-30T01:00
        SecurityCouncilNomineeElectionGovernor  newGovernor = _deployGovernor();

        SecurityCouncilNomineeElectionGovernor.InitParams memory newInitParams
        =	SecurityCouncilNomineeElectionGovernor.InitParams({
            firstNominationStartDate: Date({year: 2024, month: 8, day:31, hour:1}),
            nomineeVettingDuration: 1 days,
            nomineeVetter: address(0x11),
            securityCouncilManager: ISecurityCouncilManager(address(0x22)),
            securityCouncilMemberElectionGovernor: ISecurityCouncilMemberElectionGovernor(
                payable(address(0x33))
            ),
            token: IVotesUpgradeable(address(0x44)),
            owner: address(0x55),
            quorumNumeratorValue: 20,
            votingPeriod: 1 days
        });

        // The next selection is not available until timestamp 1740963600,
        // which is 2025-03-03T1:00:00 AM GMT
        newGovernor.initialize(newInitParams);
        assertEq(newGovernor.electionToTimestamp(1), 1740963600);

    }

You can use an online tool like https://www.epochconverter.com/ to check that 1740963600 is Monday, March 3, 2025 1:00:00 AM GMT

Tools Used

Manual review

Recommended Mitigation Steps

I recommend fixing the math so that the duration between elections are exactly 6 months like documented.

Assessed type

Math

Low issues

L-01: Insufficient check for cohorts initialization in SecurityCouncilManager.initialize

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/SecurityCouncilManager.sol#L89

function initialize(
        address[] memory _firstCohort,
        address[] memory _secondCohort,
        SecurityCouncilData[] memory _securityCouncils,
        SecurityCouncilManagerRoles memory _roles,
        address payable _l2CoreGovTimelock,
        UpgradeExecRouteBuilder _router
    ) external initializer {
        if (_firstCohort.length != _secondCohort.length) {
            revert CohortLengthMismatch(_firstCohort, _secondCohort);
        }
        firstCohort = _firstCohort;
        secondCohort = _secondCohort;
        cohortSize = _firstCohort.length;
        ...

As you can see the only check on firstCohort and secondCohort is assuring their length are the same. This is insufficient and can cause many issues, for example:

• The firstCohort and secondCohort could be of any size, violating the Arbitrum Constitution https://docs.arbitrum.foundation/dao-constitution#section-3-the-security-council, in which it states The Security Council is a committee of 12 members who are signers of a multi-sig wallet. Since variable cohortSize is set as the length of firstCohort and secondCohort at initialization, a 0 size cohort cannot be replaced by calling replaceCohort or added new member by calling addMember. The protocol will basically fail to function.
• One member could end up in both cohorts
• Duplicates in both cohorts

L-02: SecurityCouncilMemberElectionGovernorCountingUpgradeable.votesToWeight precision loss may lead to users not affecting during decreasing weight period.

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/modules/SecurityCouncilMemberElectionGovernorCountingUpgradeable.sol#L236-#L256 According to the documentation, the member voting process is designed to encourage voters to cast their vote early. Their voting power will eventually decay if they do not cast their vote within the first 7 days:

• 0 - 7 days. Votes cast will carry weight 1 per token
• 7 - 21 days. Votes cast will have their weight linearly decreased based on the amount of time that has passed since the 7 day point. By the 21st day, each token will carry a weight of 0.

The decreasing calculation is implemented as follow:

   uint256 endBlock = proposalDeadline(proposalId);
   uint256 decreasingWeightDuration = endBlock - fullWeightVotingDeadline_;
   uint256 decreaseAmount =
          votes * (blockNumber - fullWeightVotingDeadline_) / decreasingWeightDuration;
   return _downCast(votes - decreaseAmount);

The functions calculates decreaseAmount and then subtract that from original votes. However, since this is the integer division, it may end up = 0 if votes * (blockNumber - fullWeightVotingDeadline_ < decreasingWeightDuration. A malicious user can vote with a small amount and get around this.

L-03: SecurityCouncilNomineeElectionGovernor.sol.includeNominee should check if address != address(0)

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/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);
    }

Function includeNominee allows nomineeVetter to add an account to nominee list of the current list length < cohort size. However, it doesn't check if account is a valid address != address(0). If this happens, when the proposal is passed to SecurityCouncilMemberElectionGovernor and executed, SecurityCouncilManager will not accept this address and reverts, blocking subsequent elections.

L-04: There is no way to update upExecLocations in contract UpgradeExecRouteBuilder

The variable upExecLocations is of type mapping(uint256 => UpExecLocation), it decides which chainId is valid for adding security council member in SecurityConcilManager:

function _addSecurityCouncil(SecurityCouncilData memory _securityCouncilData) internal {
        ...
        if (!router.upExecLocationExists(_securityCouncilData.chainId)) {
            revert SecurityCouncilNotInRouter(_securityCouncilData);
        }
        ...
}

function upExecLocationExists(uint256 _chainId) public view returns (bool) {
        return upExecLocations[_chainId].upgradeExecutor != address(0);
}

This variable upExecLocations is only updated at initialization and no functions to update it after that; therefore, the protocol cannot add new security council with chainId different than what's initialized.

L-05: SecurityCouncilManager.generateSalt could produce the same salt, leading to collisions

The function generateSalt is currently implemented as follow:

    function generateSalt(address[] memory _members, uint256 nonce)
        external
        pure
        returns (bytes32)
    {
        return keccak256(abi.encodePacked(_members, nonce));
    }

The function uses encodePacked. In solidity, encode uses padding while encodePacked does not, this could lead to collisions and replay protections is broken, as this salt is used to create date to send to Arbitrum timelock:

function getScheduleUpdateInnerData(uint256 nonce)
        public
        view
        returns (address[] memory, address, bytes memory)
{
...
 // unique salt used for replay protection in the L1 timelock
        bytes32 salt = this.generateSalt(newMembers, nonce);
        (address to, bytes memory data) = router.createActionRouteData(
            chainIds,
            actionAddresses,
            new uint256[](securityCouncils.length), // all values are always 0
            actionDatas,
            0,
            salt
        );

        return (newMembers, to, data);
...
}