evan - Relayers for the rollover queue can be griefed

[sherlock-admin]

evan

medium

Relayers for the rollover queue can be griefed

Summary

It's possible to create persistent entries in the rollover queue to waste gas for the relayer without compensation (for both the protocol and the relayer).

Vulnerability Detail

Observation 1: a rollover entry permanently stays in the queue unless the user voluntarily overwrites or delist it. (You can look through all the lines of code that modify the rollover queue)

https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Carousel/Carousel.sol#L447
Observation 2: relayer is only rewarded for successful rollovers.

https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Carousel/Carousel.sol#L372
MintRollover uses rolloverAccounting to keep track of the last entry of the epoch that has been rolled over.

https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Carousel/Carousel.sol#L453
Observation 3: rolloverAccounting is only updated when there has been at least 1 successful rollovers.

https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Carousel/Carousel.sol#L436
It's trivial to create an entry that cannot be rolled over (I'll refer to this as a stale entry). Simply create an entry with assets = relayerFee. After 1 rollover, assets will be 0.

https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Carousel/Carousel.sol#L403
^This condition will always be satisfied for all subsequent rollovers.

Impact

Consider the following attack. A user with 100 accounts creates a stale rollover entry for each one of them in the same transaction. This results in a block of 100 stale entries.

Suppose, for example, the queue layout is something like this after a few epochs:
50 random entries | 100 stale entries | 50 random entries.

Now, suppose after a few calls to mintRollovers, rolloverAccounting is now pointing to entry 51 (beginning of the stale entries). Then, suppose 10 relayers call mintRollovers with _operations = 20. None of the relayers will get any rewards, rolloverAccounting will still be pointing to entry 51, effectively wasting 200 iterations.

Code Snippet

See Vulnerability Detail

Tool used

Manual Review

Recommendation

https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Carousel/Carousel.sol#L453
I don't think this condition is really necessary. Removing this condition should at least ensure that relayer's work is always productive for the protocol.

To mitigate the problem of stale entries, one possible solution is to allow the contract itself to call delistInRollover when it detects a stale entry.

Duplicate of #172