cccz - mintRollovers should require entitledShares >= relayerFee

[sherlock-admin]

cccz

medium

mintRollovers should require entitledShares >= relayerFee

Summary

mintRollovers should require entitledShares >= relayerFee

Vulnerability Detail

In mintRollovers, the rollover is only not skipped if queue[index].assets >= relayerFee,

                if (entitledShares > queue[index].assets) {
                    // skip the rollover for the user if the assets cannot cover the relayer fee instead of revert.
                    if (queue[index].assets < relayerFee) {
                        index++;
                        continue;
                    }

In fact, since the user is already profitable, entitledShares is the number of assets of the user, which is greater than queue[index].assets, so it should check that entitledShares >= relayerFee, and use entitledShares instead of queue[index].assets to subtract relayerFee when calculating assetsToMint later.

Impact

This will prevent rollover even if the user has more assets than relayerFee

Code Snippet

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

Tool used

Manual Review

Recommendation

Change to

                if (entitledShares > queue[index].assets) {
                    // skip the rollover for the user if the assets cannot cover the relayer fee instead of revert.
-                   if (queue[index].assets < relayerFee) {
+                   if (entitledShares < relayerFee) {
                        index++;
                        continue;
                    }
...
-                   uint256 assetsToMint = queue[index].assets - relayerFee;
+                   uint256 assetsToMint = entitledShares - relayerFee;

[3xHarry]

fix PR: Y2K-Finance/Earthquake#136

[IAm0x52]

Needs additional changes. L423 doesn't make sense to me. queue[index].assets is in shares and entitledAmount isn't but they are subtracted directly.

[3xHarry]

@IAm0x52 thx for your comment, basically since QueueItem.asset (later renamed to shares) represents share of the epoch, i converted relayerFee which is denominated in underlying asset to shares for that epoch.
Later i realized that Users only want to rollover their original deposit value, therefore i burn the original deposit Value and mint this value - relayerFeeInShares into the next epoch, the winnings sahres amount are left in the epoch to be withdrawn

[jacksanford1]

Bringing in some Discord discussion:

0x52

For 293 your comment then conflicts with how you take the fee because you're charging the user the relayer fee for epoch n+1 but you're using epoch n to estimate. You should just take the fee directly not convert it into shares

3xHarry

@0x52 thx for raising this concern:
uint256 relayerFeeInShares = previewAmountInShares(queue[index].epochId, relayerFee);
converts relayerFee into amount of shares of prev epoch (epoch users wants to rollover collateral from)

uint256 assetsToMint = queue[index].assets - relayerFeeInShares;
assets represent shares in prev epoch and arrimetic operation is done in same denominator (shares in queue[index].epochId)

0x52

Shares and assets are 1:1 for the open epoch correct?

So imagine this scenario. You deposit 100 asset into epoch 1 to get 100 shares in epoch 1. Now you queue them into the rollover.

Epoch 1 ends with a profit of 25% which means your 100 shares are now worth 125. 80 shares are burned (worth 100 assets) leaving the user with 20 shares for epoch 1.

If the relayer fee is 10 then it will be converted to 8 shares of epoch 2. But epoch 2 is still 1:1 with assets so it's only taking 8 assets from the user but sending them 10 asset as the relayer fee
So you either need to reduce epoch 1 shares by 8 (i.e. leave the user with 12 shares) or you need to reduce assetsToMint by relayer fee directly (i.e. only mint 90 to epoch 2)

[jacksanford1]

Bringing in some discussion from Y2K's repo:

3xHarry

@IAm0x52 thx for noticing the relayerFeeInShares Bug I will close this PR, but fix can be observed in 9165674

Y2K-Finance/Earthquake#136 (comment)

[IAm0x52]

Fix looks good. Fee is no longer converted since epoch in which fee is removed is always 1:1