Incorrect Reward Calculation Due to Binary Search Related Problem inside the `SingelTokenVirtualRewarderUpgradeable`

[hats-bug-reporter[bot]]

Github username: @MatinR1 Twitter username: MatinRezaii1 Submission hash (on-chain): 0x232ecd860c5fe8b879cbdd8b6a674a06fdb462130a4401d254ebc86374577fe6 Severity: high

Description: Description\ The onDettach() function is designed to handle user fund withdrawals and reward harvesting. However, it currently has a critical flaw that can lead to incorrect reward calculations, potentially causing significant fund loss for users.

The core issue stems from the behavior of the binary search algorithm used to find relative checkpoint indexes. When the binary search algorithm encounters a timestamp that is equal to the last time index checkpoint, it returns the middle index. This leads to an incorrect index being used in subsequent calculations.

        uint256 start;
        uint256 end = lastIndex_;
        while (end > start) {
            uint256 middle = end - (end - start) / 2;
            Checkpoint memory checkpoint = self_[middle];
            if (checkpoint.timestamp == timestamp_) {
                return middle;
            } else if (checkpoint.timestamp < timestamp_) {
                start = middle;
            } else {
                end = middle - 1;
            }
        }

        return start;
    }

In other words, the checkpointing mechanism which is designed to carefully track the balances and corresponding epochs, can lead to imprecise and wrong keeping of the balances by having multiple deposits/withdraws in a same epoch index.

Another point is that, all the timestamps for checkpointing task are indeed the week presentation of time, thus all the activites inside a week are rounded to just 1 epoch and checkpoints are overwritten.

Here’s a step-by-step breakdown of the problem:

Checkpoint Update During Withdrawal:

1. Inside the strategy contract, the onDettach() function first calls the withdraw function.
2. The withdraw() function inside the writes a new checkpoint and updates the last timestamp to the current timestamp. (both storage info timestamp for token and balance)
3. Immediately after withdraw(), the onDettach() function calls the harvest() function.
4. The harvest() function uses the updated last timestamp to determine the reward.
5. The harvest() function at its heart calls the _calculateAvailableRewardsAmount() function that is responsible for calculating the available rewards. This function attempts to find the nearest index of checkpoint for a token:

       uint256 index = startEpoch == 0
           ? 1
           : VirtualRewarderCheckpoints.getCheckpointIndex(
               tokensInfo[tokenId_].balanceCheckpoints,
               tokensInfo[tokenId_].checkpointLastIndex,
               startEpoch
           );

6. In the case of the equality of timestamp to the latest timestamp, the startEpoch would be the middle index of all the checkpoints.
7. This will lead to calculate the rewards imprecisely as the right and fair checkpointing index would not be achieved.

in the cases of calling onAttach()/onDettach() multiple times for a token id in same epoch timestamp, this problem becomes a serious issue which results in miscalculated rewards and potential fund loss.

Attachments

2. Revised Code File (Optional)\ Consider checking for not allowing the deposit and withdraw on the same epoch timestamp to fairly checkpointing the balances and epochs inside the strategy contract, or checkpoint the activities with a detailed time specification (block.timestamp rather than week presentation)

[BohdanHrytsak]

Hello. Thank you for submission and participating in the contest

Rounding the timestamps to the era is a desirable and intended feature. The point is that we are not interested in changes in balances, etc. during the epoch, only the final balance in this epoch is important.

For example:

1. epoch 0 - deposit 100
2. epoch 0 - withdraw 50
3. epoch 0 - deposit 100 -> 150 final balance

We are not interested in intermediate actions, as only the final balance is important, because it is part of the voting power for which the reward will be received on mVeNFT. And only the final balance will be rewarded.

So, we overwrite the user's balance always within the same epoch to have only the final state. Since we are not interested in intermediate ones. You can also see that the reward is rounded to the epoch, not the timestamp

The latter state is important because it is the only one that will be used as part of the vote. Therefore, it is the only one that will be rewarded