ElKu - Unbounded size of rolloverQueue will eventually cause the mintRollovers function to revert

[sherlock-admin]

ElKu

medium

Unbounded size of rolloverQueue will eventually cause the mintRollovers function to revert

Summary

In Carousel contract, the items in the array rolloverQueue , doesnt get deleted after a listed item is minted by the mintRollovers function. And in every call to mintRollovers function, the whole array is copied to memory. This means eventually we will hit the gas limit of the block, making the function revert and thus resulting in the funds being stuck.

Vulnerability Detail

1. When a user wants to use his leftover funds from a previous epoch to the upcoming epoch, he/she calls the enlistInRollover function. This pushes the details required for the roll over to an array structure called rolloverQueue.
2. The only case where these pushed items are popped from the array is when the user itself wants to cancel the rollover using the delistInRollover function.
3. Otherwise a relayer would call the mintRollovers function before the new epoch starts.
4. In the mintRollovers function, the whole rolloverQueue array is read into memory, irrespective of how many operations the relayer want to perform. Each element in this array requires at least 300 gas, as its a storage read(3 uint's per read).
5. Assuming that each user creates a rollover request at least once, there will be one entry for each user in the array. If n is the total number of users in a market then the call to mintRollovers function will cost a minimum of n*300 gas.

Impact

As the time goes, and the number of users of a particular market increases the gas cost of minting a rollover increases. Beyond a threshold this will break the entire market(as the 3 million gas limit of a block is approached) resulting in loss of funds, and a lot and lot of inconvenience for everyone involved.

Code Snippet

The enlistInRollover function has this part:

        if (ownerToRollOverQueueIndex[_receiver] != 0) {  //ownerToRollOverQueueIndex is derived from length of array
            // if so, update the queue
            uint256 index = getRolloverIndex(_receiver);  //ownerToRollOverQueueIndex[_receiver] - 1;
            rolloverQueue[index].assets = _assets;
            rolloverQueue[index].epochId = _epochId;
        } else {
            // if not, add to queue
            rolloverQueue.push(
                QueueItem({
                    assets: _assets,
                    receiver: _receiver,
                    epochId: _epochId
                })
            );
        }

And mintRollovers has this line, reading the entire array into memory:

QueueItem[] memory queue = rolloverQueue;

Tool used

Manual Review, VSCode.

Recommendation

Remove the struct element from the array when a rollover is minted. Or change the logic so that only elements which need to be minted are read from storage.

Duplicate of #172