mstpr-brainbot - Rollover Queue Manipulation

[sherlock-admin]

mstpr-brainbot

high

Rollover Queue Manipulation

Summary

An attacker can exploit the 'mintDepositQueue' function to create both rolling over and non-rolling over positions, which can lead to issues during epoch wins and rollovers. Specifically, the attacker can cause the rollover queue to fail, prevent others from receiving epoch tokens, and block rollovers.

Vulnerability Detail

The 'mintDepositQueue' function can be exploited by attackers to create both rolling over and non-rolling over positions. For example, in epoch2 when deposits are allowed, an attacker can create two contracts, A.C1 and A.C2. A.C1 deposits 1 Ether for epoch2 and mints 1 epoch2 token, while A.C2 deposits 0.00001 Ether and mints 0.00001 epoch2 token (the smallest possible number up to the fee in a real scenario).

The attacker then enlists A.C1 to the rollover queue, which currently consists only of A.C1. Other users may join the queue, and assume that it now contains A.C1, Alice, and Bob. The attacker then rolls over the second contract, A.C2, to the queue, which is now [A.C1, Alice, Bob, A.C2].

When epoch2 is won and epoch3 is open, users in the queue can claim their epoch3 tokens. A.C1's 1 epoch2 token is burned, and when the loop reaches the _mintShares line, A.C1 re-enters the contracts delist function. This function swaps the last index of the queue with the delisted index, so A.C1 is popped out from the array and the new index[0] is A.C2. However, since A.C1 re-entered the function, it executes the lines after the delisting process, which updates the queue as [(A.C2, 1 token, 3), (Alice, X tokens, 2), (Bob, X tokens, 2)]. This update treats A.C2 as if it has 1 epoch3 token, although it does not.

When epoch4 comes, if someone tries to call "mintRolloverQueue", it will fail because A.C2 does not have 1 epoch3 token, and the burnFrom will fail. This means that the index will not grow, and all others after A.C2 will not be able to get their epoch4 tokens. Rolling over for epoch4 is impossible because A.C2 lacks the requested epoch3 tokens.

This also means that A.C1 now has 1 epoch3 token, while A.C2 still has 0.00001 epoch2 token and a position in the rollover queue. If the attacker wants to roll over to epoch4, they can send (A.C1 is not rolling over so he can send the tokens to A.C2) the 1 epoch3 token from A.C1 to A.C2 and call the mintRolloverQueue to receive epoch4 tokens in A.C2's account. If they choose to withdraw the 1 epoch3 token from A.C1, the entire epoch4 rollover queue will be blocked because A.C2 will not be able to perform the burnFrom.

In addition to this, Attacker can conduct this attack on all epochs making the rollover queues stuck.

Impact

Since this function is ruining the intended behaviour of "mintRolloverQueue" function I'll label it as high!

Code Snippet

https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Carousel/Carousel.sol#L437
Reentering

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/3f610ebc25480bf6145e519c96e2f809996db8ed/contracts/token/ERC1155/ERC1155.sol#L277
Re-enterable point

https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Carousel/Carousel.sol#L444-L445
Index is updated after re-entering to delist

Tool used

Manual Review

Recommendation

Duplicate of #468

[mstpr]

Escalate for 10 USDC

I disagree that this finding should be considered a duplicate of issue #468.

While both issues stem from a malicious onERC1155Received() callback, they represent distinct problems. Griefing is one issue, and re-entrancy is another. Just because they share a common origin does not mean they are duplicates.

The proposed solution for issue #468 seems to involve deleting or overriding the safeCheck function. However, this is a rather broad solution. The protocol could have chosen alternative approaches, such as performing a low-level call, checking the return value, and continuing with the queue loop. In this case, issue #468 would be resolved, but this finding would still be relevant.

Issue #468 results in queue reversion, preventing users from participating in rollover-deposit actions.

This issue, on the other hand, describes a different scenario where an attacker manipulates the rollover queue to their advantage without causing the queue to revert.

While the protocol might choose to override the safeCheck function, this decision should not render this finding as invalid. As mentioned earlier, other solutions for issue #468 could have been implemented, and removing or overriding the safeMint function is not the only option. Other solutions may not prevent this discovery from occurring. Of course, eliminating the safeMint function would address this issue, but as Watsons, our focus should be on examining all aspects of findings without making assumptions about what the protocol may choose to do. If the protocol decided not to override the function and only inspected the target contract's callback hook, this issue would remain valid.

[sherlock-admin]

Escalate for 10 USDC

I disagree that this finding should be considered a duplicate of issue #468.

While both issues stem from a malicious onERC1155Received() callback, they represent distinct problems. Griefing is one issue, and re-entrancy is another. Just because they share a common origin does not mean they are duplicates.

The proposed solution for issue #468 seems to involve deleting or overriding the safeCheck function. However, this is a rather broad solution. The protocol could have chosen alternative approaches, such as performing a low-level call, checking the return value, and continuing with the queue loop. In this case, issue #468 would be resolved, but this finding would still be relevant.

Issue #468 results in queue reversion, preventing users from participating in rollover-deposit actions.

This issue, on the other hand, describes a different scenario where an attacker manipulates the rollover queue to their advantage without causing the queue to revert.

While the protocol might choose to override the safeCheck function, this decision should not render this finding as invalid. As mentioned earlier, other solutions for issue #468 could have been implemented, and removing or overriding the safeMint function is not the only option. Other solutions may not prevent this discovery from occurring. Of course, eliminating the safeMint function would address this issue, but as Watsons, our focus should be on examining all aspects of findings without making assumptions about what the protocol may choose to do. If the protocol decided not to override the function and only inspected the target contract's callback hook, this issue would remain valid.

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.

[hrishibhat]

Escalation rejected

Valid duplicate of #468
Given the range of issues originating from the standard onERC1155Received callback cycle, it is practical to consider these issues together.

[sherlock-admin]

Escalation rejected

Valid duplicate of #468
Given the range of issues originating from the standard onERC1155Received callback cycle, it is practical to consider these issues together.

This issue's escalations have been rejected!

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