Nominee vetting period can change for existing not yet executed proposals

[code423n4]

Lines of code

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/modules/SecurityCouncilNomineeElectionGovernorTiming.sol#L69-L71 https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol#L332 https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol#L142-L150 https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol#L266-L269

Vulnerability details

Impact

The SecurityCouncilNomineeElectionGovernor contract inherits from SecurityCouncilNomineeElectionGovernorTiming which implements proposalVettingDeadline to retrieve the deadline for the nominee vetting period for a given proposalId. While the GovernorUpgradeable's proposalDeadline correctly stores the proposal parameters with the proposal/proposalId itself, the proposalVettingDeadline uses the nomineVettingDuration (as proposalDeadline(proposalId) + nomineVettingDuration) of the SecurityCouncilNomineeElectionGovernorTiming contract which is initialized at the end of its __SecurityCouncilNomineeElectionGovernorTiming_init initializer and therefore can mutate globally during upgrades affecting all existing proposals (for both the _execute function and the onlyVettingPetiod).

Proof of Concept

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

    function __SecurityCouncilNomineeElectionGovernorTiming_init(
        Date memory _firstNominationStartDate,
        uint256 _nomineeVettingDuration
    ) internal onlyInitializing {

        ...

        firstNominationStartDate = _firstNominationStartDate;
        nomineeVettingDuration = _nomineeVettingDuration;
    }

    ...

    /// @notice Deadline for the nominee vetting period for a given `proposalId`
    function proposalVettingDeadline(uint256 proposalId) public view returns (uint256) {
        return proposalDeadline(proposalId) + nomineeVettingDuration;
    }

From

    /// @dev    `GovernorUpgradeable` function to execute a proposal overridden to handle nominee elections.
    ///         Can be called by anyone via `execute` after voting and nominee vetting periods have ended.
    ///         If the number of compliant nominees is > the target number of nominees,
    ///         we move on to the next phase by calling the SecurityCouncilMemberElectionGovernor.
    /// @param  proposalId The id of the proposal
    function _execute(
        uint256 proposalId,
        address[] memory, /* targets */
        uint256[] memory, /* values */
        bytes[] memory callDatas,
        bytes32 /*descriptionHash*/
    ) internal virtual override {
        // we can only execute when the vetting deadline has passed
        uint256 vettingDeadline = proposalVettingDeadline(proposalId);
        if (block.number <= vettingDeadline) {
            revert ProposalInVettingPeriod(block.number, vettingDeadline);
        }

        ...

    }

Tools Used

n/a

Recommended Mitigation Steps

Store nomineeVettingDuration with the proposal / proposalId.

Assessed type

Governance

[0xSorryNotSorry]

This looks like the intended behaviour even giving more room in setting of the proposal deadlines via nomineeVettingDuration during upgrades.

Could be QA.

[c4-pre-sort]

0xSorryNotSorry marked the issue as low quality report

[c4-judge]

0xean changed the severity to QA (Quality Assurance)

[eierina]

Hey @0xean I think there was an oversight on this one, can you please reconsider?

I don't think we can say this is the "intended behaviour" as this vulnerability can have at least the following impacts:

• Manipulation of Proposal Execution: By changing the nomineeVettingDuration, an attacker (or even an honest actor) can potentially delay or hasten the execution of proposals. Not just the future ones, but retroactively with a side-effect that applies indiscriminately to all active proposals. This can lead to governance decisions being executed at unexpected times.
• Inconsistent State: Existing proposals can be in (or apply to) an inconsistent state where their vetting deadlines have changed.
• Loss of Trust: Governance systems, especially in blockchain environments, rely on trust. If participants see that parameters can be changed after the fact, it can erode trust in the system.

The base OZ GovernorUpgradeable base class is implemented correctly where the state/parameters of each proposal are stored with the proposalId to guarantee the proposal's immutability (including timing) while SecurityCouncilNomineeElectionGovernorTiming's timing is mutable.

[0xean]

I don't believe this qualifies as M, I don't see how an "attacker" can change nomineeVettingDuration. Broadly, this could fall under some "Centralization concerns" that should be OOS based on the bot report. I believe QA is correct here.

[eierina]

@0xean couple of points here:

• I don't think we can categorize an implementation error as Centralization concerns and OOS
• The problem is not who leverages this but the fact that the nomineeVettingDuratio is the amount of time after voting ends that the nomineeVetter can exclude noncompliant nominee. So for example the nomineVetter may think he has 1 week to exclude noncompliant nominess but instead changing the nomineeVettingDuration just passed on Friday night and changed to 48 hours, so that the nomineeVetter will find only on Monday that he cannot exclude noncompliant nominess. If nominess were proposed when vetting period is 1 week, that should not change until the existing proposal is completed and fully vetted.

[0xean]

QA has ended, thanks for your comments. This will remain as judged.