nobody2018 - Due to rolloverQueue.pop(), mintRollovers may skip some users' QueueItem

[sherlock-admin]

nobody2018

medium

Due to rolloverQueue.pop(), mintRollovers may skip some users' QueueItem

Summary

As we all know, a block has a maximum gas limit. If a transaction needs to process a very long queue, it needs to be divided into multiple transactions for processing. If rolloverQueue.length is too large, the relayer will call mintRollovers multiple times. If rolloverQueue is destroyed between these calls, such as pop(), QueueItem at the end of the queue will be moved to the front, possibly causing them to be skipped.

Vulnerability Detail

To describe this issue, we simply assume a scenario:

• rolloverQueue has 100 QueueItem.
• relayer calls mintRollovers to process 50 QueueItem each time.
• bob's QueueItem is at the 20th position in the queue.
• alice's QueueItem is at the end of the queue.
• bob and alice won epoch they is rolling over.
• new round of epoch is coming, id is E1.

Now let's see how alice's QueueItem is moved to bob's position, the steps are as follows:

1. relayer calls mintRollovers(E1, 50) .
   Before: rolloverAccounting[E1] = 0.
   After:    rolloverAccounting[E1] = 50, bob's QueueItem has been processed.
2. bob calls delistInRollover(bob).
   Before: rolloverQueue.length = 100, alice's QueueItem is at the end of the queue.
   After:    rolloverQueue.length = 99, bob's QueueItem has been deleted, alice's QueueItem is at the 20th position in the queue.
3. relayer calls mintRollovers(E1, 50) again.
   Before: rolloverAccounting[E1] = 50.
   After:    rolloverAccounting[E1] = 99, QueueItem at positions 50 to 99 are all processed, but alice's QueueItem is not.

Therefore, alice's QueueItem can only be processed in the next round of epoch. Now let's consider the situation a bit more complex: During the period between the end of step 1 and the beginning of step 3, some users who have already been processed call delistInRollover at the same time, which will cause some users at the end of the queue to be skipped.

Impact

When a user's QueueItem is delayed to the next round, two situations may occur:

• in this round, he should have made a profit but missed it;
• in this round, he should have lost but escaped.

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

During the period when the relayer processes the entire rolloverQueue, the structure of the rolloverQueue cannot be destroyed. So add a bool variable to prevent users from calling delistInRollover.

Duplicate of #72