sherlock-audit / 2023-03-Y2K-judging

7 stars 1 forks source link

chainNue - Rollover doesn't check next epoch `minRequiredDeposit` (relayerFee) #495

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

chainNue

medium

Rollover doesn't check next epoch minRequiredDeposit (relayerFee)

Summary

rollover doesn't check next epoch minRequiredDeposit in scenario where the next epoch's relayerFee is increasing, resulting a failed mint of the rollover

Vulnerability Detail

When user want to enlist rollover, they will call enlistInRollover() which have modifier minRequiredDeposit of asset they are trying to rollover. but then mintRollovers doesn't check again this relayerFee (because it might be increased).

if for example a user deposit a minimum amount of deposit, so to speak, they just deposit right amount just to pass minRequiredDeposit, then when the next epoch's relayerFee is greater than previous relayerFee, the mintRollovers() will failed.

File: Carousel.sol
436:                     uint256 assetsToMint = queue[index].assets - relayerFee;

since the asset is previously equal to relayerFee (on previous epoch), and if next epoch the relayerFee is increasing, this will failed / reverted without any handling.

Impact

User will failed to rollover their position because next epoch's relayerFee might be larger than previous epoch's relayerFee.

Code Snippet

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

Tool used

Manual Review

Recommendation

When calling enlistInRollover need to have a check on if the asset amount will pass the minRequiredDeposit (or not less than next epoch's relayerFee). because if not it will successfully enlisted in rollover, but later when trying to mint, it will failed.

sherlock-admin commented 1 year ago

Escalate for 10 USDC

First, please ignore this issue title because I think it's not clearly describe the issue (and too short), so read from the Summary, Vulnerability Detail, and the Impact of this issue.

This is exactly the same as #355 (which is a duplicate of #418), and it is about rollover will failed/grieving/bricked.

The rollover mechanism would forever be bricked for entries after the bad entry (the automatic generated entry where assets <= relayerFee). is same as this issue

the problematic code

uint256 assetsToMint = queue[index].assets - relayerFee;

is also the same in

  • 355

  • 418

You've deleted an escalation for this issue.

sherlock-admin commented 1 year ago

Escalate for 10 USDC

The previous escalation is wrong...

This is exactly the same as #355 (which is a duplicate of #418), and it is about rollover will failed/grieving/bricked.

The issues the escalator links to point out a different issue.

This current issue is invalid because this check will skip over any entries where queue[index].assets - relayerFee will underflow.

What I described in the "bonus" section of my issue, and what Bernd described in his issue, is that there's a special edge case where the vault's TVL is 0 and there will be a division by 0 error. That division happens before the abovementioned check. So that is a similar, but different, issue than the one that is pointed out here (and which is invalid as is taken care of by the abovementioned check).

You've deleted an escalation for this issue.

chainNue commented 1 year ago

Escalate for 10 USDC

The previous escalation is wrong...

This is exactly the same as #355 (which is a duplicate of #418), and it is about rollover will failed/grieving/bricked.

The issues the escalator links to point out a different issue.

This current issue is invalid because this check will skip over any entries where queue[index].assets - relayerFee will underflow.

What I described in the "bonus" section of my issue, and what Bernd described in his issue, is that there's a special edge case where the vault's TVL is 0 and there will be a division by 0 error. That division happens before the abovementioned check. So that is a similar, but different, issue than the one that is pointed out here (and which is invalid as is taken care of by the abovementioned check).

After reading it again I think you're right Kenzo, thanks for the correction, I missed that point, thus removed my escalation. Congrats for the result.

KenzoAgada commented 1 year ago

Congrats for the result.

Thank you mate 🙂