BPZ - The mintRollovers function will skip users

[sherlock-admin]

BPZ

medium

The mintRollovers function will skip users

BPZ

medium

Summary

The protocol has the functionallity to allow users to automatically roll their positions over to the next available epoch. This functionallity is implemented by a relayer calling the mintRollovers function. For a given epoch, the starting index of where to begin iterating through the rollOverQueue is defined here. However, if a user that has already had their position rolled over to the next epoch calls the delistInRollover function, and the rollOverQueue has not been iterated through completely, the user who owns the position last in the rollOverQueue will have their position swapped to the index of the user's position that is delisting and, then, will not have their position rolled over to the next epoch.

Vulnerability Detail

If the rollOverQueue has not been completely iterated through and a user that has had their position rolled over to the next epoch invokes the delistInRollover function on their position in the queue, the user that has their position swapped with the user's position that is delisting will not have their position rolled over to the next epoch. This is due to the fact that the mintRollOvers function iterates through the rollOverQueue beginning at the index defined here:

372    uint256 index = rolloverAccounting[_epochId];

Once the mintRollOvers function has iterated through the amount of positions defined by the 'operations' argument, the index is updated here:

453    if (executions > 0) rolloverAccounting[_epochId] = index;

Given the queue has not been iterated through completely for the respective epoch, if a user with a position in the rollOverQueue that is less than the epoch's respective index (meaning their position has been rolled over to the next epoch or iterated passed) delists their position by calling the delistInRollover function, the user that currently has their position last in the queue will not have their position rolled over to the next epoch because their position in the queue will be updated to the user's positon in the queue that invoked the delistInRollover function. This functionallity is shown here:

294     rolloverQueue[index] = rolloverQueue[length - 1];
295     // remove the last item in the queue
296     rolloverQueue.pop();
297     // update the index of prev last user ( mapping index is allways array index + 1)
298     ownerToRollOverQueueIndex[rolloverQueue[index].receiver] =
299          index +
300          1;
301     // remove receiver from index mapping
302     delete ownerToRollOverQueueIndex[_owner];

Because the respective rolloverQueue index of the user who had their position swapped to the new index in the queue is, now, less than the index defined by 'rolloverAccounting[_epochId]' for the next epoch, this user's position will not be rolled over to the respective epoch because this index in the queue has already been iterated passed.

Impact

Users will experience a DOS for the automatic roll-over functionality and their funds will be idle.

Code Snippet

Please see the links and mentioned blocks of code above for the affected code.

Tool used

Manual Review

Recommendation

As a mitigation for this, it is recommended to not allow a user to call the delistInRollover function if their position has already been rolled over to the next epoch. This can be done by implementing the following check with a respective custom error in the delistInRollover function on line 287 between the definitions of the 'index' and 'length' variables:

286    uint256 index = getRolloverIndex(_owner);
287    if(rollOverQueue[index].epochId == epochs[epochs.length - 1]) revert DelistFromCurrentEpoch()
288    uint256 length = rolloverQueue.length;

Duplicate of #72