Delvir0 - Due to incorrect way of tracking rewards by LP's an attacker can receive an unfair share of yield accumulated in MainVault.sol

[sherlock-admin]

Delvir0

high

Due to incorrect way of tracking rewards by LP's an attacker can receive an unfair share of yield accumulated in MainVault.sol

Summary

The docs mentions "Note that the user will be generating yield with our vault LP tokens from the moment of depositing funds."

While the vault tracks how many LP's an user has, it does not take into account when a user deposits and received these LP's. A user that deposits into the vault just after a rebalance and a user that deposits into the vault just before the next rebalance (after approx. 2 weeks) are the same.

Since the rebalance period is known and is timed an attacker could deposit just before a rebalance and withdraw just after the rebalance is done, taking up the yield while not actually contributing to the protocol. The attacker could deposit a high amount which could lead to accumulating a big portion of the yield.

Vulnerability Detail

Step 1: Just before a rebalance, an attacker uses the MainVault.deposit() function to receive LP's e.g. deposits 100 USDC at an exchangeRate of 2$/LP = 50LP's.

Step 2 (optional): Also before rebalance, to assure the attacker can withdraw, calls the MainVault.withdrawalRequest() to ensure a withdrawal after rebalance.

Step 3: The rebalance happens and the new exchangeRate is set (2.1$/LP). The attacker withdraws 50LP's and receives 2.1*50=105 The 5$ is actually yield that has been accumulated by users that have had their assets deposited in the protocols.

Impact

Attacker can receive an unfair share amount of yield

Code Snippet

https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/MainVault.sol#L106-L124

Tool used

Manual Review

Recommendation

Track the timestamp of the user when depositing (like Uniswap/ Masterchef).

Duplicate of #369

[Delvir0]

Escalate for 10 USDC Disagree with duplicate. #88 mentions manipulation of exchange rate, this issue is not about that. This submission outlines two issues

1. which is most important, a user can simply deposit before rebalance and withdraw after. This creates a window of accumulating yield but not contributing. exchangeRate does not take this into account and there's no way the users "activity" is tracked. This is why Masterchef tracks timestamp and Yearn has a time window where yield is distributed. Yearn docs:

Also see following, this basically outlines the same issue: https://github.com/code-423n4/2022-05-sturdy-findings/issues/61

2. exchangeRate can be calculated prior to being recalculated, making users being able to take advantage of that. This can be used to support the decision whether to perform the above or not. This isn't a big factor since the vault pairs stables though but can be at depegs.

So where I mention "The 5$ is actually yield that has been accumulated by users that have had their assets deposited in the protocols.", I mean to say that the yield actually belongs to the users since they provided value to the protocol where the attacker didn't.

[sherlock-admin]

Escalate for 10 USDC Disagree with duplicate. #88 mentions manipulation of exchange rate, this issue is not about that. This submission outlines two issues

1. which is most important, a user can simply deposit before rebalance and withdraw after. This creates a window of accumulating yield but not contributing. exchangeRate does not take this into account and there's no way the users "activity" is tracked. This is why Masterchef tracks timestamp and Yearn has a time window where yield is distributed. Yearn docs:

Also see following, this basically outlines the same issue: https://github.com/code-423n4/2022-05-sturdy-findings/issues/61

2. exchangeRate can be calculated prior to being recalculated, making users being able to take advantage of that. This can be used to support the decision whether to perform the above or not. This isn't a big factor since the vault pairs stables though but can be at depegs.

So where I mention "The 5$ is actually yield that has been accumulated by users that have had their assets deposited in the protocols.", I mean to say that the yield actually belongs to the users since they provided value to the protocol where the attacker didn't.

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.

[hrishibhat]

Escalation accepted

Valid duplicate of #369

[sherlock-admin]

Escalation accepted

Valid duplicate of #369

This issue's escalations have been accepted!

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