Dug - Delisting a rollover can prevent minting of other queued rollovers

[sherlock-admin]

Dug

medium

Delisting a rollover can prevent minting of other queued rollovers

Summary

Due to how rollover mints are tracked, if a user delists a rollover from the queue after it has been minted for an epoch, a subsequent rollover in the queue could be prevented from being minted for that epoch.

Vulnerability Detail

When mintRollovers is called to execute rollovers for a given epoch, progress through the rolloverQueue is tracked with an index stored in rolloverAccounting[_epochId].

When a rollover in the queue is executed, the index is incremented. This allows subsequent calls to mintRollovers for the same epoch to skip over the previously executed rollovers.

However this causes an issue when considering how delistInRollover is implemented. When a rollover is delisted, it's overwritten by the rollover at the end of the queue, and then the last item is poped off the queue.

// 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 {
    // overwrite the item to be removed with the last item in the queue
    rolloverQueue[index] = rolloverQueue[length - 1];
    // remove the last item in the queue
    rolloverQueue.pop();
    // update the index of prev last user ( mapping index is allways array index + 1)
    ownerToRollOverQueueIndex[rolloverQueue[index].receiver] =
        index +
        1;
    // remove receiver from index mapping
    delete ownerToRollOverQueueIndex[_owner];
}

Impact

This means that if an executed rollover is delisted, it's position in the queue could be overwritten by a rollover that has not yet been executed. When mintRollovers is called again for the same epoch, that replacement rollover will not be called for the epoch.

This could happen frequently as you could imagine the scenario where a user creates a rollover, then immediately mints it as a way to keep the relayerFee for themselves. It later, they decide to delist it, a future queued rollover would be moved to its already-executed position in the rolloverQueue.

Proof of concept

function testCanBuryOtherRollovers() public {
    // create first epoch
    uint40 _epochBegin = uint40(block.timestamp + 1 days);
    uint40 _epochEnd = uint40(block.timestamp + 2 days);
    uint256 _epochId = 1;

    vault.setEpoch(_epochBegin, _epochEnd, _epochId);

    helperDepositInEpochs(_epochId, USER, false);
    helperDepositInEpochs(_epochId, USER2, false);

    // enlist first user in rollover for next epoch
    vm.startPrank(USER);
    vault.enlistInRollover(_epochId, 8 ether, USER);
    vm.stopPrank();

    // enlist second user in rollover for next epoch
    vm.startPrank(USER2);
    vault.enlistInRollover(_epochId, 8 ether, USER2);
    vm.stopPrank();

    // resolve first epoch
    vm.warp(_epochEnd + 1 days);
    vm.startPrank(controller);
    vault.resolveEpoch(_epochId);
    vm.stopPrank();

    // create second epoch
    _epochBegin = uint40(block.timestamp + 1 days);
    _epochEnd = uint40(block.timestamp + 2 days);
    _epochId = 2;

    vault.setEpoch(_epochBegin, _epochEnd, _epochId);

    // simulate prev epoch win
    stdstore
        .target(address(vault))
        .sig("claimTVL(uint256)")
        .with_key(1)
        .checked_write(1000 ether);

    // first user mints their rollover then delists
    vm.startPrank(USER);
    vault.mintRollovers(_epochId, 1);
    vault.delistInRollover(USER);
    vm.stopPrank();

    // additional rollover mints
    vm.startPrank(relayer);
    vault.mintRollovers(_epochId, 5000);
    vm.stopPrank();

    // assert only first user was rolled over
    assertEq(vault.rolloverAccounting(_epochId), 1);
    assertEq(vault.balanceOf(USER2, _epochId), 0);
}

Code Snippet

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

Tool used

Manual Review

Recommendation

A new way to handle delisted rollovers would need to be implemented. You could potentially reduce a rollover's QueueItem.assets to 0 to indicate that it has been delisted. This would allow mintRollovers to skip over it.

Duplicate of #72