Ajna Protocol - ladboy233's results

Lines of code

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

Vulnerability details

Summary

User's staking reward can be lost in RewardsManager.sol

Vulnerability Detail

In the current implementation of the RewardManager.sol,

When claiming reward, if the smart contract has insufficient AJNA token balance to distribute the reward, the reward is truncated to the current token balance.

User can call the function below in rewardManager

    function claimRewards(
        uint256 tokenId_,
        uint256 epochToClaim_
    ) external override {
        StakeInfo storage stakeInfo = stakes[tokenId_];

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

        if (isEpochClaimed[tokenId_][epochToClaim_]) revert AlreadyClaimed();

        _claimRewards(stakeInfo, tokenId_, epochToClaim_, true, stakeInfo.ajnaPool);
    }

which calls _claimReward:

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

note the last piece of the code:

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

When claiming reward, if the smart contract has insufficient AJNA token balance to distribute the reward, the reward is truncated to the current token balance.

Impact

For example, the user stakes a for a long and he is eligile to claim 200 AJNA token, but the current smart contract only hold 100 AJNA token, then because of the logic below:

if (rewardsEarned > ajnaBalance) rewardsEarned = ajnaBalance;

// transfer rewards to sender
IERC20(ajnaToken).safeTransfer(msg.sender, rewardsEarned);

his reward is truncated to 100 AJNA, then the user lose staking reward.

Tool used

Manual Review

Recommendation

We recommend the protocol only let the user partially claim the reward when the smart contract has insufficient AJNA token balance to distribute the staking reward.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Division before multiplication result in high precision loss when calculating new rewards

Proof of Concept

note the logic in RewardsManager.sol when calcuating new reward

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

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

we also note:

    function wmul(uint256 x, uint256 y) internal pure returns (uint256) {
        return (x * y + WAD / 2) / WAD;
    }

and

    function wdiv(uint256 x, uint256 y) internal pure returns (uint256) {
        return (x * WAD + y / 2) / y;
    }

In the code, mulitple divison before muplication occurs

The RewardsManager.calculateNewRewards function calculates the new rewards for a staker by first dividing interestEarned by totalInterestEarnedInPeriod and then multiplying by totalBurnedInPeriod. If interestEarned_ is small enough and totalInterestEarnedInPeriod is large enough, the division may result in a value of 0, resulting in the staker receiving 0 rewards.

Tools Used

Manual Review

Recommended Mitigation Steps'

We recommend the protocol avoid precise loss operation and only perform divison in the last step of the calcuation.

Assessed type

Math