Ajna Protocol - nobody2018's results

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/PositionManager.sol#L323 https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/RewardsManager.sol#L175 https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/PositionManager.sol#L372 https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/PositionManager.sol#L455 https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/PositionManager.sol#L477 https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/RewardsManager.sol#L236 https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/RewardsManager.sol#L334 https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/RewardsManager.sol#L393

Vulnerability details

Impact

toPosition.depositTime is updated to the from bucket deposit time. In some scenario, toPosition will be considered bankrupt by PositionManager. All functions that subcall PositionManager._bucketBankruptAfterDeposit will be affected. RewardsManager.moveStakedLiquidity cannot be avoided either.

Proof of Concept

The buckets mapping from the Pool contract records all buckets, and the information of each bucket is represented as IPoolState.Bucket. PositionManager.moveLiquidity internally calls IPool.moveQuoteToken, which is to move the fromIndex bucket to the toIndex bucket. In other words, it is to update two IPoolState.Bucket structures. By tracking the entire flow of the function, IPool.moveQuoteToken allows  toBucket.bankruptcyTime to be greater than 0. This is a scenario that may cause problems.

//https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/libraries/external/LenderActions.sol#L220-L341
function moveQuoteToken(
        mapping(uint256 => Bucket) storage buckets_,
        DepositsState storage deposits_,
        PoolState calldata poolState_,
        MoveQuoteParams calldata params_
    ) external returns (uint256 fromBucketRedeemedLP_, uint256 toBucketLP_, uint256 movedAmount_, uint256 lup_) {
        ...

        Bucket storage toBucket = buckets_[params_.toIndex];

        MoveQuoteLocalVars memory vars;
        vars.toBucketBankruptcyTime = toBucket.bankruptcyTime;

        // cannot move in the same block when target bucket becomes insolvent
        if (vars.toBucketBankruptcyTime == block.timestamp) revert BucketBankruptcyBlock();

        Bucket storage fromBucket       = buckets_[params_.fromIndex];
        Lender storage fromBucketLender = fromBucket.lenders[msg.sender];
        ....
        ....
        // update lender and bucket LP balance in target bucket
        Lender storage toBucketLender = toBucket.lenders[msg.sender];

        vars.toBucketDepositTime = toBucketLender.depositTime;
->      if (vars.toBucketBankruptcyTime >= vars.toBucketDepositTime) {
            // bucket is bankrupt and deposit was done before bankruptcy time, reset lender lp amount
            toBucketLender.lps = toBucketLP_;

            // set deposit time of the lender's to bucket as bucket's last bankruptcy timestamp + 1 so deposit won't get invalidated
->          vars.toBucketDepositTime = vars.toBucketBankruptcyTime + 1;
        } else {
            toBucketLender.lps += toBucketLP_;
        }

        // set deposit time to the greater of the lender's from bucket and the target bucket
->      toBucketLender.depositTime = Maths.max(vars.fromBucketDepositTime, vars.toBucketDepositTime);

        // update bucket LP balance
        toBucket.lps += toBucketLP_;
    ...
    }

With the above code, toBucketLender.depositTime is updated to a value greater than toBucket.bankruptcyTime. For the pool, the condition that a lender can withdraw LP is Bucket.bankruptcyTime < BucketLender.depositTime. Inside PositionManager.moveLiquidity, after executing IPool(params_.pool).moveQuoteToken, update toPosition.depositTime to fromPosition.depositTime.

function moveLiquidity(
        MoveLiquidityParams calldata params_
    ) external override mayInteract(params_.pool, params_.tokenId) nonReentrant {
        Position storage fromPosition = positions[params_.tokenId][params_.fromIndex];

        MoveLiquidityLocalVars memory vars;
        vars.depositTime = fromPosition.depositTime;
        ...
        ...
        // move quote tokens in pool
        (
            vars.lpbAmountFrom,
            vars.lpbAmountTo,
        ) = IPool(params_.pool).moveQuoteToken(
            vars.maxQuote,
            params_.fromIndex,
            params_.toIndex,
            params_.expiry
        );

        Position storage toPosition = positions[params_.tokenId][params_.toIndex];

        // update position LP state
        fromPosition.lps -= vars.lpbAmountFrom;
        toPosition.lps   += vars.lpbAmountTo;
        // update position deposit time to the from bucket deposit time
->      toPosition.depositTime = vars.depositTime;	//@audit  vars.depositTime is stale
        ...
    }

This value is clearly stale, not fresh. When fromPosition.depositTime is less than toBucket.bankruptcyTime, toPosition will be treated as bankruptcy by PositionManager.

Will the scenario where moving QuoteToken to a Bucket with a bankruptcyTime greater than 0 happen in practice?

1. As mentioned earlier, IPool.moveQuoteToken accepts this situation.
2. When the user submits the tx of PositionManager.moveLiquidity, toBucket.bankruptcyTime is 0. But tx was delayed for a while, toBucket.bankruptcyTime was updated during this time. When tx is mined, toBucket.bankruptcyTime is greater than 0.

