Concurrent distributions result in locked tokens within the treasury.

[code423n4]

Lines of code

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

Vulnerability details

Impact

Initiating a new distribution before the completion of the previous one results in locked tokens within the treasury. This could result in the locking of 3%-6% of the initial treasury token supply. Additionally, this can result in inaccurate calculations during voting and extraordinary funding, leading to unexpected behavior.

Proof of Concept

Make a new file in ajna-grants/test/unit and palce the code bellow PoC Following the completion of distribution 1 approximately 90 days after the protocol's activation, the potential for an attack arises. This issue stems from the code snippet below, where the statement if (block.number <= currentDistributionEndBlock) permits the function to be called immediately after the previous distribution's end block. However, both the current and previous distributions will not be updated since the condition (block.number > _getChallengeStageEndBlock(currentDistributionEndBlock)) requires waiting for the end block plus 7 days. Consequently, the logic is vulnerable to being compromised by either intentional or unintentional actions. Although this vulnerability can be triggered only once, it can occur repeatedly after each period as users are expected to initiate new distributions after the previous ones end.

        //@audit  block.number > end block
    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
            //@audit  block.number > end block + 7 days
            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);
        }
    }

Tools Used

Manual audit + Foundry

Recommended Mitigation Steps

One straightforward solution would be to modify the code as shown below:

-    if (currentDistributionId > 1 && !_isSurplusFundsUpdated[currentDistributionId - 1]) 

+    if (currentDistributionId >= 1 && !_isSurplusFundsUpdated[currentDistributionId - 1]) 

Alternatively, another approach would be to initiate the new distribution after the protocol has identified the most suitable slate, which occurs 7 days after the previous distribution's end.

Assessed type

Governance

[c4-sponsor]

ith-harvey marked the issue as sponsor disputed

[ith-harvey]

The issue mentioned that 3 - 6% of tokens will be locked with each new distribution. But the update treasury method will unlock these tokens and add them back to treasury.

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid

[Picodes]

It seems to me as well that unused tokens will be added back to the treasury