joestakey - Carousel.enlistRollover always overwrite the _receiver ownerToRollOverQueueIndex mapping, breaking the delisting process.

[sherlock-admin]

joestakey

medium

Carousel.enlistRollover always overwrite the _receiver ownerToRollOverQueueIndex mapping, breaking the delisting process.

Summary

enlistRollover always overwrite the _receiver ownerToRollOverQueueIndex mapping, meaning users calling the function more than once will have their mapping wrongfully updated and allow them to delist other users queued rollovers.

Vulnerability Detail

Users can enlist in a rollover queue with enlistInRollover.
The function pushes the rollover in rolloverQueue the first time. If a user calls enlistInRollover again, their rollover is simply updated.

File: Earthquake/src/v2/Carousel/Carousel.sol
252: // check if user has already queued up a rollover
253:         if (ownerToRollOverQueueIndex[_receiver] != 0) {
254:             // if so, update the queue
255:             uint256 index = getRolloverIndex(_receiver);
256:             rolloverQueue[index].assets = _assets;
257:             rolloverQueue[index].epochId = _epochId;
258:         } else {
259:             // if not, add to queue
260:             rolloverQueue.push(
261:                 QueueItem({
262:                     assets: _assets,
263:                     receiver: _receiver,
264:                     epochId: _epochId
265:                 })
266:             );
267:         }
268:         ownerToRollOverQueueIndex[_receiver] = rolloverQueue.length;//@audit should be in 'else' block

The issue is that ownerToRollOverQueueIndex[_receiver] is updated outside the blocks.
This means it will be updated every time the user calls this function, and not just during the first call.

Impact

A user calling this function for a second time will have their ownerToRollOverQueueIndex[_receiver] pointing to the same index as another user.

This breaks tracking of users in the rollover queue.

A direct consequence is that users can delist rollovers of other users, as ownerToRollOverQueueIndex[_owner] is used to get the index of the item to be removed in delistRollover, which is a grieving attack.

Note that the "attack" could be an accident:

• Alice calls enlistRollover. ownerToRollOverQueueIndex[Alice] == 1
• Bob calls enlistRollover. ownerToRollOverQueueIndex[Bob] == 2
• Alice calls enlistRollover again. ownerToRollOverQueueIndex[Alice] == 2
• Alice decides she does not want to rollover and calls delistInRollover. index is assigned line 286 with 2, which means Bob's rollover is the one that will get delisted.

Another way to label the issue is that a user calling enlistRollover twice will not be able to call delistRollover, as it will not be possible to update ownerToRollOverQueueIndex[] back to their actual index (unless other users decide to delist to reduce the size of the rolloverQueue array)

Proof Of Concept

Add this in Carousel.test.t(), showing how two users will have their mapping pointing to the same index as described above:

function testOverwriteRolloverMultiple() public {
        // test multiple rollovers
        // roll over users from testDepositIntoQueueMultiple test
        testDepositIntoQueueMultiple();

        // create new epoch
        uint40 _epochBegin = uint40(block.timestamp + 3 days);
        uint40 _epochEnd = uint40(block.timestamp + 4 days);
        uint256 _epochId = 3;
        uint256 _emissions = 100 ether;

        deal(emissionsToken, address(vault), 100 ether, true);
        vault.setEpoch(_epochBegin, _epochEnd, _epochId);
        vault.setEmissions( _epochId, _emissions);

        uint256 prevEpochUserBalance = 10 ether - relayerFee;

        uint256 prevEpoch = 2;
        // enlist in rollover for next epoch
        helperRolloverFromEpoch(prevEpoch, USER,  prevEpochUserBalance);
        helperRolloverFromEpoch(prevEpoch, USER2, prevEpochUserBalance);
        helperRolloverFromEpoch(prevEpoch, USER3, prevEpochUserBalance / 3);
        helperRolloverFromEpoch(prevEpoch, USER4, prevEpochUserBalance);

        // check balance of relayer
        uint256 balanceBefore = IERC20(UNDERLYING).balanceOf(address(this));

        //@audit all indexes are correct
        assertEq(vault.getRolloverIndex(USER), 0);
        assertEq(vault.getRolloverIndex(USER2), 1);
        assertEq(vault.getRolloverIndex(USER3), 2);
        assertEq(vault.getRolloverIndex(USER4), 3);

        //@audit USER3 updates their queue
        helperRolloverFromEpoch(prevEpoch, USER3, prevEpochUserBalance / 3);

        //@audit USER3 index is now the same as USER4!
        assertEq(vault.getRolloverIndex(USER), 0);
        assertEq(vault.getRolloverIndex(USER2), 1);
        assertEq(vault.getRolloverIndex(USER3), 3);
        assertEq(vault.getRolloverIndex(USER4), 3);
}

Code Snippet

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

Tool used

Manual Review, Foundry

Recommendation

File: Earthquake/src/v2/Carousel/Carousel.sol
252: // check if user has already queued up a rollover
253:         if (ownerToRollOverQueueIndex[_receiver] != 0) {
254:             // if so, update the queue
255:             uint256 index = getRolloverIndex(_receiver);
256:             rolloverQueue[index].assets = _assets;
257:             rolloverQueue[index].epochId = _epochId;
258:         } else {
259:             // if not, add to queue
260:             rolloverQueue.push(
261:                 QueueItem({
262:                     assets: _assets,
263:                     receiver: _receiver,
264:                     epochId: _epochId
265:                 })
266:             );
+                 ownerToRollOverQueueIndex[_receiver] = rolloverQueue.length;
267:         }
-268:         ownerToRollOverQueueIndex[_receiver] = rolloverQueue.length;

Duplicate of #2