SecurityCouncilNomineeElectionGovernor vetting period will be shorten each election

[code423n4]

Lines of code

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol##L151-#L153

Vulnerability details

Impact

• SecurityCouncilNomineeElectionGovernor vetting period will be shorten each election
• No function to update vetting period

Proof of Concept

The document for nominee election https://forum.arbitrum.foundation/t/proposal-security-council-elections-proposed-implementation-spec/15425/1#election-stages-in-detail-9 specifies the Nominee selection is 7 days long and Compliance check by the Foundation is 14 days long. However, contract SecurityCouncilNomineeElectionGovernor always expresses these durations using block numbers. For example the onlyVettingPeriod modifier:

modifier onlyVettingPeriod(uint256 proposalId) {
        // voting is over and the proposal must have succeeded, not active or executed
        ProposalState state_ = state(proposalId);
        if (state_ != ProposalState.Succeeded) {
            revert ProposalNotSucceededState(state_);
        }

        // the proposal must not have passed the vetting deadline
        uint256 vettingDeadline = proposalVettingDeadline(proposalId);
        if (block.number > vettingDeadline) {
            revert ProposalNotInVettingPeriod(block.number, vettingDeadline);
        }

        _;
    }

According to the stats here, https://ycharts.com/indicators/ethereum_blocks_per_day, the blocks per day in Ethereum has increased by 11.4% from last year. This means if for example vetting period is set a number of blocks for 14 days in 2022, by now it's only equal to 12 days; which means the vetting duration is shorten by 2 days.

I understand that the contract inherit openzeppelin's GovernorUpgradeable contract and it uses block.number by default. However, the impact is still that these periods will be shorten. An important thing I want to mention here is that variable nomineeVettingDuration in contract SecurityCouncilNomineeElectionGovernorTiming is only set at initialization, and there's no other function to update it:

function __SecurityCouncilNomineeElectionGovernorTiming_init(
        Date memory _firstNominationStartDate,
        uint256 _nomineeVettingDuration
    ) internal onlyInitializing {
        ...
        firstNominationStartDate = _firstNominationStartDate;
        nomineeVettingDuration = _nomineeVettingDuration;
    }

Unlike fullWeightDuration in SecurityCouncilMemberElectionGovernorCountingUpgradeable (this is the Member election period documented in this https://forum.arbitrum.foundation/t/proposal-security-council-elections-proposed-implementation-spec/15425/1#h-3-member-election-21-days-14), can be updated using function setFullWeightDuration

    function setFullWeightDuration(uint256 newFullWeightDuration) public onlyGovernance {
        if (newFullWeightDuration > votingPeriod()) {
            revert FullWeightDurationGreaterThanVotingPeriod(newFullWeightDuration, votingPeriod());
        }

        fullWeightDuration = newFullWeightDuration;
        emit FullWeightDurationSet(newFullWeightDuration);
    }

Tools Used

Manual review

Recommended Mitigation Steps

If you have the intention to keep using block numbers as durations, I think the best way is to provide a function to update nomineeVettingDuration similar to setFullWeightDuration. After that, all 3 variables related to durations votingPeriod, nomineeVettingDuration and setFullWeightDuration can be updated. Elections only happen every 6-12 months so prior to every election, an authorized user can update these values using latest average block number per day so that those durations will matched their specification.

Assessed type

Governance

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[c4-sponsor]

yahgwai marked the issue as sponsor acknowledged

[c4-sponsor]

yahgwai marked the issue as disagree with severity

[c4-sponsor]

yahgwai marked the issue as sponsor confirmed

[yahgwai]

Fix: https://github.com/ArbitrumFoundation/governance/pull/187. I think it is useful to point out that if the L1 block time changes then an upgrade will need to be made to the system to change the block periods. This is, however, the expected behaviour of the system. Severity: Low, only change is to add some better documentation.

[0xean]

QA - assumes future changes to average L1 block times which may or may not occur.

[c4-judge]

0xean changed the severity to QA (Quality Assurance)

[c4-judge]

0xean marked the issue as grade-a

[ktg9]

Hi @0xean, @yahgwai , thank you for your responses. The thing I want to emphasize in this finding is that vetting period will be shorten and there is no way to update it. As nomineeVettingDuration in contract SecurityCouncilNomineeElectionGovernorTiming is only initialized, no function to update this time value. As @yahgwai states it is useful to point out that if the L1 block time changes then an upgrade will need to be made to the system to change the block periods; however there is no function to update it, while other time values like fullWeightDuration, votingPeriod are all updatable.

Can you reconsider this finding?

Regards.

[0xean]

Decision is final here.