Ajna Protocol - Koolex's results

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/PositionManager.sol#L300

Vulnerability details

Impact

A lender can move liquidity from a bucket to another via moveLiquidity method. This method calls IPool(relevant pool).moveQuoteToken to move the quote tokens in the pool. On moveQuoteToken's success, it returns two values lpbAmountFrom and lpbAmountTo. PositionManager then update position LP state for each as follows:

	fromPosition.lps -= vars.lpbAmountFrom;
	toPosition.lps   += vars.lpbAmountTo;

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/PositionManager.sol#L320-L321

The issue is that, the "bucket index from" is removed from positionIndexes as follows:

	// remove bucket index from which liquidity is moved from tracked positions
	if (!positionIndex.remove(params_.fromIndex)) revert RemovePositionFailed();

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/PositionManager.sol#L300

If fromPosition.lps is still greater than zero after deducting the moved LP, then the lender will lose those LPs since the "bucket index from" is no longer tracked. This results in a loss of LP for the lender.

Proof of Concept

PositionManager.moveLiquidity calculates the max amount of quote tokens that can be moved, given the tracked LP

	vars.maxQuote = _lpToQuoteToken(
		vars.bucketLP,
		vars.bucketCollateral,
		vars.bucketDeposit,
		fromPosition.lps,
		vars.bucketDeposit,
		_priceAt(params_.fromIndex)
	);

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/PositionManager.sol#L287-L295

Then it moves the maximum quote tokens in the pool

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

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/PositionManager.sol#L306-L315

However, the returned lpbAmountFrom is not always guaranteed to be equal to fromPosition.lps. If we check Pool.moveQuoteToken, it calls LenderActions.moveQuoteToken method. In this method we have the following code block that determines what to return:

	uint256 scaledLpConstraint = Maths.wmul(params_.lpConstraint, exchangeRate);
	if (
		params_.depositConstraint < scaledDepositAvailable &&
		params_.depositConstraint < scaledLpConstraint
	) {
		// depositConstraint is binding constraint
		removedAmount_ = params_.depositConstraint;
		redeemedLP_    = Maths.wdiv(removedAmount_, exchangeRate);
	} else if (scaledDepositAvailable < scaledLpConstraint) {
		// scaledDeposit is binding constraint
		removedAmount_ = scaledDepositAvailable;
		redeemedLP_    = Maths.wdiv(removedAmount_, exchangeRate);
	} else {
		// redeeming all LP
		redeemedLP_    = params_.lpConstraint;
		removedAmount_ = Maths.wmul(redeemedLP_, exchangeRate);
	}

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/libraries/external/LenderActions.sol#L711-L727

There are 2 conditions checked before redeeming all LP. If one of these conditions is true, then we know that the returned value won't be all LP. In other words, In PositionManager, lpbAmountFrom will not be equal to fromPosition.lps.

Tools Used

Manual analysis

Recommended Mitigation Steps

Only remove the fromPosition from positionIndexs set if fromPosition.lps is zero.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/RewardsManager.sol#L719-L726

Vulnerability details

Impact

In RewardsManager, when claiming the rewards, _updateBucketExchangeRates method is called. This method does the following:

1. If curBurnEpoch == 0 then just update the rates without calculating any reward.
2. Otherwise, retrieve accumulator values (e.g. totalBurned)

	// retrieve accumulator values used to calculate rewards accrued
	(
		uint256 curBurnTime,
		uint256 totalBurned,
		uint256 totalInterestEarned
	) = _getPoolAccumulators(pool_, curBurnEpoch, curBurnEpoch - 1);

3. Loop through the bucket index (positions recorded for the lender)
   • For each index, update bucket exchange rate and calculate the rewards (done via _updateBucketExchangeRateAndCalculateRewards)
   • Add the rewards to updatedRewards_ to track the total rewards.

	for (uint256 i = 0; i < indexes_.length; ) {

		// calculate rewards earned for updating bucket exchange rate
		updatedRewards_ += _updateBucketExchangeRateAndCalculateRewards(
			pool_,
			indexes_[i],
			curBurnEpoch,
			totalBurned,
			totalInterestEarned
		);

		// iterations are bounded by array length (which is itself bounded), preventing overflow / underflow
		unchecked { ++i; }
	}

4. Calculate the rewards cap based on totalBurned

	uint256 rewardsCap            = Maths.wmul(UPDATE_CAP, totalBurned);

5. Make sure old updateRewardsClaimed in epoch + updatedRewards_ doesn't exceed the rewards cap.

	// update total tokens claimed for updating bucket exchange rates tracker
	if (rewardsClaimedInEpoch + updatedRewards_ >= rewardsCap) {
		// if update reward is greater than cap, set to remaining difference
		updatedRewards_ = rewardsCap - rewardsClaimedInEpoch;
	}

