roguereddwarf - Carousel: When a rollover is delisted it can prevent another rollover from being processed

[sherlock-admin]

roguereddwarf

medium

Carousel: When a rollover is delisted it can prevent another rollover from being processed

Summary

Rollovers are removed from the queue using the Carousel.delistInRollover function.

When a rollover is removed from the queue, the rollover at the end of the queue is moved to the position where the rollover was removed.

Rollovers are minted from head to tail. This means that when a rollover is moved from the end of the queue to an earlier position in the queue, this earlier position might have already been processed.

This means that the rollover that was moved to an earlier position will not get processed.

I will show how this can just occur as part of normal operation and without the need for an attacker to deliberately make this happen.

Vulnerability Detail

First I show how removing an element from the queue works:
Let's say the queue contains the following elements:

  head                tail
    1    2    3    4    5

Then element 2 is delisted and element 5 is moved to its position:

  head           tail
    1    5    3    4

This occurs by executing the else block in the delistInRollover function:
https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Carousel/Carousel.sol#L292-L303

The issue comes about because the mintRollovers function makes use of a rolloverAccounting mapping which keeps track of the next index in the queue that needs to be rolled over:
https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Carousel/Carousel.sol#L453

So say at a point in time all rollovers have been processed and the value in the rolloverAccounting mapping points to the next index where there is currently no element:

                 rolloverAccounting
                            |
  head                tail
    1    2    3    4    5

Then a new rollover 6 is queued and before it can get executed the element 2 is removed from the queue such that element 6 is moved towards the front of the queue. The queue then looks like this:

                 rolloverAccounting
                            |
  head                tail
    1    6    3    4    5

As you can see, the rollover 6 cannot get executed because rolloverAccounting is pointing to the end of the queue and rollover 6 has been moved to an earlier position within the queue.

Impact

Pending rollovers will not get executed if rollovers are delisted which just happens as part of normal operation.

Code Snippet

https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Carousel/Carousel.sol#L276-L304

https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Carousel/Carousel.sol#L361-L459

Tool used

Manual Review

Recommendation

This issue has been discussed with the sponsor but it was not possible to find a good solution with the current implementation of the queue.

It might be necessary to use a linked list implementation such that elements within the list can be removed without affecting the rest of the list.

Instead of pointing to an index, the rolloverAccounting mapping would then point to the next element to be processed directly or to a NULL element if the linked list is empty or all elements have been processed.
If the implementation is changed from a primitive array to a linked list there are further changes necessary to keep the state correctly updated so a broader refactoring would be necessary.

Duplicate of #72