DoS at CitadelMinter.sol

[code423n4]

Lines of code

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/CitadelMinter.sol#L250-L284

Vulnerability details

Impact

At CitadelMinter.sol, Funding Pool Weight can't be set at the beginning since totalFundingPoolWeight value is not assigned and Zero meanwhile being cached to _newTotalWeight. Hence the substraction will not perform as it will yield to a negative value which the cached variable is an opposite uint. Please check the @audit notes below;

Proof of Concept

    function setFundingPoolWeight(address _pool, uint256 _weight)
        external
        onlyRole(POLICY_OPERATIONS_ROLE)
        gacPausable
        nonReentrant
    {
        require(
            address(_pool) != address(0),
            "CitadelMinter: address(0) check"
        );

        bool poolExists = fundingPools.contains(_pool);

        // NOTE: Could cachedTotalFundingPoolWeight but honestly logic is already messy enough

        // Remove existing pool on 0 weight
        if (_weight == 0 && poolExists) {
            _removeFundingPool(_pool);

            emit FundingPoolWeightSet(_pool, _weight, totalFundingPoolWeight);
        } else if (_weight > 0) {
            // Add new pool or modify existing pool
            require(_weight <= 10000, "exceed max funding pool weight");
            if (!poolExists) {
                _addFundingPool(_pool);
            }
            uint256 _newTotalWeight = totalFundingPoolWeight; // @audit totalFundingPoolWeight is Zero
            _newTotalWeight = _newTotalWeight - fundingPoolWeights[_pool]; // @audit <--- This substraction will not perform
            fundingPoolWeights[_pool] = _weight;
            _newTotalWeight = _newTotalWeight + _weight;
            totalFundingPoolWeight = _newTotalWeight;

            emit FundingPoolWeightSet(_pool, _weight, _newTotalWeight);
        }
    }

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/CitadelMinter.sol#L276-L277

Tools Used

Manual Review

Recommended Mitigation Steps

Zero check logic can be implemented.

[GalloDaSballo]

@dapp-whisperer wdyt?

Pretty sure the idea was to subtract old weight from total, but perhaps the warden has a point?

[shuklaayush]

Seems to be fine since fundingPoolWeights[_pool] should always be less than totalFundingPoolWeight. Calculations are a bit redundant though

[jack-the-pug]

This is invalid as when totalFundingPoolWeight == 0, fundingPoolWeights[_pool] must be 0.

_newTotalWeight = _newTotalWeight - fundingPoolWeights[_pool];