Emmanuel - Attacker can rollover more shares than a user's intention or cause the mintRollovers function to be unusable.

[sherlock-admin]

Emmanuel

high

Attacker can rollover more shares than a user's intention or cause the mintRollovers function to be unusable.

Summary

Due to a bug in enlistInRollover function, an attacker can rollover more shares than a user's intention or cause a permanent DoS in the mintRollovers function, making it unusable.

Vulnerability Detail

In the enlistInRollover function of carousel contract,

    function enlistInRollover(
        uint256 _epochId,
        uint256 _assets,
        address _receiver
    ) public epochIdExists(_epochId) minRequiredDeposit(_assets) {
        // check if sender is approved by owner
        if (
            msg.sender != _receiver &&
            isApprovedForAll(_receiver, msg.sender) == false
        ) revert OwnerDidNotAuthorize(msg.sender, _receiver);
        // check if user has enough balance
        if (balanceOf(_receiver, _epochId) < _assets)
            revert InsufficientBalance();

        // check if user has already queued up a rollover
        if (ownerToRollOverQueueIndex[_receiver] != 0) {
            // if so, update the queue
            uint256 index = getRolloverIndex(_receiver); //@audit index in rolloverQueue for a user is gotten by subtracting 1 from `ownerToRolloverQueueIndex[user]`
            rolloverQueue[index].assets = _assets;
            rolloverQueue[index].epochId = _epochId;
        } else {
            // if not, add to queue
            rolloverQueue.push(
                QueueItem({
                    assets: _assets,
                    receiver: _receiver,
                    epochId: _epochId
                })
            );
        }
        ownerToRollOverQueueIndex[_receiver] = rolloverQueue.length; //@audit always updated for every _receiver because it is outside the else block

        emit RolloverQueued(_receiver, _assets, _epochId);
    }

The public state mapping, ownerToRolloverQueueIndex for a _receiver is always updated to the last index in the rolloverQueue array whenever the function is called. This can allow an attacker to override the epochId and _assets amount that other users originally set.

Let's say an attacker has 500 _assets from an _epochId 1500, and innocent Bob has 200 _assets from an _epochId 1200,

Attacker enlists some _assets for rollover, calling enlistInRollover(1500,500,attackerAddress) which will push QueueItem({assets:500,receiver:attackerAddress,epochId:1500}) to the state array rolloverQueue. Now, ownerToRolloverQueueIndex[attackerAddress] will be set to 1 (i.e. rolloverQueue.length), and the attacker index in the rolloverQueue array is 0

Bob then enlists his 200 _assets, calling enlistInRollover(1200,200,bobAddress) which will push QueueItem({assets:200,receiver:bobAddress,epochId:1200}) to the state array rolloverQueue. ownerToRolloverQueueIndex[bobddress] will be set to 2 (i.e. rolloverQueue.length), and bob's index in the rolloverQueue array is 1

Attacker sees that Bob has been added to rolloverQueue, and then calls enlistInRollover(1500,500,attackerAddress) again. This would check if attacker has already been added to the rolloverQueue, if (ownerToRollOverQueueIndex[_receiver] != 0) , then updates the QueueItem the attacker previously enlisted at index 0, BUT AFTER THAT, ownerToRolloverQueueIndex[attackerAddress] GETS UPDATED AGAIN to the length of the array(in this case, 2).

Attacker will then call the function again with the same parameters, but this time, instead of updating the index position in the rolloverQueue of the attacker(which was originally 0), the index position 1 will be updated because index is calculated as ownerToRolloverQueueIndex[attackerAddress]-1. Therefore, bob's QueueItem is updated to QueueItem({assets:500,receiver:bobAddress,epochId:1500}).

This bug can cause two things:
Remember that Bob initially listed 200 _assets for rollover

• If Bob has up to 500 _assets, it gets rolled over against his will.
• If Bob does not have up to 500 _assets, there will PERMANENTLY be a DoS in the mintRollover function because _burn(queue[index].receiver,queue[index].epochId,queue[index].assets) (LINE 409) will always revert. (This is more disastrous).
  Since Bob can help solve the DoS issue by getting more _assets, Attacker could use an EOA or Contract account instead of Bob to make the attack insoluble

Impact

Attacker can make mintRollovers function unusable by causing a permanent DoS.

Code Snippet

https://github.com/Y2K-Finance/Earthquake/blob/736b2e1e51bef6daa6a5ecd1decb7d156316d795/src/v2/Carousel/Carousel.sol#L238-L271

Tool used

Manual Review

Recommendation

Move ownerToRollOverQueueIndex[_receiver] = rolloverQueue.length; in LINE 268 to the else block to prevent updating ownerToRollOverQueueIndex[_receiver] for those that have previously called the function.

Duplicate of #2