csanuragjain - User deposit may never be entertained from deposit queue

[sherlock-admin]

csanuragjain

medium

User deposit may never be entertained from deposit queue

Summary

Due to FILO (first in last out) stack structure, while dequeuing, the first few entries may never be retrieved. These means User deposit may never be entertained from deposit queue if there are too many deposits

Vulnerability Detail

1. Assume User A made a deposit which becomes 1st entry in depositQueue
2. Post this X more deposits were made, so depositQueue.length=X+1
3. Relayer calls mintDepositInQueue and process X-9 deposits

 while ((length - _operations) <= i) {
            // this loop impelements FILO (first in last out) stack to reduce gas cost and improve code readability
            // changing it to FIFO (first in first out) would require more code changes and would be more expensive
            _mintShares(
                queue[i].receiver,
                _epochId,
                queue[i].assets - relayerFee
            );
            emit Deposit(
                msg.sender,
                queue[i].receiver,
                _epochId,
                queue[i].assets - relayerFee
            );
            depositQueue.pop();
            if (i == 0) break;
            unchecked {
                i--;
            }
        }

4. This reduces deposit queue to only 10
5. Before relayer could process these, Y more deposits were made which increases deposit queue to y+10
6. This means Relayer might not be able to again process User A deposit as this deposit is lying after processing Y+9 deposits

Impact

User deposit may remain stuck in deposit queue if a large number of deposit are present in queue and relayer is interested in dequeuing all entries

Code Snippet

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

Tool used

Manual Review

Recommendation

Allow User to dequeue deposit queue based on index, so that if such condition arises, user would be able to dequeue his deposit (independent of relayer)

[3xHarry]

depositing into queue should count as committing to an epoch. By giving the user the ability to delist his queue he could take advantage of market movements. However, we will raise min deposit for the queue to make DDoS very expensive.

[twicek]

Escalate for 10 USDC

My issues #62 and #63 are both marked as duplicate of this issue when only #63 is actually a duplicate.
#63 is a duplicate of #174 who both relate to how queued deposits can get stuck in the deposit queue for various reasons.
#62 however, does not describe anything related to both the deposit queue and the separate fact that a there is DoS attack vector. Instead, it relates to how relayers can get griefed because unrollable rollover items in the rollover queue can aggregate and lead them to not get paid for their work. In the duplicates, that I will cite below, this issue is achieved in various different ways but they all lead to the same impact.

Therefore, to reiterate, the #62 and #63 are different because they don't involve the same states, attack vector and users.
#63 involve the deposit queue and a DoS attack that lead to economic damage for regular users of the protocol.
#62 involve the rollover queue and a griefing attack that lead to economic damage for relayers.

In my judging repos I market as duplicate of #63:
#79 #82 #114 #174 #220 #274 #295 #317 #342 #431 #447
and as duplicate of #62:
#80 #218 #235 #275 #284 #309 #393 #411 #475

From what I have seen all or almost all this issue are present here as duplicate of #174. A lot of people submitted them as different issues because they are indeed completely different.
I might have made some mistakes in my judging, but it's mostly consistent with what I say above. Specifically, I missed #176 for which I partially agree with the escalation of securitygrid that it should not be a duplicate of #174 but it also should not be a solo finding because it is a duplicate of #62 and its duplicates.

[sherlock-admin]

Escalate for 10 USDC

My issues #62 and #63 are both marked as duplicate of this issue when only #63 is actually a duplicate.
#63 is a duplicate of #174 who both relate to how queued deposits can get stuck in the deposit queue for various reasons.
#62 however, does not describe anything related to both the deposit queue and the separate fact that a there is DoS attack vector. Instead, it relates to how relayers can get griefed because unrollable rollover items in the rollover queue can aggregate and lead them to not get paid for their work. In the duplicates, that I will cite below, this issue is achieved in various different ways but they all lead to the same impact.

Therefore, to reiterate, the #62 and #63 are different because they don't involve the same states, attack vector and users.
#63 involve the deposit queue and a DoS attack that lead to economic damage for regular users of the protocol.
#62 involve the rollover queue and a griefing attack that lead to economic damage for relayers.

In my judging repos I market as duplicate of #63:
#79 #82 #114 #174 #220 #274 #295 #317 #342 #431 #447
and as duplicate of #62:
#80 #218 #235 #275 #284 #309 #393 #411 #475

From what I have seen all or almost all this issue are present here as duplicate of #174. A lot of people submitted them as different issues because they are indeed completely different.
I might have made some mistakes in my judging, but it's mostly consistent with what I say above. Specifically, I missed #176 for which I partially agree with the escalation of securitygrid that it should not be a duplicate of #174 but it also should not be a solo finding because it is a duplicate of #62 and its duplicates.

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]

Agree re 62, here the issues were grouped per 'pop' allowing for various manipulations.

[hrishibhat]

Escalation accepted

After reviewing the issues and all its duplicates, given the complexity as well as the similarities between these issues, the fair move would be to split up these issues into two categories based on the functions of the root cause: mintDepositInQueue & mintRollovers.
These two categories of duplicates primarily contain issues related to large queue lengths resulting in dos or insufficient relayer incentives from the same root cause.

[sherlock-admin]

Escalation accepted

After reviewing the issues and all its duplicates, given the complexity as well as the similarities between these issues, the fair move would be to split up these issues into two categories based on the functions of the root cause: mintDepositInQueue & mintRollovers.
These two categories of duplicates primarily contain issues related to large queue lengths resulting in dos or insufficient relayer incentives from the same root cause.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

[IAm0x52]

Issue has been acknowledged by sponsor