0Kage - Updating rollover details in enlistInRollover for one user overrides existing rollover details of another user

[sherlock-admin]

0Kage

high

Updating rollover details in enlistInRollover for one user overrides existing rollover details of another user

Summary

enlistInRollover is used to create/update new/existing rollover requests by users for a given epoch Id. Current implementation assigns a wrong roll-over index when updating roll-over details. This can be exploited by a malicious user who can delist the roll-over details placed by other users & even worse, steal funds of other users by replacing receiver address with his own

Vulnerability Detail

enlistRollover in Carousel allows users to place new roll-over requests or update existing roll-over requests.

Note that, when an existing roll-over request for a receiver is updated, on line 268, the ownerToRollOverQueueIndex for that receiver is updated to the last element in the rolloverQueue although the position of the actual request is unchanged. getRolloverIndex for the receiver will now point to the last index that belongs to different user.

User can then call delistInRollover and delete roll-over requests of other users. Another serious attack here is, a user can overwrite the receiver address of a roll-over request that has a higher asset value. Assets and emissions of another receiver can get minted to malicious user who changed the receiver address when mintRollovers is called.

    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);
            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 -> this should only be done for new request

        emit RolloverQueued(_receiver, _assets, _epochId);
    }

Impact

A user can delete other users roll-over requests at will & even worse, can change receiver address of other user's roll-over requests with higher asset value

Code Snippet

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

Tool used

Manual Review

Recommendation

The line 268 is only applicable for new roll-over requests and should be inside the else rather than outside.

     if (ownerToRollOverQueueIndex[_receiver] != 0) {
            // if so, update the queue
            uint256 index = getRolloverIndex(_receiver);
            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 - this is inside the `else` flow
        }

Duplicate of #2