sherlock-audit / 2023-03-Y2K-judging

7 stars 1 forks source link

minhtrng - Faulty index update of ownerToRollOverQueueIndex could break rollover #482

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

minhtrng

high

Faulty index update of ownerToRollOverQueueIndex could break rollover

Summary

When enlisting rollovers, the index of the receiver is always updated even when it has not changed, allowing for manipulation of other queue items. This allows to prevent others from rolling over, or breaking the rollover completely.

Vulnerability Detail

The function Carousel.enlistInRollover always updates the ownerToRollOverQueueIndex even if the queue item of the _receiver has only been updated and not freshly inserted:

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;

This means that after calling the function, the _receiver has control over the last queue item. The attacker could delist it to prevent the actual owner from rolling over, but they could also update it to an asset value that they hold (to pass checks) but that is higher than the actual owner has. This would break the function mintRollOvers due tue not being able to burn sufficient assets:

//in mintRollovers, will fail due to insufficient balance
_burn(
    queue[index].receiver,
    queue[index].epochId,
    queue[index].assets
);

If the attacker owns both addresses, then there is also no chance of the actual owner delisting his item to fix the issue.

Impact

Harming users and breaking core functionality.

Code Snippet

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

Tool used

Manual Review

Recommendation

The update of ownerToRollOverQueueIndex should be within the else-branch, when a new item is inserted.

Duplicate of #2