kenzo - Depositor can not rollover 2 different times for the same epoch

[sherlock-admin]

kenzo

medium

Depositor can not rollover 2 different times for the same epoch

Summary

A user may try to rollover assets from a certain epoch, and after the rollover has been minted, try to rollover more assets, or assets from another epoch.
His second enlistInRollover transaction will succeed, but the assets will actually never be rolled over.

Vulnerability Detail

The rollover queue saves the last index reached while minting rollovers for the next epoch.

If a user asks to change his previous rollover request, his previous entry in the queue will be edited.

But then, if his first request was already minted,
rolloverAccounting[_epochId] will be already further ahead of his request,
and his rollover request will not be minted.

POC:

• Hagbard won epoch 2.
• He enlists to rollover 100 assets from epoch 2 to epoch 3.
• His request is minted by somebody else calling mintRollovers.
• He then decides to rollover an additional 100 assets to epoch 3.
• He asks to enlist, and the enlist function updates his previous entry correctly.
• But if the minting function is again called, it will not go over his earlier index again, as rolloverAccounting[_epochId] is already further ahead, and so Hagbard's assets will not be rolled over.

Impact

The user assumed that his rollover will be minted, as the transaction has succeeded, while actually his funds will just remain sitting idle in the contract.
While the principal can still be delisted and withdrawn, the user will lose the rewards (collateral/premium) that were to be his had his rollover been minted.
This is why I believe a medium severity is justified.

Code Snippet

If a user has already an entry in the queue, upon subsequent enlistings, that entry will just be updated:

        if (ownerToRollOverQueueIndex[_receiver] != 0) {
            // if so, update the queue
            uint256 index = getRolloverIndex(_receiver);
            rolloverQueue[index].assets = _assets;
            rolloverQueue[index].epochId = _epochId;

The rollover minting function always starts from the previous index reached:

        uint256 index = rolloverAccounting[_epochId];

This creates the problem described above.

Tool used

Manual Review

Recommendation

It looks like the rollover queue needs to be reworked; unfortunately I don't have enough time at the moment to think of the full solution; if I'll have time, I'll try to get back to this.
At the very least, if a user was already rolled over (rollover minted) to the next epoch, his subsequent rollover requests (for the same epoch) should revert, so he will know the assets will not be rolled over.

[KenzoAgada]

Escalate for 10 USDC

As far as I see, this issue describes a valid bug with a similar impact to another issue that was accepted (#72).
(Note: I am writing a similar escalation for both #262 and #253. Although my opinion is that this specific issue is a little less "important" than #262).
Important note: if this escalation is to be accepted, I noticed that there are at least 2 more issues which describe this same bug: #380 which was excluded, and #402 which was wrongly duped.

The escalation:
One of the main additions of this codebase after the previous C4 audit is the Carousel contract and it's Rollover functionality.
This bug that I found breaks the core Rollover functionality of the Carousel, and causes loss of user yield.

In this issue's scenario,
The contract tells the user that his request to enlist in rollover was successful,
but the contract will then not actually mint the rollover.

After a rollover request, rollovers should programatically reroll themselves automatically in each further epoch.

Therefore a used might come back after 6 months to withdraw his winnings,
only to discover his funds have been sitting idle for 6 months.

So as the Carousel's core functionality is impacted, and user yield is lost, it seems to me that a medium severity is appropriate.

[sherlock-admin]

Escalate for 10 USDC

As far as I see, this issue describes a valid bug with a similar impact to another issue that was accepted (#72).
(Note: I am writing a similar escalation for both #262 and #253. Although my opinion is that this specific issue is a little less "important" than #262).
Important note: if this escalation is to be accepted, I noticed that there are at least 2 more issues which describe this same bug: #380 which was excluded, and #402 which was wrongly duped.

The escalation:
One of the main additions of this codebase after the previous C4 audit is the Carousel contract and it's Rollover functionality.
This bug that I found breaks the core Rollover functionality of the Carousel, and causes loss of user yield.

In this issue's scenario,
The contract tells the user that his request to enlist in rollover was successful,
but the contract will then not actually mint the rollover.

After a rollover request, rollovers should programatically reroll themselves automatically in each further epoch.

Therefore a used might come back after 6 months to withdraw his winnings,
only to discover his funds have been sitting idle for 6 months.

So as the Carousel's core functionality is impacted, and user yield is lost, it seems to me that a medium severity is appropriate.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[dmitriia]

The rolloverAccounting[_epochId] is per epoch, so it's back to zero for the next epoch. The same query is to be repeated over and over again in this design.

The enlistInRollover() index treating, #2 and dups, is a valid high as there it is accounting malfunction with material impacts. In my understanding the fact that user cannot enter the same epoch mint queue twice is a design limitation. There is no impact of user funds being not utilized as the next epoch enlistInRollover() will use them.

This way #262, #253, #380, #402 are invalid (where there is a statement that user funds will be idle for a prolonged amount of time, which isn't correct) or informational (where it is that user cannot mint twice to the same target epoch). #402 is incorrectly duped to #2 indeed, should be informational.

[hrishibhat]

Escalation rejected

Not a valid issue based on the Lead Judge's comment above as this is a design choice to mint rollover once per epoch

[sherlock-admin]

Escalation rejected

Not a valid issue based on the Lead Judge's comment above as this is a design choice to mint rollover once per epoch

This issue's escalations have been rejected!

Watsons who escalated this issue will have their escalation amount deducted from their next payout.