6. Add updatedRewards_ to updateRewardsClaimed in epoch

	// accumulate the full amount of additional rewards
	updateRewardsClaimed[curBurnEpoch] += updatedRewards_;

Since rewardsClaimedInEpoch is always supposed to be not be greater than rewardsCap we have no revert issue (note: the method reverts if rewardsCap is lower than rewardsClaimedInEpoch). However, to guarantee this, totalBurned should never decrease. In other words, if totalBurned went from a high to low value at any point, then rewardsCap will follow. If we check how totalBurned is calculated, we notice that there is a possibilty that this happens.

	uint256 totalBurned   = totalBurnedLatest   != 0 ? totalBurnedLatest   - totalBurnedAtBlock   : totalBurnedAtBlock;

totalBurnedLatest => total burned of current epoch totalBurnedAtBlock => total burned of previous epoch

If totalBurnedLatest equals zero, it takes the difference between totalBurnedLatest and totalBurnedAtBlock. otherwise, it takes totalBurnedAtBlock. because of this logic, it is possible that we have the following:

• Assume totalBurnedLatest = 1500 and totalBurnedAtBlock = 1000
  • totalBurned = 500
• totalBurnedAtBlock increased with 100
  • totalBurned = 400

Now. since totalBurned went lower, rewardsCap will go lower as well. The issue is that, in case the above happens, updateRewardsClaimed in epoch will be greater than rewardsCap and this condition will be true regardless of how much updatedRewards_ the user should receive

rewardsClaimedInEpoch + updatedRewards_ >= rewardsCap

This results in a revert in the following line since rewardsCap is lower than rewardsClaimedInEpoch:

	// if update reward is greater than cap, set to remaining difference
	updatedRewards_ = rewardsCap - rewardsClaimedInEpoch;

Therefore, the user won't be able to claim rewards in such situations. And since claim rewards is also done upon staking, unstaking and moveStakedLiquidity, all of those will revert due to this issue.

Proof of Concept

Please check above for explanation. https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/RewardsManager.sol#L719-L726

Tools Used

Manual analysis

Recommended Mitigation Steps

Check if rewardsCap is lower than rewardsClaimedInEpoch, set updatedRewards_ to zero.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/PositionManager.sol#L192-L199

Vulnerability details

Impact

When the lender calls PositionManager.memorializePositions method the following happens:

1. Records bucket indexes along with its deposit times and lpBalances
2. Transfers LP ownership from the lender to PositionManager contract.

In point 1, it checks if there is a previous deposit and the bucket went bankrupt after prior memorialization, then it zero out the previous tracked LP. However, the lender could still have unclaimed rewards. In this case, the lender loses the rewards due to the lack of claiming rewards before zeroing out the previous tracked LP balance. If you check claim rewards functionality in RewardsManager, the bucket being not bankrupt is not a requirement. Please note that claiming rewards relies on the tracked LP balance in PositionManager.

Proof of Concept

• PositionManager.memorializePositions method

  • check for previous deposits and zero out the previous tracked LP if bucket is bankrupt

  	// check for previous deposits
  	if (position.depositTime != 0) {
  		// check that bucket didn't go bankrupt after prior memorialization
  		if (_bucketBankruptAfterDeposit(pool, index, position.depositTime)) {
  			// if bucket did go bankrupt, zero out the LP tracked by position manager
  			position.lps = 0;
  		}
  	}
  
  

  https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/PositionManager.sol#L192-L199

• In RewardsManager, check claimRewards and _claimRewards method. there is no a check for bucket's bankruptcy.

  https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/RewardsManager.sol#L114

  https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/RewardsManager.sol#L561

Tools Used

Manual analysis

Recommended Mitigation Steps

On memorializePositions, check if the lender already claimed his/her rewards before zeroing out the previous tracked LP.

Assessed type

Other

On memorializePositions, check if the lender already claimed his/her rewards before zeroing out the previous tracked LP.

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/PositionManager.sol#L188-L202 https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/PositionManager.sol#L213

Vulnerability details

Impact

After minting a position by calling PositionManager.mint, the lender can call PositionManager.memorializePositions which does the following:

1. Records bucket indexes along with its deposit times and lpBalances
2. Transfers LP ownership from the lender to PositionManager contract.

For transfering LP ownership to succeed, the lender should increase LP Allowance for PositionManager. otherwise, pool.transferLP will revert.

