nobody2018 - mintRollovers does not update rolloverAccounting[_epochId] causing stuck

[sherlock-admin]

nobody2018

high

mintRollovers does not update rolloverAccounting[_epochId] causing stuck

Summary

For each epochId, if the rolloverQueue is too long, the relayer will call mintRollovers multiple times to process the entire rolloverQueue. One block has a maximum gas limit, so relayer cannot process too many QueueItem at one time. The rolloverAccounting mapping records the currently processed rolloverQueue location for each epochId. However, at the end of mintRollovers , rolloverAccounting[_epochId] is updated only if executions > 0. executions are only incremented by 1 if the QueueItem has won the current epoch and QueueItem.asset is bigger than relayerFee. Assuming that the maximum number of QueueItems that can be processed each time is 100, if none of the first 100 QueueItems in the rolloverQueue can increase executions, then rolloverAccounting[_epochId] will always be 0. No matter how many times the relayer calls mintRollovers, it keeps looping through the first 100 QueueItems without moving forward.

Vulnerability Detail

Due to the maximum block gas limit, we assume that mintRollovers can process at most 100 QueueItems at a time. Therefore, the relayer cannot change this number arbitrarily. If it is changed to a larger value, the trade will be reverted. Let's assume the following scenario:

relayerFee is 1e18. The current epochId is E1, and the next epochId is E2. At present, rolloverQueue has 50 normal user QueueItem. Bob has deposited 100e18 assets before the start of E1, so balanceOf(bob, E1) = 100e18.

1. Bob creates 100 addresses, each address has setApprovalForAll to bob. He calls two functions for each address:

   Carousel.safeTransferFrom(bob, eachAddress, E1, 1e18)

   Carousel.enlistInRollover(E1, 1e18, eachAddress), 1e18 equal to minRequiredDeposit.

2. rolloverQueue.length equals to 150(100+50).

3. As time goes by, E1 ends. The relayer calls mintRollovers(E1, 100) twice, and the entire queue has been processed.

4. 50 normal user has called delistInRollover to exit the rolloverQueue. Now, rolloverQueue only has QueueItems for those 100 addresses created by bob. These 100 QueueItem stay in rolloverQueue forever. 

5. E2 is coming. The new user's QueueItem(for E2) is pushed to the rolloverQueue, at the end of the queue.

6. As time goes by, E2 ends. The relayer calls mintRollovers(E2, 100) many times, rolloverAccounting[E2] is always 0. 

Let's explain why the QueueItem of these 100 addresses does not increase the executions variable. There are two cases here:

1. These 100 addresses all lost in E1. It will not enter [if (titledShares > queue[index].assets)](https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Carousel/Carousel.sol#L401). So executions will be not increased.
2. These 100 addresses all won in E1. Because each QueueItem.asset is 1e18, and the relayerFee is subtracted, so it must be less than 1e18. Then at E2, it will enter [if (queue[index].assets < relayerFee)](https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Carousel/Carousel.sol#L403) to continue. So executions will be not increased also.

As mentioned above, starting from E2, mintRollovers will always be stuck at the front 100 QueueItem.

Impact

• mintRollovers never work properly.
• The relayer calling mintRollovers cannot get the relayerFee from Carousel.

Code Snippet

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

Tool used

Manual Review

Recommendation

• Remove if (executions > 0) .
• We should change the single queue to queue mapping. In this way, relayer only needs to process the queue corresponding to the epochId.

--- a/Earthquake/src/v2/Carousel/Carousel.sol
+++ b/Earthquake/src/v2/Carousel/Carousel.sol
@@ -23,7 +23,7 @@ contract Carousel is VaultV2 {
     IERC20 public immutable emissionsToken;
 
     mapping(address => uint256) public ownerToRollOverQueueIndex;
-    QueueItem[] public rolloverQueue;
+    mapping(uint256 => QueueItem[]) public rolloverQueues;
     QueueItem[] public depositQueue;
     mapping(uint256 => uint256) public rolloverAccounting;
     mapping(uint256 => mapping(address => uint256)) public _emissionsBalances;
@@ -450,7 +450,7 @@ contract Carousel is VaultV2 {
             index++;
         }

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

         if (executions * relayerFee > 0)
             asset.safeTransfer(msg.sender, executions * relayerFee);

Duplicate of #172

[securitygrid]

Escalate for 10 USDC
I think this report is not a duplicate of #174. It should be a unique findings. The reasons are as follows:

1. This report describes the problem from the perspective of the offline relayer program. As a program, the _operations parameter passed to mintRollovers cannot be changed at will, it is a fixed parameter. The method that raises min deposit for the queue to make DDoS very expensive does not work. Because the cost of the attack depends on the value of _operations.
2. This report describes how to keep rolloverAccounting[_epochId] at 0 no matter how many times mintRollovers is called, which causes the relayer to consume gas without receiving any relayerFee.
3. This issue won't trigger a DOS, but it can spend all the native tokens in the relayer's wallet.
4. In mintRollovers, rolloverQueue will not perform pop operation, and one attack will affect all epochs.

I don't think this is a duplicate of #62.

[sherlock-admin]

Escalate for 10 USDC
I think this report is not a duplicate of #174. It should be a unique findings. The reasons are as follows:

1. This report describes the problem from the perspective of the offline relayer program. As a program, the _operations parameter passed to mintRollovers cannot be changed at will, it is a fixed parameter. The method that raises min deposit for the queue to make DDoS very expensive does not work. Because the cost of the attack depends on the value of _operations.
2. This report describes how to keep rolloverAccounting[_epochId] at 0 no matter how many times mintRollovers is called, which causes the relayer to consume gas without receiving any relayerFee.
3. This issue won't trigger a DOS, but it can spend all the native tokens in the relayer's wallet.
4. In mintRollovers, rolloverQueue will not perform pop operation, and one attack will affect all epochs.

I don't think this is a duplicate of #62.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[hrishibhat]

Escalation accepted

Although not a duplicate of #174, after further consideration, this issue is marked as a duplicate of #172 based on the large rolloverQueue.
Comment from #174:
#174 (comment)

[sherlock-admin]

Escalation accepted

Although not a duplicate of #174, after further consideration considering this issue as a duplicate of #172 based on the large rolloverQueue.
Comment from #174:
#174 (comment)

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.