hickuphh3 - depositFee can be bypassed via deposit queue

[sherlock-admin]

hickuphh3

medium

depositFee can be bypassed via deposit queue

Summary

The deposit fee can be circumvented by a queue deposit + mintDepositInQueue() call in the same transaction.

Vulnerability Detail

A deposit fee is charged and increases linearly within the deposit window. However, this fee can be avoided if one deposits into the queue instead, then mints his deposit in the queue.

POC

Assume non-zero depositFee, valid epoch _id = 1. At epoch end, instead of calling deposit(1, _assets, 0xAlice), Alice writes a contract that performs deposit(0,_assets,0xAlice) + mintDepositInQueue(1,1) to mint her deposit in the same tx (her deposit gets processed first because FILO system) . She pockets the relayerFee, essentially paying zero fees instead of incurring the depositFee.

Impact

Loss of protocol fee revenue.

Code Snippet

https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Carousel/Carousel.sol#L494-L500
https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Carousel/Carousel.sol#L332-L333
https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Carousel/Carousel.sol#L354

Tool used

Manual Review

Recommendation

Because of the FILO system, charging the dynamic deposit fee will be unfair to queue deposits as they're reliant on relayers to mint their deposits for them. Consider taking a proportion of the relayer fee.

[3xHarry]

This is a valid issue. We will apply depositFee to all mints (queue and direct). However, given that queue has the potential to affect when users's shares are minted because of FILO, min deposit has to be raised for the queue, to make it substantially harder to DDoS the queue. Minimizing DDoS queue deposits will lead to queue deposits getting the least fees as relayers can mint from the first second the epoch is created.

[3xHarry]

fix PR: Y2K-Finance/Earthquake#126

[3xHarry]

@IAm0x52 to elaborate on this issue: relayers are incentivized to mint the depositQueue from the second a new epoch is created to extract the most amount of relayerFees. In fact Y2K will have a build in relayerInfra into the deployment process. The assumption is, that queueDeposit users will pay a minimal Fee. The attack factor of the queue beeing to long leading to prolonged queue deposit executions will be mitigated by adding a significant deposit requirement for queue deposits. These measures will mitigate high deposit Fees for Queue deposits as well as prevent late direct depositors using the queue to evade the depositFee.

[jacksanford1]

Bringing in this discussion from Discord:

0x52

As a follow up for PR126. You keep the minRequiredDeposit modifier on enlistInRollover but the way you modified it, it can only apply if epochId == 0 but enlistInRollover doesn't work for epochId == 0 so the modifier is useless on that function. My suggestion would be to either remove it if you no longer need that protection or make a new modifier specifically designed for enlistInRollover

3xHarry

regarding [issue] 75 / PR 126 fixed in Y2K-Finance/Earthquake@9c65916

[IAm0x52]

Fix looks good. enlistInRollover now applies a minimum deposit requirement