0xnirlin - Critical Indexing Flaw in Enlist Function that lead to user information collision in ownerToRollOverQueueIndex mapping and prevent user from calling withdraw and safeTransferFrom in Carousel.sol

[sherlock-admin]

0xnirlin

high

Critical Indexing Flaw in Enlist Function that lead to user information collision in ownerToRollOverQueueIndex mapping and prevent user from calling withdraw and safeTransferFrom in Carousel.sol

Summary

When multiple users are enlisted using the enlistRollOver functions, wrong index will be assigned to a user's data when they wish to update their information for the second time. This can lead to unexpected behavior in the transferFrom and withdraw functions that rely on the mapping and its associated modifier. The impact of this vulnerability could result in not allowing users from transferring or withdrawing the assets

Vulnerability Detail

Consider the scnerio where we have USER1, USER2, and USER3 and each indivisually call the elistInRollover() function with the epochId, receiver and assets lets say 8 eth for the first time.
Now in mapping ownerToRollOverQueueIndex we have USER1 = 1, USER2 = 2, and USER3 = 3.
But USER1 calls the enlistInRollover() but this time assets set as 20ETH and USER2 does the same, their respective information will be update in the rollOverQueue but here is the catch :
https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Carousel/Carousel.sol#L253
We will enter this if block and as the length of the queue remains same on the following line USER1 and USER2 address will be mapped to USER3 address on the following line so now USER2 and USER1 are mapped to USER3 index and all have index of 2:
https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Carousel/Carousel.sol#L268
As above line is outside if else block will be executed for all calls to function.

Now we have the following modifier that is dependent on mapping and being used in transferFrom() and withdraw() function

   modifier notRollingOver(
        address _receiver,
        uint256 _epochId,
        uint256 _assets
    ) {
        if (ownerToRollOverQueueIndex[_receiver] != 0) {
            QueueItem memory item = rolloverQueue[getRolloverIndex(_receiver)];
            if (
                item.epochId == _epochId &&
                (balanceOf(_receiver, _epochId) - item.assets) < _assets
            ) revert AlreadyRollingOver();
        }
        _;
    }

this modifier can revert as the USER1 and USER2 set the asset on second update to 20ETH but here we will be accessing the data of USER3 whose asset value is 8ETH and same for the epochId, we will be wrongly accessing the epochId of USER3 for USER1 and USER2.

Reverting on this modifier means that it will prevent the users from calling the transferFrom() and withdraw function as both function using this modifier, even though if notRolling USER1 and USER2 will be prevented from withdrawing or transferring the emissions and assets. Which will lock the users assets within contract.

As we can see here the modifier on both the functions:

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

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

Impact

This vulnerability as prevent the user from calling the safeTransferFrom() and Withdraw() even if they are not rollingOver and the user their mapping is pointing to is actually rolling over.
Secondly can cause issues when the ui is integrated with smart contract and will result in showing wrong information for the user. As we say USER1 and USER2 updated their asset to 20ETH but it will still show the epochId and asset of USER3 for the USER1 and USER2 and all other such clashes.

Code Snippet

Tool used

Foundry and Forge

Recommendation

Add some variation of following test, it shows that multiple user have same index and is passing.

    function testEnlistRolloverMultiple() 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);
                helperDepositInEpochs(_epochId,USER3, false);

        vm.warp(_epochBegin - 10 minutes);

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

        // enlist in rollover for next epoch
        vm.startPrank(USER);
        //_epochId == epoch user is depositing in / amount of shares he wants to rollover
        vault.enlistInRollover(_epochId, 8 ether, USER);
        vm.stopPrank();

        vm.startPrank(USER2);
        vault.enlistInRollover(_epochId, 8 ether, USER2);
        vm.stopPrank();

        vm.startPrank(USER3);
        vault.enlistInRollover(_epochId, 8 ether, USER3);
                vm.stopPrank();

         vm.startPrank(USER);
        vault.enlistInRollover(_epochId, 9 ether, USER);
        vm.stopPrank();

        vm.startPrank(USER2);
        vault.enlistInRollover(_epochId, 9 ether, USER2);
                vm.stopPrank();

        assertEq(vault.getRolloverIndex(USER),vault.getRolloverIndex(USER2));
        assertEq(vault.getRolloverIndex(USER),vault.getRolloverIndex(USER3));
        assertEq(vault.getRolloverIndex(USER2),vault.getRolloverIndex(USER3));

    }

Mitigation Steps

Change the logic to correctly update the index and mapping.

Duplicate of #2