Governance could accidentally DOS member elections by setting `_votingPeriod` less than `fullWeightDuration`

[code423n4]

Lines of code

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

Vulnerability details

Bug Description

In SecurityCouncilMemberElectionGovernorCountingUpgradeable, setFullWeightDuration() has a check to ensure that fullWeightDuration is more than the voting period:

SecurityCouncilMemberElectionGovernorCountingUpgradeable.sol#L77-L84

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

        fullWeightDuration = newFullWeightDuration;
        emit FullWeightDurationSet(newFullWeightDuration);
    }

However, the setVotingPeriod() function in Openzeppelin's GovernorSettingsUpgradeable module doesn't ensure that _votingPeriod is above fullWeightDuration. This means that governance could accidentally cause fullWeightDuration to be greater than _votingPeriod by calling setVotingPeriod() to decrease the voting period.

Should this occur, votesToWeight() will revert due to an arithmetic underflow when performing the following calculation:

SecurityCouncilMemberElectionGovernorCountingUpgradeable.sol#L241-L249

        // Between proposalSnapshot and fullWeightVotingDeadline all votes will have 100% weight - each vote has weight 1
        uint256 fullWeightVotingDeadline_ = fullWeightVotingDeadline(proposalId);
        if (blockNumber <= fullWeightVotingDeadline_) {
            return _downCast(votes);
        }

        // Between the fullWeightVotingDeadline and the proposalDeadline each vote will have weight linearly decreased by time since fullWeightVotingDeadline
        // slope denominator
        uint256 decreasingWeightDuration = endBlock - fullWeightVotingDeadline_;

Where:

• endBlock is equal to startBlock + _votingPeriod.
• fullWeightVotingDeadline_ is equal to startBlock + fullWeightDuration.

Since votesToWeight() is used to determine the weightage of votes in _countVote(), all voting functions (eg. castVote()) will always revert once fullWeightVotingDeadline_ has passed, causing all voting to be DOSed.

Impact

Governance could accidentally DOS voting for member elections by reducing the voting period below fullWeightDuration using setVotingPeriod().

This could occur if fullWeightDuration is initally equal to votingPeriod (votes have 100% weightage during the entire voting period), and governance decides to reduce the voting period to a shorter duration.

Given that setFullWeightDuration() is also called by governance, and has to be scheduled through timelocks, it might not be possible for governance to call setFullWeightDuration() to reduce fullWeightDuration in time after realizing the DOS has occurred.

Recommended Mitigation

In the SecurityCouncilMemberElectionGovernor contract, consider overriding the setVotingPeriod() function to ensure that the new voting period is always greater than fullWeightDuration. For example:

    function setVotingPeriod(uint256 newVotingPeriod) public override onlyGovernance {
        if (newVotingPeriod <= fullWeightDuration) {
            revert NewVotingPeriodLessThanFullWeightDuration();
        }

        _setVotingPeriod(newVotingPeriod);
    }

Assessed type

Access Control

[0xSorryNotSorry]

__GovernorSettings_init() is called during initializing which calls _setVotingPeriod(initialVotingPeriod).

Hence the voting period is set first and then the weight later.

[c4-pre-sort]

0xSorryNotSorry marked the issue as low quality report

[c4-judge]

0xean marked the issue as unsatisfactory: Invalid

[MiloTruck]

Hi @0xean, could you clarify on why this finding is invalid?

My argument here is that setFullWeightDuration() contains a check that ensures fullWeightDuration isn't larger than votingPeriod, but setVotingPeriod() is missing a check to make sure that votingPeriod is larger than fullWeightDuration.

This could potentially create a scenario where the Arbitrum DAO decreases votingPeriod below fullWeightDuration using setVotingPeriod() (through the relay() function), which would DOS elections.

Thanks!

[0xean]

Thanks for clarifying, still not a M severity as its input sanitization and therefore QA.

[c4-judge]

0xean changed the severity to QA (Quality Assurance)

[c4-judge]

0xean marked the issue as grade-a