code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-grants/src/grants/base/StandardFunding.sol#L120-L124 https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-grants/src/grants/base/StandardFunding.sol#L127-L139 https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-grants/src/grants/base/StandardFunding.sol#L152-L157 https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-grants/src/grants/base/StandardFunding.sol#L175

Vulnerability details

Impact

AJNA tokens will be stuck in GrantFund.sol contract if startNewDistributionPeriod() function is called before challenge period ends.

Proof of Concept

In AJNA protocol, there is a voting mechanism that is called Primary Funding Mechsnism (PFM). It has two stages in which holders of AJNA tokens can vote: the screening stage and the funding stage; it also has the challenge period, in which anyone can submit a set of proposals that get through the funding stage to execute.

Anyone can start a distribution period and in order to do so one has to call the startNewDistributionPeriod() function.

It writes to the parameteres of QuarterlyDistribution struct. In particular, it sets the endBlock parameter to block.number + DISTRIBUTION_PERIOD_LENGTH.

uint48 startBlock = SafeCast.toUint48(block.number);
uint48 endBlock = startBlock + DISTRIBUTION_PERIOD_LENGTH;

Before that, the endBlock parameter is used at the start of the startNewDistributionPeriod() function

uint256 currentDistributionEndBlock = _distributions[currentDistributionId].endBlock;

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

And the treasury global variable should be updated with unused funds from the last two distributions

// 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);
    }
}

It is then used to calculate the available funds for the distribution period

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;

The issue is in the line 129. It checks whether the blocknumber is greater than the endBlock_ + CHALLENGE_PERIOD_LENGTH

if (currentDistributionId > 0 && (block.number > _getChallengeStageEndBlock(currentDistributionEndBlock)))

function _getChallengeStageEndBlock(
    uint256 endBlock_
) internal pure returns (uint256) {
    return endBlock_ + CHALLENGE_PERIOD_LENGTH;
}

So if the startNewDistributionPeriod() function is called with the endBlock < block.number < endBlock_ + CHALLENGE_PERIOD_LENGTH the new distribution period would start and the unused funds would not be updated.

Consider the following scenario:

For this distribution period, the GBC, or global budgetary constraint, is 10M AJNA tokens.
There are 3 proposals (pA, pB, pC) that get through screening and funding stages. pA = 9M votes, pB + pC = 8M votes, so proposal pA will be executed as it has more votes.
During the challenge period somebody starts, accidentally or not, a new distribution period so the treasury is not updated properly and there is a lack of 1M AJNA tokens that would be stuck in a contract.

Please note that there will always be some loss as per docs the slate of proposals must not exceed 9/10 of currentDistribution.fundsAvailable

// check if slate of proposals exceeded budget constraint ( 90% of GBC )
if (totalTokensRequested > (gbc * 9 / 10)) {
revert InvalidProposalSlate();
}

Tools Used

Manual review

Assessed type

Governance

c4-judge commented 1 year ago

Picodes marked the issue as duplicate of #290