ltyu - Rollover delisting causes underflow

[sherlock-admin]

ltyu

medium

Rollover delisting causes underflow

Summary

When a rollover is delisted, it can cause an underflow, and therefore block rollover minting.

Vulnerability Detail

Currently, mintRollovers() relies the rolloverQueue.length in calculating the max number of _operations:
https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Carousel/Carousel.sol#L371-L372

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

After a user's rollover is complete, they are able to delist. This is problematic because rolloverQueue.length will decrease when users call delistInRollover(). Eventually, rolloverQueue.length will be small enough that the calculation will underflow.

Consider the case when there are 3 items in rolloverQueue:

1. User calls mintRollovers(_epochId, 2), and tries to mint 2 rollovers.
2. The following local variables will be used
   • length = rolloverQueue.length = 3
   • index = rolloverAccounting[_epochId] = 0
3. At the end, this storage will be set
   • rolloverAccounting[_epochId] = index = 2 (due to index++)
4. Two of the rollover users delist. The rolloverQueue.length will now be 1
5. User attempts to mint the last rollover. The following local variables will be used
   • length = rolloverQueue.length = 1
   • index = rolloverAccounting[_epochId] = 2
6. With the lowest _operation value as possible, 1, the if condition will execute since index +_operations > length. _operations will be calculated as length - index = 1 - 2, which will lead to underflow.

Impact

This is a medium impact. If users are partially rolled over, and there are delists, the other users will not be rolled over on a timely basis i.e., epoch may start.

Code Snippet

Here is a unit test. Add this to CarouselTest.t.sol, and run forge test --match-test testMintAndThenDelistRollover

function testMintAndThenDelistRollover() public {
	// 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);

	// 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);

	// mint rollovers, this will change the index to 1
	vault.mintRollovers(_epochId, 2);
	// this will change the index to 1
	assertEq(vault.rolloverAccounting(_epochId), 2);

	// Delist USER1 and USER2
	vm.prank(USER);
	vault.delistInRollover(USER);
	vm.prank(USER2);
	vault.delistInRollover(USER2);
	// rolloverQueue.length will be 1

	// Should underflow when trying to rollover USER3
	vm.expectRevert();
	vault.mintRollovers(_epochId, 1);

}

Tool used

Manual Review

Recommendation

Consider some way of tracking if a user has rolled over already, possibly by deleting the ownerToRollOverQueueIndex[_owner] after it has been successfully rolled over.

Duplicate of #72