code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L119-L164 https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L197-L220

Vulnerability details

Impact

When block.number is more than the end block of the current distribution period and less than the end block of the challenge stage for the current distribution period, the following StandardFunding.startNewDistributionPeriod function can be called to start a new distribution period. In this case, because block.number > _getChallengeStageEndBlock(currentDistributionEndBlock) is false, _updateTreasury(currentDistributionId) will not be executed. Since the StandardFunding._updateTreasury function below for currentDistributionId is not called, the tokens that are not distributed during the current distribution period would not be added back to treasury for the new distribution period. Therefore, after executing uint256 gbc = Maths.wmul(treasury, GLOBAL_BUDGET_CONSTRAINT) and newDistributionPeriod.fundsAvailable = SafeCast.toUint128(gbc) in the StandardFunding.startNewDistributionPeriod function, fundsAvailable for the new distribution period is less than it should be comparing to the situation in which the non-distributed tokens from the current distribution period could be re-added to treasury before starting the new distribution period. Since the proposals' tokens requested and voters' delegate rewards are limited by fundsAvailable for the new distribution period that is less than it should be, the maximum number of tokens that can be requested by the proposals and the maximum delegate rewards that can be claimed by the voters would be less than they should be for the new distribution period, which are unfair to these proposals and voters.

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L119-L164

    function startNewDistributionPeriod() external override returns (uint24 newDistributionId_) {
        uint24  currentDistributionId       = _currentDistributionId;
        uint256 currentDistributionEndBlock = _distributions[currentDistributionId].endBlock;

        // check that there isn't currently an active distribution period
        if (block.number <= currentDistributionEndBlock) revert DistributionPeriodStillActive();

        // update Treasury with unused funds from last two distributions
        {
            // Check if any last distribution exists and its challenge stage is over
            if (currentDistributionId > 0 && (block.number > _getChallengeStageEndBlock(currentDistributionEndBlock))) {
                // Add unused funds from last distribution to treasury
                _updateTreasury(currentDistributionId);
            }

            // checks if any second last distribution exist and its unused funds are not added into treasury
            if (currentDistributionId > 1 && !_isSurplusFundsUpdated[currentDistributionId - 1]) {
                // Add unused funds from second last distribution to treasury
                _updateTreasury(currentDistributionId - 1);
            }
        }

        // set the distribution period to start at the current block
        uint48 startBlock = SafeCast.toUint48(block.number);
        uint48 endBlock = startBlock + DISTRIBUTION_PERIOD_LENGTH;

        // set new value for currentDistributionId
        newDistributionId_ = _setNewDistributionId();

        // create QuarterlyDistribution struct
        QuarterlyDistribution storage newDistributionPeriod = _distributions[newDistributionId_];
        newDistributionPeriod.id              = newDistributionId_;
        newDistributionPeriod.startBlock      = startBlock;
        newDistributionPeriod.endBlock        = endBlock;
        uint256 gbc                           = Maths.wmul(treasury, GLOBAL_BUDGET_CONSTRAINT);
        newDistributionPeriod.fundsAvailable  = SafeCast.toUint128(gbc);

        // decrease the treasury by the amount that is held for allocation in the new distribution period
        treasury -= gbc;

        ...
    }

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L197-L220

    function _updateTreasury(
        uint24 distributionId_
    ) private {
        bytes32 fundedSlateHash = _distributions[distributionId_].fundedSlateHash;
        uint256 fundsAvailable  = _distributions[distributionId_].fundsAvailable;

        uint256[] memory fundingProposalIds = _fundedProposalSlates[fundedSlateHash];

        uint256 totalTokensRequested;
        uint256 numFundedProposals = fundingProposalIds.length;

        for (uint i = 0; i < numFundedProposals; ) {
            Proposal memory proposal = _standardFundingProposals[fundingProposalIds[i]];

            totalTokensRequested += proposal.tokensRequested;

            unchecked { ++i; }
        }

        // readd non distributed tokens to the treasury
        treasury += (fundsAvailable - totalTokensRequested);

        _isSurplusFundsUpdated[distributionId_] = true;
    }

Proof of Concept

The following steps can occur for the described scenario.

For the current distribution period, the number of tokens that were not distributed can be 1e24.
Currently, block.number is more than the end block of the current distribution period and less than the end block of the challenge stage for the current distribution period.
The StandardFunding.startNewDistributionPeriod function is called to start the new distribution period.
Because _updateTreasury(currentDistributionId) is not executed, the 1e24 tokens that were not distributed during the current distribution period are not added back to treasury for the new distribution period.
Since GLOBAL_BUDGET_CONSTRAINT is 0.03 * 1e18, executing uint256 gbc = Maths.wmul(treasury, GLOBAL_BUDGET_CONSTRAINT) and newDistributionPeriod.fundsAvailable = SafeCast.toUint128(gbc) would cause fundsAvailable for the new distribution period to be 1e24 * 0.03 = 3e22 tokens less than it should be.

Tools Used

VSCode

Recommended Mitigation Steps

The StandardFunding.startNewDistributionPeriod function can be updated to revert if block.number is still less than or equal to the end block of the challenge stage for the current distribution period. If using this approach, the new distribution period can only be started after the challenge stage for the current distribution period is ended.

Assessed type

Other

c4-judge commented 1 year ago

Picodes marked the issue as primary issue

MikeHathaway commented 1 year ago

In the current design, a new distribution period can start before the challenge stage ends. Those funds won't be re-added to the treasury and made available until the next distribution period. _updateTreasury has logic to look back up to two previous distribution periods, and re-add funds to the treasury if they haven't been added already.

That being said, we are moving ahead with a change to prevent the distribution period from starting until the challenge stage ends, and then automatically re-adding any surplus on new distribution start.

c4-sponsor commented 1 year ago

MikeHathaway marked the issue as sponsor acknowledged

Picodes commented 1 year ago

Downgrading to Low as this seems to be the intended design: there is indeed a logic to look back up to two previous distribution periods