rolloverQueue is corrupted on repeated enlisting

Summary

enlistInRollover() resets ownerToRollOverQueueIndex of the _receiver to the end of the line even when there is old queue entry in the middle of it.

Vulnerability Detail

When user calls enlistInRollover() already having the rollover request, which is a correct use case, user's old entry is correctly updated, but then ownerToRollOverQueueIndex[_receiver] is being reset.

After this getRolloverIndex(_receiver) will return the last element of the rolloverQueue, which most likely will have no connection to the user.

User's rolloverQueue accounting most likely be corrupted this way.

Impact

Attacker can use this to remove other depositor's entries for griefing one-by-one with enlistInRollover -> delistInRollover combo, which results in attacker deleting the last entry that doesn't belong to them. All entries up to the attacker's can be removed this way.

Also, notRollingOver() check will be directed to another entry, allowing attacker to double allocate the funds.

Code Snippet

When _receiver already have a rolloverQueue position, it is updated, but then index is just overwritten:

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

    function enlistInRollover(
        uint256 _epochId,
        uint256 _assets,
        address _receiver
    ) public epochIdExists(_epochId) minRequiredDeposit(_assets) {
        ...

        // check if user has already queued up a rollover
        if (ownerToRollOverQueueIndex[_receiver] != 0) {
            // if so, update the queue
@>          uint256 index = getRolloverIndex(_receiver);
@>          rolloverQueue[index].assets = _assets;
@>          rolloverQueue[index].epochId = _epochId;
        } else {
            // if not, add to queue
            rolloverQueue.push(
                QueueItem({
                    assets: _assets,
                    receiver: _receiver,
                    epochId: _epochId
                })
            );
        }
@>      ownerToRollOverQueueIndex[_receiver] = rolloverQueue.length;

        emit RolloverQueued(_receiver, _assets, _epochId);
    }

I.e. correct ownerToRollOverQueueIndex of _receiver is rewritten to be rolloverQueue.length, so getRolloverIndex() becomes incorrect:

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

    function getRolloverIndex(address _owner) public view returns (uint256) {
        return ownerToRollOverQueueIndex[_owner] - 1;
    }

For example, attacker can clear the queue this way as delistInRollover() relies on getRolloverIndex(_owner):

        // swich the last item in the queue with the item to be removed
        uint256 index = getRolloverIndex(_owner);
        uint256 length = rolloverQueue.length;
        if (index == length - 1) {
            // if only one item in queue
            rolloverQueue.pop();
            delete ownerToRollOverQueueIndex[_owner];
        } else {

notRollingOver() check can be skipped as another rolloverQueue entry is checked for the attacker:

    modifier notRollingOver(
        address _receiver,
        uint256 _epochId,
        uint256 _assets
    ) {
        if (ownerToRollOverQueueIndex[_receiver] != 0) {
            QueueItem memory item = rolloverQueue[getRolloverIndex(_receiver)];
            if (
                item.epochId == _epochId &&
                (balanceOf(_receiver, _epochId) - item.assets) < _assets
            ) revert AlreadyRollingOver();
        }
        _;
    }

Tool used

Manual Review

Recommendation

As a straightforward solution it might be resetting the index only when new item was created as otherwise index is correct:

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

    function enlistInRollover(
        ...

        // check if user has already queued up a rollover
        if (ownerToRollOverQueueIndex[_receiver] != 0) {
            // if so, update the queue
            ...
        } else {
            // if not, add to queue
            rolloverQueue.push(
                ...
            );
+           ownerToRollOverQueueIndex[_receiver] = rolloverQueue.length;            
        }
-       ownerToRollOverQueueIndex[_receiver] = rolloverQueue.length;

        emit RolloverQueued(_receiver, _assets, _epochId);
    }

However, as user might want to renew the position in the queue (when queue index moved beyond it will be stuck if previous variant be implemented), the better solution is to update the rolloverQueue in full:

    function enlistInRollover(
        ...

        // check if user has already queued up a rollover
        if (ownerToRollOverQueueIndex[_receiver] != 0) {
            // if so, update the queue
            uint256 index = getRolloverIndex(_receiver);
+           QueueItem item = rolloverQueue[index];  // create new item based on the old one
-           rolloverQueue[index].assets = _assets;
-           rolloverQueue[index].epochId = _epochId;
+           item.assets = _assets;
+           item.epochId = _epochId;
+           rolloverQueue.push(item);
+           delete rolloverQueue[index];  // empty element will be skipped?
        } else {
            // if not, add to queue
            rolloverQueue.push(
                QueueItem({
                    assets: _assets,
                    receiver: _receiver,
                    epochId: _epochId
                })
            );
        }
        ownerToRollOverQueueIndex[_receiver] = rolloverQueue.length;

        emit RolloverQueued(_receiver, _assets, _epochId);
    }

Duplicate of #2

sherlock-audit / 2023-03-Y2K-judging

iglyx - rolloverQueue is corrupted on repeated enlisting #502