code423n4 commented 1 year ago

Lines of code

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/modules/ArbitrumGovernorVotesQuorumFractionUpgradeable.sol#L31-L35

Vulnerability details

Bug Description

In ArbitrumGovernorVotesQuorumFractionUpgradeable, the getPastCirculatingSupply() function is used when calculating quorum for proposals:

ArbitrumGovernorVotesQuorumFractionUpgradeable.sol#L31-L35

    /// @notice Get "circulating" votes supply; i.e., total minus excluded vote exclude address.
    function getPastCirculatingSupply(uint256 blockNumber) public view virtual returns (uint256) {
        return
            token.getPastTotalSupply(blockNumber) - token.getPastVotes(EXCLUDE_ADDRESS, blockNumber);
    }

As seen from the natspec comment above, getPastCirculatingSupply() is supposed to return the circulating votes supply (ie. total number of votes - excluded votes).

However, it uses the getPastTotalSupply() function, which returns the token's total supply instead:

ERC20VotesUpgradeable#L88

    /**
     * @dev Retrieve the `totalSupply` at the end of `blockNumber`. Note, this value is the sum of all balances.
     * It is but NOT the sum of all the delegated votes!
     *
     * Requirements:
     *
     * - `blockNumber` must have been already mined
     */
    function getPastTotalSupply(uint256 blockNumber) public view virtual override returns (uint256) {
        require(blockNumber < block.number, "ERC20Votes: block not yet mined");
        return _checkpointsLookup(_totalSupplyCheckpoints, blockNumber);
    }

This becomes an issue as it is possible for a user to hold tokens, but have no votes, causing total supply to be greater than the actual number of votes. For example, if a user delegates his votes to address(0), his tokens will have no corresponding votes as the zero address cannot hold votes:

ERC20VotesUpgradeable.sol#L225

            if (dst != address(0)) {
                (uint256 oldWeight, uint256 newWeight) = _writeCheckpoint(_checkpoints[dst], _add, amount);
                emit DelegateVotesChanged(dst, oldWeight, newWeight);
            }

As such, total supply will now be larger than the total number of votes.

In this scenario, the user's tokens should not be included in the amount used to calculate quorum. This has been confirmed by the sponsor as well:

However, getPastCirculatingSupply() will still count his tokens as it uses total supply. As such, the calculation for quorum is now inaccurate.

Impact

As getPastCirculatingSupply() uses the total supply of ARB tokens instead of the circulating votes supply, quorum calculation is now inaccurate.

This affects both the SecurityCouncilNomineeElectionGovernor and SecurityCouncilMemberRemovalGovernor contracts, as nominee and member removal elections require proposals to pass quorum (0.2% and 10% respectively).