_updateBucketExchangeRates could possibly revert

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/RewardsManager.sol#L679 https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/RewardsManager.sol#L693-L731 https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/RewardsManager.sol#L697 https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/RewardsManager.sol#L653-L655

Vulnerability details

Impact

_updateBucketExchangeRates will not work correctly and would revert in case totalBurnedLatest < totalBurnedAtBlock causing DOS for the users when they try to claimRewards, moveStakedLiquidity, stake or unstake.

Proof of Concept

When the curBurnEpoch doesn't not equal zero at _updateBucketExchangeRates function at L679 will jump to else block and execute those lines L693-L731

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

            if (block.timestamp <= curBurnTime + UPDATE_PERIOD) {

                // update exchange rates and calculate rewards if tokens were burned and within allowed time period
                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; }
                }

                uint256 rewardsCap            = Maths.wmul(UPDATE_CAP, totalBurned);
                uint256 rewardsClaimedInEpoch = updateRewardsClaimed[curBurnEpoch];

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

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

So basically the code block above is calculating the rewardsCap and rewardsClaimedInEpoch, updating the total tokens claimed for bucket exchange rates with accumulating the full amount of additional rewards. Now the issue case is happening when totalBurned value is returned by _getPoolAccumulators function on L697

            (
                uint256 curBurnTime,
                uint256 totalBurned,
                uint256 totalInterestEarned
            ) = _getPoolAccumulators(pool_, curBurnEpoch, curBurnEpoch - 1);

The totalBurned is being calculated at L653-L655

        uint256 totalBurned   = totalBurnedLatest   != 0 ? totalBurnedLatest   - totalBurnedAtBlock   : totalBurnedAtBlock;
        uint256 totalInterest = totalInterestLatest != 0 ? totalInterestLatest - totalInterestAtBlock : totalInterestAtBlock;

Since totalBurnedLatest is starting from zero, It is possible that totalBurned of current block (totalBurnedLatest) is less than totalBurned of previous block (totalBurnedAtBlock) resulting in a revert.

Note: this also applicable on totalInterest

Tools Used

Manual Review

Recommended Mitigation Steps

Check if totalBurnedLatest greater than totalBurnedAtBlock for the totalBurned, Also do the same for totalInterest.

Assessed type

DoS

[c4-sponsor]

ith-harvey marked the issue as sponsor disputed

[ith-harvey]

ith-harvey marked the issue as sponsor disputed

The issue listed in their C4 issue is invalid. They make the claim that if totalBurnedLatest < totalBurnedAtBlock there would be a revert. This is not possible in the code as the amount burned returned from burnInfo() call to the pool is always an increasing number.

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid