_checkpoint_total_supply() can checkpoint before a timestamp is complete

Summary

The _checkpoint_total_supply() function is in the RewardsDistributorV2 and is callable by anyone. The function has an incorrect comparison of > instead of >=, and this can lead to a checkpoint being recorded when a timestamp is not yet complete. This leads to mistakes in the internal accounting. In the worst case, a user can never successfully claim() again, which permanently freezes their LP tokens that have been deposited into ve.

Vulnerability Detail

The implementation of _checkpoint_total_supply() is as follows:

    function _checkpoint_total_supply() internal {
        address ve = voting_escrow;
        uint t = time_cursor;
        uint rounded_timestamp = block.timestamp / WEEK * WEEK;
        IVotingEscrow(ve).checkpoint();

        for (uint i = 0; i < 20; i++) {
            if (t > rounded_timestamp) {
                break;
            } else {
                uint epoch = _find_timestamp_epoch(ve, t);
                IVotingEscrow.Point memory pt = IVotingEscrow(ve).point_history(epoch);
                int128 dt = 0;
                if (t > pt.ts) {
                    dt = int128(int256(t - pt.ts));
                }
                ve_supply[t] = Math.max(uint(int256(pt.bias - pt.slope * dt)), 0);
            }
            t += WEEK;
        }
        time_cursor = t;
    }
}

Notice that whenever ve_supply[t] is assigned to a value, the t += WEEK increment happens immediately after, which permanently progresses the cursor so that ve_supply[t] will never be assigned to again. Also, notice that the ve_supply[t] assignment is only skipped if t > rounded_timestamp. This is incorrect. It should also be skipped if t == rounded_timestamp, otherwise this code can record the Math.max(uint(int256(pt.bias - pt.slope * dt)), 0) value before the timestamp itself is complete. This means the check should actually be: if (t >= rounded_timestamp).

Impact

In the scenario when t == rounded_timestamp, the code will incorrectly cache the Math.max(uint(int256(pt.bias - pt.slope * dt)), 0) of the current timestamp early. Any actions taken in ve after this (but on the same timestamp) will not be reflected in the ve_supply[] value. On the other hand, the _claimable() function will correctly account for these last-second actions in each individual _tokenId. As a result, it is possible for the balance_of value below to contain deposits that did not contribute to the ve_supply[week_cursor] value:

if (balance_of != 0) {
   to_distribute += balance_of * tokens_per_week[week_cursor] / ve_supply[week_cursor];
}

In the worst-case scenario, the first user of VotingEscrow can end up in a situation where ve_supply[week_cursor] == 0 and balance_of > 0. In this case, the claim() function will revert due to a division by zero, and it will permanently fail to progress past the broken week.

Since the withdraw() function in the ve contract has the following code:

IRewardsDistributorV2(distributor).claim(_tokenId, false);

it will always revert on this line (Actually in VE contract there is another bug where VE lacks of claim reward action when withdrawing, supposing that the VE contract implementing correct mechanesim of withdraw function), and users will be in a state where they are permanently unable to withdraw their LP tokens.

Code Snippet

https://github.com/sherlock-audit/2024-06-velocimeter/blob/main/v4-contracts/contracts/RewardsDistributorV2.sol#L149

Tool used

Manual Review

Recommendation

Consider changing from > to >= if (t >= rounded_timestamp)

References:

This report is inspired by the report from Immunefi Alchemix Boost #31076

Duplicate of #495

sherlock-audit / 2024-06-velocimeter-judging

sonny2k - _checkpoint_total_supply() can checkpoint before a timestamp is complete #581