Ajna Protocol - Bauchibred's results

Lines of code

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

Vulnerability details

Impact

While attempting to claim rewards, the amount of rewards senders receive is completely dependent on the contract's ajnaToken's balance which could go wrong in a lot of ways.

• If the balance of AjnaToken is let's say 1 or even just less than the rewardsEarned the check in the _transferAjnaRewards() function will always be true

        if (rewardsEarned_ > ajnaBalance) rewardsEarned_ = ajnaBalance;

And rewardsEarned will always be set to ajnaBalance This means that the user will be truncated.

• That's to say a case could be that a user stakes a for a long and he is eligible to claim 200 AJNA token, but the current smart contract only holds 1AJNA token, then because of the logic from the _transferAjnaRewards() function his reward is truncated to 1 AJNA, then the user lose all the remaining 199 tokens from the staking reward.

Proof of Concept

_claimRewards()

    function _claimRewards(
        StakeInfo storage stakeInfo_,
        uint256 tokenId_,
        uint256 epochToClaim_,
        bool validateEpoch_,
        address ajnaPool_
    ) internal {

        // revert if higher epoch to claim than current burn epoch
        if (validateEpoch_ && epochToClaim_ > IPool(ajnaPool_).currentBurnEpoch()) revert EpochNotAvailable();

        // update bucket exchange rates and claim associated rewards
        uint256 rewardsEarned = _updateBucketExchangeRates(
            ajnaPool_,
            positionManager.getPositionIndexes(tokenId_)
        );

        rewardsEarned += _calculateAndClaimRewards(tokenId_, epochToClaim_);

        uint256[] memory burnEpochsClaimed = _getBurnEpochsClaimed(
            stakeInfo_.lastClaimedEpoch,
            epochToClaim_
        );

        emit ClaimRewards(
            msg.sender,
            ajnaPool_,
            tokenId_,
            burnEpochsClaimed,
            rewardsEarned
        );

        // update last interaction burn event
        stakeInfo_.lastClaimedEpoch = uint96(epochToClaim_);

        // transfer rewards to sender
        _transferAjnaRewards(rewardsEarned);
    }

As seen the last step of the execution is to transfer the rewards to to sender

        // transfer rewards to sender
        _transferAjnaRewards(updateReward);

Now below is the implementation of _transferAjnaRewards()

RewardsManager._transferAjnaRewards();

    function _transferAjnaRewards(uint256 rewardsEarned_) internal {
        // check that rewards earned isn't greater than remaining balance
        // if remaining balance is greater, set to remaining balance
        uint256 ajnaBalance = IERC20(ajnaToken).balanceOf(address(this));
        if (rewardsEarned_ > ajnaBalance) rewardsEarned_ = ajnaBalance;

        if (rewardsEarned_ != 0) {
            // transfer rewards to sender
            IERC20(ajnaToken).safeTransfer(msg.sender, rewardsEarned_);
        }
    }

Tool used

Manual Review

Recommendation

if the balance is insufficient, the transfer should be prevented, and the execution of claimRewards should be reverted or better still one could track this and sends what's available to senders and have a track of how much protocol owes to users

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/RewardsManager.sol#L135-L198 https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/RewardsManager.sol#L806-L821

Vulnerability details

Impact

Unfair loss of sender's rewards

Proof of Concept

Below is the codeblock for moveStakedLiquidity()

• moveStakedLiquidity()

    function moveStakedLiquidity(
        uint256 tokenId_,
        uint256[] memory fromBuckets_,
        uint256[] memory toBuckets_,
        uint256 expiry_
    ) external nonReentrant override {
        StakeInfo storage stakeInfo = stakes[tokenId_];

        if (msg.sender != stakeInfo.owner) revert NotOwnerOfDeposit();

        // check move array sizes match to be able to match on index
        uint256 fromBucketLength = fromBuckets_.length;
        if (fromBucketLength != toBuckets_.length) revert MoveStakedLiquidityInvalid();

        address ajnaPool = stakeInfo.ajnaPool;
        uint256 curBurnEpoch = IPool(ajnaPool).currentBurnEpoch();

        // claim rewards before moving liquidity, if any
        _claimRewards(
            stakeInfo,
            tokenId_,
            curBurnEpoch,
            false,
            ajnaPool
        );

        uint256 fromIndex;
        uint256 toIndex;
        for (uint256 i = 0; i < fromBucketLength; ) {
            fromIndex = fromBuckets_[i];
            toIndex = toBuckets_[i];

            // call out to position manager to move liquidity between buckets
            IPositionManagerOwnerActions.MoveLiquidityParams memory moveLiquidityParams = IPositionManagerOwnerActions.MoveLiquidityParams(
                tokenId_,
                ajnaPool,
                fromIndex,
                toIndex,
                expiry_
            );
            positionManager.moveLiquidity(moveLiquidityParams);

            // update to bucket state
            BucketState storage toBucket = stakeInfo.snapshot[toIndex];
            toBucket.lpsAtStakeTime  = uint128(positionManager.getLP(tokenId_, toIndex));
            toBucket.rateAtStakeTime = uint128(IPool(ajnaPool).bucketExchangeRate(toIndex));
            delete stakeInfo.snapshot[fromIndex];

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

        emit MoveStakedLiquidity(tokenId_, fromBuckets_, toBuckets_);

        // update to bucket list exchange rates, from buckets are aready updated on claim
        // calculate rewards for updating exchange rates, if any
        uint256 updateReward = _updateBucketExchangeRates(
            ajnaPool,
            toBuckets_
        );

``        // transfer rewards to sender
        _transferAjnaRewards(updateReward);``
    }

As seen the last step of the execution is to transfer the rewards to to sender

        // transfer rewards to sender
        _transferAjnaRewards(updateReward);

Now below is the implementation of _transferAjnaRewards()

RewardsManager._transferAjnaRewards();

    function _transferAjnaRewards(uint256 rewardsEarned_) internal {
        // check that rewards earned isn't greater than remaining balance
        // if remaining balance is greater, set to remaining balance
        uint256 ajnaBalance = IERC20(ajnaToken).balanceOf(address(this));
        if (rewardsEarned_ > ajnaBalance) rewardsEarned_ = ajnaBalance;

        if (rewardsEarned_ != 0) {
            // transfer rewards to sender
            IERC20(ajnaToken).safeTransfer(msg.sender, rewardsEarned_);
        }
    }

This means that if senders rewards at the time of execution is more than ajnaBalance, their rewards get truncated to ajnaBalance and the rest of the rewards are lost, which implies that losers lose parts of their rewards unfairly while moving staked liquidity

Tool used

Manual Review

Recommendation

If the balance is insufficient, the transfer should be prevented, This will ensure that the user can receive their earned rewards later on, or you could implement a "force" parameter to allow users to bypass restrictions and go ahead to move their staked liquidity

Assessed type

Other

Lines of code

(https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/RewardsManager.sol#L806-L821 https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/RewardsManager.sol#L207-L260

Vulnerability details

Impact

While staking or updating buckets exchange rates to claim, senders could lose their rewards, this is due to the implementation of the _transferAjnaRewards()

Proof of Concept

[RewardsManager._transferAjnaRewards();]((https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/RewardsManager.sol#L806-L821)

 /** @notice Utility method to transfer `Ajna` rewards to the sender
     *  @dev   This method is used to transfer rewards to the `msg.sender` after a successful claim or update.
     *  @dev   It is used to ensure that rewards claimers will be able to claim some portion of the remaining tokens if a claim would exceed the remaining contract balance.
     *  @param rewardsEarned_ Amount of rewards earned by the caller.
     */
    function _transferAjnaRewards(uint256 rewardsEarned_) internal {
        // check that rewards earned isn't greater than remaining balance
        // if remaining balance is greater, set to remaining balance
        uint256 ajnaBalance = IERC20(ajnaToken).balanceOf(address(this));
        if (rewardsEarned_ > ajnaBalance) rewardsEarned_ = ajnaBalance;

        if (rewardsEarned_ != 0) {
            // transfer rewards to sender
            IERC20(ajnaToken).safeTransfer(msg.sender, rewardsEarned_);
        }
    }

As seen if at the time of execution the (rewardsEarned_ > ajnaBalance) senders only get sent ajnaBalance, note that this function is implemented in both the stake(z) and updateBucketExchangeRatesAndClaim() functions:

• stake()

    function stake(
        uint256 tokenId_
    ) external override {
        address ajnaPool = PositionManager(address(positionManager)).poolKey(tokenId_);

        // check that msg.sender is owner of tokenId
        if (IERC721(address(positionManager)).ownerOf(tokenId_) != msg.sender) revert NotOwnerOfDeposit();

        StakeInfo storage stakeInfo = stakes[tokenId_];
        stakeInfo.owner    = msg.sender;
        stakeInfo.ajnaPool = ajnaPool;

        uint256 curBurnEpoch = IPool(ajnaPool).currentBurnEpoch();

        // record the staking epoch
        stakeInfo.stakingEpoch = uint96(curBurnEpoch);

        // initialize last time interaction at staking epoch
        stakeInfo.lastClaimedEpoch = uint96(curBurnEpoch);

        uint256[] memory positionIndexes = positionManager.getPositionIndexes(tokenId_);

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

            uint256 bucketId = positionIndexes[i];

            BucketState storage bucketState = stakeInfo.snapshot[bucketId];

            // record the number of lps in bucket at the time of staking
            bucketState.lpsAtStakeTime = uint128(positionManager.getLP(
                tokenId_,
                bucketId
            ));
            // record the bucket exchange rate at the time of staking
            bucketState.rateAtStakeTime = uint128(IPool(ajnaPool).bucketExchangeRate(bucketId));

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

        emit Stake(msg.sender, ajnaPool, tokenId_);

        // transfer LP NFT to this contract
        IERC721(address(positionManager)).transferFrom(msg.sender, address(this), tokenId_);

        // calculate rewards for updating exchange rates, if any
        uint256 updateReward = _updateBucketExchangeRates(
            ajnaPool,
            positionIndexes
        );

        // transfer rewards to sender
        _transferAjnaRewards(updateReward);
    }

updateBucketExchangeRatesAndClaim()

       function updateBucketExchangeRatesAndClaim(
        address pool_,
        uint256[] calldata indexes_
    ) external override returns (uint256 updateReward) {
        updateReward = _updateBucketExchangeRates(pool_, indexes_);

        // transfer rewards to sender
        _transferAjnaRewards(updateReward);
    }

Tool used

Manual Review

Recommendation

Multiple ideas, but one should be allowing the user partially claim the reward when the smart contract has insufficient AJNA token balance to distribute the staking reward.

Or if the balance is insufficient, the transfer should be prevented, This will ensure that the user can receive their earned rewards. this might affect the case of normal flowing of staking or unstaking so consider adding a "force" parameter to allow users to bypass restrictions

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/RewardsManager.sol#L510-L551

Vulnerability details

Impact

Stakers may not receive rewards due to precision loss.

Proof of Concept

The RewardsManager._calculateNewRewards() function calculates the new rewards for a staker by dividing the product of interestEarned_ and totalBurnedInPeriod by the totalInterestEarnedInPeriod and then multiplying by REWARD_FACTOR If the product of interestEarned_ and totalBurnedInPeriodis small enough and totalInterestEarnedInPeriod is large enough, the division may result in a value of 0, resulting in the staker receiving 0 rewards.

Take a look at the function below and grep the "audit" tag to find out where the precision loss occurs

RewardsManager._calculateNewRewards()

    /**
     *  @notice Calculate new rewards between current and next epoch, based on earned interest.
     *  @param  ajnaPool_              Address of the pool.
     *  @param  interestEarned_        The amount of interest accrued to current epoch.
     *  @param  nextEpoch_             The next burn event epoch to calculate new rewards.
     *  @param  epoch_                 The current burn event epoch to calculate new rewards.
     *  @param  rewardsClaimedInEpoch_ Rewards claimed in epoch.
     *  @return newRewards_            New rewards between current and next burn event epoch.
     */
    function _calculateNewRewards(
        address ajnaPool_,
        uint256 interestEarned_,
        uint256 nextEpoch_,
        uint256 epoch_,
        uint256 rewardsClaimedInEpoch_
    ) internal view returns (uint256 newRewards_) {
        (
            ,
            // total interest accumulated by the pool over the claim period
            uint256 totalBurnedInPeriod,
            // total tokens burned over the claim period
            uint256 totalInterestEarnedInPeriod
        ) = _getPoolAccumulators(ajnaPool_, nextEpoch_, epoch_);

        // calculate rewards earned
        newRewards_ = totalInterestEarnedInPeriod == 0 ? 0 : Maths.wmul(
            REWARD_FACTOR,
            Maths.wdiv(
                Maths.wmul(interestEarned_, totalBurnedInPeriod),
                totalInterestEarnedInPeriod
            )//@audit M precision loss still possible, Maths.wdiv is not the outermost bracket
        );

        uint256 rewardsCapped = Maths.wmul(REWARD_CAP, totalBurnedInPeriod);

        // Check rewards claimed - check that less than 80% of the tokens for a given burn event have been claimed.
        if (rewardsClaimedInEpoch_ + newRewards_ > rewardsCapped) {

            // set claim reward to difference between cap and reward
            newRewards_ = rewardsCapped - rewardsClaimedInEpoch_;
        }
    }

Tool used

Manual Review

Recommendation

Consider calculating the new rewards by first doing all the multiplication i.e multiplying interestEarned_ by totalBurnedInPeriod and then by REWARD_FACTOR before dividing by totalInterestEarnedInPeriod to avoid precision loss.

Assessed type

Math