Once these scenarios appears, the LP of toPosition is lost forever.

Tools Used

Manual Review

Recommended Mitigation Steps

toPosition.depositTime should be updated to the fresh value.

--- a/ajna-core/src/PositionManager.sol
+++ b/ajna-core/src/PositionManager.sol
@@ -320,7 +320,7 @@ contract PositionManager is ERC721, PermitERC721, IPositionManager, Multicall, R
         fromPosition.lps -= vars.lpbAmountFrom;
         toPosition.lps   += vars.lpbAmountTo;
         // update position deposit time to the from bucket deposit time
-        toPosition.depositTime = vars.depositTime;
+        (, toPosition.depositTime) = pool.lenderInfo(params_.toIndex, ownerOf(params_.tokenId));   
 
         emit MoveLiquidity(
             ownerOf(params_.tokenId),

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The ajnaToken balance of the GrantFund contract is inconsistent with treasury, and the gap will become larger and larger over time. This ultimately affects the distribution of project funds and voter rewards.

Proof of Concept

The StandardFunding.startNewDistributionPeriod function adds unused funds from the last two distributions to treasury, then uses the new treasury to calculate the new fundsAvailable.

function startNewDistributionPeriod() external override returns (uint24 newDistributionId_) {
        ...
        // 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);
            }
        }
    ...
->      uint256 gbc                           = Maths.wmul(treasury, GLOBAL_BUDGET_CONSTRAINT);
->      newDistributionPeriod.fundsAvailable  = SafeCast.toUint128(gbc);

        ...
    }

QuarterlyDistribution.fundsAvailable is divided into 2 parts: 90% as project funds and 10% as rewards for voters. However, _updateTreasury adds unused funds to the treasury without deducting the rewards for voters.

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

Obviously, the accumulated treasury is bigger than the real value. This results in the new fundsAvailable also being bigger.

Tools Used

Manual Review

Recommended Mitigation Steps

--- a/ajna-grants/src/grants/base/StandardFunding.sol
+++ b/ajna-grants/src/grants/base/StandardFunding.sol
@@ -214,7 +214,11 @@ abstract contract StandardFunding is Funding, IStandardFunding {
         }
 
         // readd non distributed tokens to the treasury
-        treasury += (fundsAvailable - totalTokensRequested);
+        if (totalTokensRequested > 0) {
+            treasury += (fundsAvailable * 9 / 10 - totalTokensRequested);
+        } else {
+            treasury += (fundsAvailable - totalTokensRequested);
+        }

         _isSurplusFundsUpdated[distributionId_] = true;
     }

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Users who did not vote in the screening stage but voted in the funding stage are not allowed to claim rewards via claimDelegateReward. Voting in the funding stage will occupy the distribution ratio of rewards. Since these rewards cannot be claimed, in the long run, the ajnaToken balance of the GrantFund contract is inconsistent with treasury.

Proof of Concept

At the beginning of the StandardFunding.claimDelegateReward function, check whether the caller voted in the screening stage.

function claimDelegateReward(
        uint24 distributionId_
    ) external override returns(uint256 rewardClaimed_) {
        // Revert if delegatee didn't vote in screening stage
->      if(screeningVotesCast[distributionId_][msg.sender] == 0) revert DelegateRewardInvalid();

        QuarterlyDistribution memory currentDistribution = _distributions[distributionId_];
        ...
    }

StandardFunding.fundingVote is used to vote in the funding stage. This function does not check whether the caller voted in the screening stage. fundingVote subcalls _fundingVote, which affects the allocation of rewards. _getDelegateReward is used by claimDelegateReward to calculate the reward distributed to the caller.

function _getDelegateReward(
        QuarterlyDistribution memory currentDistribution_,
        QuadraticVoter memory voter_
    ) internal pure returns (uint256 rewards_) {
        // calculate the total voting power available to the voter that was allocated in the funding stage
        uint256 votingPowerAllocatedByDelegatee = voter_.votingPower - voter_.remainingVotingPower;

        // if none of the voter's voting power was allocated, they receive no rewards
        if (votingPowerAllocatedByDelegatee == 0) return 0;

        // calculate reward
        // delegateeReward = 10 % of GBC distributed as per delegatee Voting power allocated
->      rewards_ = Maths.wdiv(
            Maths.wmul(
                currentDistribution_.fundsAvailable,	//total funds in current distribution
                votingPowerAllocatedByDelegatee		//voter's vote power
            ),
            currentDistribution_.fundingVotePowerCast	//total vote power in current distribution
        ) / 10;						// 10% fundsAvailable
    }

As long as fundingVote is successfully called, it means that the reward is locked to the caller. However, the caller cannot claim these rewards. There is no code to calculate the amount of these rewards that can never be claimed.

Tools Used

Manual Review

Recommended Mitigation Steps

Two ways to fix this problem:

1. FundingVote does not allow users who didn't vote in screening stage.
2. Delete the code on line L240.

Assessed type

Other