Skip to content
This repository has been archived by the owner on Oct 1, 2023. It is now read-only.

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

Closed
sherlock-admin opened this issue Mar 28, 2023 · 0 comments
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label High A valid High severity issue Reward A payout will be made for this issue

Comments

@sherlock-admin
Copy link
Contributor

sherlock-admin commented Mar 28, 2023

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

@github-actions github-actions bot closed this as completed Apr 3, 2023
@github-actions github-actions bot added High A valid High severity issue Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label labels Apr 3, 2023
@sherlock-admin sherlock-admin added the Reward A payout will be made for this issue label Apr 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label High A valid High severity issue Reward A payout will be made for this issue
Projects
None yet
Development

No branches or pull requests

1 participant