0xPkhatri - QueueItem added to rolloverQueue list after attacker add QueueItem can be repeatedly replaced by attacker

[sherlock-admin]

0xPkhatri

high

QueueItem added to rolloverQueue list after attacker add QueueItem can be repeatedly replaced by attacker

---

name: 001-H
about: ""
title: QueueItem added to rolloverQueue list after attacker add QueueItem can be repeatedly replaced by attacker
labels: High
assignees: ""

Summary

The enlistInRollover function in the code contains an error that causes the ownerToRollOverQueueIndex mapping to be updated incorrectly. The error occurs because the code for updating the mapping is placed outside of an else loop, which results in the mapping being updated even when the if statement is run. This can lead to the mapping being updated to the wrong list index when a user has already queued up a rollover and runs enlistInRollover again.

Vulnerability Detail

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;

    emit RolloverQueued(_receiver, _assets, _epochId);
}

(1) Let Attacker run enlistInRollover function first time ownerToRollOverQueueIndex[attacker] == 0 else statement run and it will push QueueItem to rolloverQueue list and update ownerToRollOverQueueIndex[attacker] = rolloverQueue.length;,
assume rolloverQueue.length = 5;

(2) Now normal user come and run enlistInRollover function which push it's QueueItem to rolloverQueue. because user push QueueItem so rolloverQueue.length = 6 and it will update ownerToRollOverQueueIndex[user] = 6.

(3) Attacker run enlistInRollover function one more time because attacker has already queued up a rollover now this time if statement run. and because code(#L268): ownerToRollOverQueueIndex[_receiver] = rolloverQueue.length; is outside if and else loop #L268 line of code will executed and it will update ownerToRollOverQueueIndex[attacker]=6 last element of list which is already assign to user who queued up a rollover previously.

(4) now Attacker run delistInRollover function, in line #L286 code: index = getRolloverIndex(attacker); point to last user QueueItem instead of attacker QueueItem and it will pop previous user QueueItem. also delete ownerToRollOverQueueIndex[attacker]

uint256 index = getRolloverIndex(_owner);
uint256 length = rolloverQueue.length;
if (index == length - 1) {
    // if only one item in queue
    rolloverQueue.pop();
    delete ownerToRollOverQueueIndex[_owner];
}

(5) attacker again run enlistInRollover because delistRollover function delete ownerToRollOverQueueIndex[attacker] now this time attacker can add QueueItem one more time.

(6) if any user add there QueueItem again attacker will follow 3, 4, 5 step again and replace user QueueItem with his QueueItem,and submit multiple QueueItem in rolloverQueue list.

Impact

the attacker can repeatedly add their own QueueItem to the list and replace other users' QueueItems, and causing unexpected behavior in the contract.

Code Snippet

Added in Vulnerability Detail
https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Carousel/Carousel.sol#L238-L271

Tool used

Manual Review

Recommendation

#L268 line of code ownerToRollOverQueueIndex[_receiver] = rolloverQueue.length; move inside the else loop.
Updated code:

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;
    }
    

    emit RolloverQueued(_receiver, _assets, _epochId);
}

Duplicate of #2