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#L119-L164 https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-grants/src/grants/GrantFund.sol#L58-L68

Vulnerability details

Impact

The deployment of GrandFund.sol and the transfer of AJNA tokens to the treasury can be executed separately on different blocks if not deployed through another smart contract function.

There is a risk that someone could front-run the treasury funding transaction to startNewDistributionPeriod() with an empty treasury, wasting the first period and delaying the start of a new period with proper settings by 90 days.

This risk also exists in subsequent periods if fund transfers occur after the current period has ended. In such cases, anyone could front-run the transaction to start the new period with the leftover amount from the last period. However, this is unlikely as deposits can be made in advance.

Proof of Concept

According to the deployment script and testing contract, the contract deployment and fundTreasury() are likely to be executed separately. In this case, someone could call startNewDistributionPeriod() between these two transactions to start the first granting period before the treasury is set.

This would result in a 0 reward start, which cannot be restarted or reset with a new treasury amount. Each period is set to end roughly 90 days in the future, and any attempts to call startNewDistributionPeriod() while block.number is within the current period will revert.

File: ajna-grants/src/grants/base/StandardFunding.sol

119:    function startNewDistributionPeriod() external override returns (uint24 newDistributionId_) {
120:        uint24  currentDistributionId       = _currentDistributionId;
121:        uint256 currentDistributionEndBlock = _distributions[currentDistributionId].endBlock;
122:
123:        // check that there isn't currently an active distribution period
124:        if (block.number <= currentDistributionEndBlock) revert DistributionPeriodStillActive();
125:
126:        // update Treasury with unused funds from last two distributions

dizzy#6247 confirmed that the contracts are immutable and not behind any upgradeable proxy. As a result, the team cannot reset the storage value with a new implementation. Additionally, there is no way to withdraw the treasury once it has been deposited unless the team realizes they are being front-run and cancels the fundTreasury() call in time.

The following PoC code can be appended to the file ajna-grants/test/unit/GrantFund.t.sol. The test should pass without errors.

File: ajna-grants/test/unit/GrantFund.t.sol

    function testFrontRunStartNewDistributionPeriod() external {
        vm.stopPrank(); // there's an active prank from setUp()

        // Starting new period is available.
        // All fund in the reasury has been distributed. Need to add more fund before starting the next one.
        // Or it can be that someone start the first period before any fund added to the treasury.
        assertTrue(_grantFund.treasury() == 0, "Treasury should has been fully distributed");

        // _tokenHolder1 attempt to add extra fund to the treasury before starting the new period.
        // But someone frontrun _tokenHolder1 transferring and start the new period with old treasury amount.
        _grantFund.startNewDistributionPeriod();

        // _tokenHolder1 add extra fund to the treasury
        vm.startPrank(_tokenHolder1);
        _token.approve(address(_grantFund), 50_000_000 * 1e18);
        _grantFund.fundTreasury(50_000_000 * 1e18);

        // _tokenHolder1 transaction to start a new period reverted because someone already did.
        vm.expectRevert(IStandardFunding.DistributionPeriodStillActive.selector);
        _grantFund.startNewDistributionPeriod();
        vm.stopPrank();
    }

run: forge test --match-test testFrontRunStartNewDistributionPeriod

Running 1 test for test/unit/GrantFund.t.sol:GrantFundTest
[PASS] testFrontRunStartNewDistributionPeriod() (gas: 132279)
Test result: ok. 1 passed; 0 failed; finished in 1.26s

Tools Used

Manual review
Foundry

Recommended Mitigation Steps

To ensure a minimum required treasury before starting a new distribution period, add a check at the beginning of the startNewDistributionPeriod() function. For example, verify that treasury > 0.

File: ajna-grants/src/grants/base/StandardFunding.sol

119:    function startNewDistributionPeriod() external override returns (uint24 newDistributionId_) {
+           if (treasury == 0) revert("Cannot start with empty treasury!");
120:        uint24  currentDistributionId       = _currentDistributionId;
121:        uint256 currentDistributionEndBlock = _distributions[currentDistributionId].endBlock;
122:
123:        // check that there isn't currently an active distribution period
124:        if (block.number <= currentDistributionEndBlock) revert DistributionPeriodStillActive();
125:
126:        // update Treasury with unused funds from last two distributions

The deploying process can mitigate this issue by either:

Deploying the contract through a function from another contract, guaranteeing it to be in the same transaction and preventing front-running. This requires deploying 2 contracts instead of 1.
Immediately funding the treasury after the contract has been deployed to minimize the gap between the two transactions and prevent front-running.

Fixing this issue in the code is strongly recommended as it mitigates not only the first period but also any other periods that require a minimum amount of AJNA in the treasury.