pool.transferLP method doesn't take amount as a parameter. instead, the method will transfer the minimum between LP allowance and LP balance. For example, if the lender has 1000 LP balance and the lender set for PositionManager 500 LP allowance, then pool.transferLP will transfer the minimum between both which is 500 LP.

The issue is in PositionManager.memorializePositions, as it always updates the position.lps to lpBalance. This means, the lender can set the allowance to 1 LP, then PositionManager records all the LP balance of the lender (e.g. 1000 LP) but only 1 LP is transferred from the lender to the PositionManager. This has a critical impact in may ways. For example, when the lender calls PositionManager.reedemPositions to redeem his/her positions, he/she will receive 1000LP instead of 1LP. Thus, taking other lenders LPs. Please note that if there is not enough LP of that relevant bucket in PositionManager, then reedemPositions will revert due to LP transfer failure.

This impacts also moveLiquidity and any other functionality that relies directly or indirectly on LP balance of the position (i.e. position.lps).

Lastly, there is an impact on rewards calculations as the lender will get higher rewards than what he/she is supposed to receive. This is because LP balance is considered when calculating the rewards.

Proof of Concept

Imagine the following scenario as an example:

1. Lender has 1000 LP for a certain bucket B
2. Lender mints a position NFT token Id 1
3. Lender set allowance 1 LP for PositionManager contract
4. Lender calls memorializePositions for token Id 1 and bucket B
5. A new position added wtih 1000 LP for token Id 1 and bucket B
6. 1 LP transferred from Lender to PositionManager
7. Lender now has 999 LP
8. Lender now reedemPositions and receives 1000 LP (assuming there is enough balance of LP from other lenders)
9. 1000 LP transferred from PositionManager to Lender
10. Lender now has 1999 LP stealing 999 LP from other lenders.

Another scenario: Instead of redeeming positions, Lender can stake the recorded 1000 LP and later claim rewards based on that when only 1 LP actually was sent to PositionManager.

Code:

• memorializePositions method

  • Reading lpBalance from lenderInfo and updates the position LP accordingly

  	(uint256 lpBalance, uint256 depositTime) = pool.lenderInfo(index, owner);
  	.
  	.
  	.
  	// update token position LP
  	position.lps += lpBalance;
  

  https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/PositionManager.sol#L188-L202

  • When transferring LPs, pool.transferLP is called as follows

  	pool.transferLP(owner, address(this), params_.indexes);
  
  

  https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/PositionManager.sol#L213

• Pool.transferLP method

  • It calls LPActions.transferLP

      LPActions.transferLP(
          buckets,
          _lpAllowances,
          approvedTransferors,
          owner_,
          newOwner_,
          indexes_
      );
  

  https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/base/Pool.sol#L520-L527

• LPActions.transferLP method

  • Reading allownce and balance, then taking the minimum

  	uint256 ownerLpBalance   = bankruptcyTime < ownerDepositTime ? owner.lps : 0;
  
  	uint256 allowedAmount = allowances_[ownerAddress_][newOwnerAddress_][index];
  	if (allowedAmount == 0) revert NoAllowance();
  
  	// transfer allowed amount or entire LP balance
  	allowedAmount = Maths.min(allowedAmount, ownerLpBalance);
  

  https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/libraries/external/LPActions.sol#L238-L244

  • Update the balance of the owner (sender) and the new owner(receiver)

  	if (newOwnerDepositTime > bankruptcyTime) {
  		// deposit happened in a healthy bucket, add amount of LP to new owner
  		newOwner.lps += allowedAmount;
  	} else {
  		// bucket bankruptcy happened after deposit, reset balance and add amount of LP to new owner
  		newOwner.lps = allowedAmount;
  	}
  
  	owner.lps     -= allowedAmount; // remove amount of LP from old owner
  
  

  https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/libraries/external/LPActions.sol#L252-L260

• RewardsManager._calculateNextEpochRewards internal method

  • calcualte interest earned

    		// calculate the amount of interest accrued in current epoch
    	interestEarned += _calculateExchangeRateInterestEarned(
    		ajnaPool_,
    		nextEpoch,
    		bucketIndex,
    		bucketSnapshot.lpsAtStakeTime,
    		bucketRate
    	); 
    

  • calculate rewards based on interestEarned

        epochRewards_ = _calculateNewRewards(
            ajnaPool_,
            interestEarned,
            nextEpoch,
            epoch_,
            claimedRewardsInNextEpoch
        );

Tools Used

Manual analysis

Recommended Mitigation Steps

Instead of recording LP balance of the lender to the position, record the minimum between allowance and LP balance. Basically, similiar to the logic implemented in LPActions.transferLP

Assessed type

Token-Transfer