nobody2018 - Malicious user can make rolloverQueue never get processed

[sherlock-admin]

nobody2018

medium

Malicious user can make rolloverQueue never get processed

Summary

rolloverQueue is shared by all epochs. For each round of epoch, mintRollovers will process rolloverQueue from the beginning. A normal user calls enlistInRollover to enter the rolloverQueue, and in the next round of epoch, he will call delistInRollover to exit the rolloverQueue. In this case, rolloverQueue.length is acceptable. However, malicious user can make the rolloverQueue.length huge, causing the relayer to consume a huge amount of gas for every round of epoch. Carousel will send relayerFee to relayer in order to encourage external relayer to call mintRollovers. Malicious user can make external relayer unwilling to call mintRollovers. Ultimately, rolloverQueue will never be processed.

Vulnerability Detail

Let's assume the following scenario:

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

1. Bob creates 1000 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 1010(1000+10). 

These 1000 addresses will never call delistInRollover to exit the rolloverQueue, so no matter whether these addresses win or lose, their QueueItem will always be in the rolloverQueue. In each round of epoch, the relayer has to process at least 1000 QueueItems, and these QueueItems are useless. Malicious users only need to do it once to cause permanent affects. 

When a normal user loses in a certain round of epoch, he may not call delistInRollover to exit the rolloverQueue. For example, he left the platform and stopped playing. In this case, rolloverQueue.length will become larger and larger as time goes by.

Carousel contract will not send any relayerFee to the relayer, because these useless QueueItem will not increase the value of  [executions](https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Carousel/Carousel.sol#L447). Obviously, calling mintRollovers has no benefit for the relayer. Therefore, no relayer is willing to do this.

Impact

The relayer consumes a huge amount of gas for calling mintRollovers for each round of epoch. In other words, as long as the rolloverQueue is unacceptably long, it is a permanent DOS.

Code Snippet

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

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

Tool used

Manual Review

Recommendation

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;

[3xHarry]

I would disagree with the feasibility of this attack.

1. there is a non neglectable minDeposit which makes this attack much more expensive
2. the queue can be processed in multiple transactoins and the relayerFee is supposed to be configured so much so that each processed item gas consumption is reimbursed with a profit

[IAm0x52]

Issue has been acknowledged by sponsor