code-423n4 / 2023-05-ajna-findings

2 stars 0 forks source link

Treasury miscalculation #397

Closed code423n4 closed 1 year ago

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#L197-L220

Vulnerability details

Impact

In startNewDistributionPeriod() before the start of the new distribution period the treasury is updated with unused funds from the last two distributions. This happens in _updateTreasury() where in the for loop totalTokensRequested is calculated and then subtracted from fundsAvailable to calculate how much the unused funds are. The problem here is that when adding tokensRequested for each proposal to totalTokensRequested there is no check that the proposal is actually executed.

Proof of Concept

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

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

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

startNewDistributionPeriod() is called and a new distribution d is started

Then _updateTreasury is called for distributions d-1 and d-2.

In distribution d-1 we have p amount of proposals with a x amount of tokens requested for each(calculations are easier this way) and t(where t <= p) proposals that are not executed.

This means that instead of (p - t) * x the end result for totalTokensRequested will be p * t and t * x tokens will be locked.

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

Tools Used

Manual review

Recommended Mitigation Steps

Check that proposal is executed before increasing totalTokensRequested

If (proposal.executed) {
    totalTokensRequested += proposal.tokensRequested;
}

Assessed type

Math

MikeHathaway commented 1 year ago

Checking if it's executed is irrelevant as only proposals in a finalized slate can be executed and those funds are already set aside and not readded.

c4-judge commented 1 year ago

Picodes marked the issue as unsatisfactory: Insufficient proof