roguereddwarf - VaultV2: epochHasNotStarted and epochHasStarted modifiers are not well-defined which can lead to loss of user funds

[sherlock-admin]

roguereddwarf

high

VaultV2: epochHasNotStarted and epochHasStarted modifiers are not well-defined which can lead to loss of user funds

Summary

The VaultV2 contract defines the epochHasNotStarted and epochHasStarted modifiers.

They ensure that certain functions can only be called when the epoch has not started or has started.

The issue is that when block.timestamp==epochBegin both checks pass. I.e. the epoch has started and has not started at the same time.

This can lead to edge cases where in the worst case users lose their funds.

Vulnerability Detail

Let's have a look at the checks that the modifiers perform and let's assume block.timestamp==epochBegin.

The check in epochHasNotStarted passes:

if (block.timestamp > epochConfig[_id].epochBegin) 
         revert EpochAlreadyStarted(); 
     _; 

Also the check in epochHasStarted passes:

if (block.timestamp < epochConfig[_id].epochBegin) 
         revert EpochNotStarted(); 
     _;

So what is the edge case that can occur that leads to a loss of user funds?

The issue comes down to the fact that a deposit can be made after the epoch has been resolved.

The deposit function checks that epochHasNotStarted:
https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/VaultV2.sol#L93-L104

The resolveEpoch function checks that epochHasStarted:
https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/VaultV2.sol#L290-L299

As we have seen both checks pass when block.timestamp==epochBegin.

So what can happen is that a user wants to make a last second deposit but before his transaction is processed and within the same block the controller resolves the epoch and triggers a NULL epoch (assuming there were no funds in one of the Vaults prior to the deposit of the user) with the ControllerPeggedAssetV2.triggerNullEpoch function.

In the ControllerPeggedAssetV2.triggerNullEpoch function, the claimTVL is set which means that the user that performs the deposit in a later transaction of the same block cannot withdraw any funds.

Impact

User funds can be lost.

Code Snippet

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

https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/VaultV2.sol#L440-L444

https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/VaultV2.sol#L93-L104

https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/VaultV2.sol#L290-L299

https://github.com/sherlock-audit/2023-03-Y2K/blob/main/Earthquake/src/v2/Controllers/ControllerPeggedAssetV2.sol#L208-L264

Tool used

Manual Review

Recommendation

The modifiers have to be well-defined.
To make it consistent with Controller logic the epoch should have started when block.timestamp==epochBegin.
This means that the check in epochHasNotStarted should not pass in this case.

Fix:

diff --git a/Earthquake/src/v2/VaultV2.sol b/Earthquake/src/v2/VaultV2.sol
index d481a29..8457f38 100644
--- a/Earthquake/src/v2/VaultV2.sol
+++ b/Earthquake/src/v2/VaultV2.sol
@@ -430,7 +430,7 @@ contract VaultV2 is IVaultV2, SemiFungibleVault, ReentrancyGuard {
     /** @notice You can only call functions that use this modifier before the epoch has started
      */
     modifier epochHasNotStarted(uint256 _id) {
-        if (block.timestamp > epochConfig[_id].epochBegin)
+        if (block.timestamp >= epochConfig[_id].epochBegin)
             revert EpochAlreadyStarted();
         _;
     }

Duplicate of #480

[roguereddwarf]

Escalate for 10 USDC

This issue shows that when a user makes a deposit toward the end of the epoch and the epoch is closed in the same block before the deposit is made, the deposit will still execute successfully.

This then leads to a loss of funds for the user since claimTVL is set to zero.

Since there is a loss of funds (and it's not the fault of the user when the funds are lost. It's because the epoch start / epoch end is not well defined in the modifiers) this issue should be medium.

In fact it is a duplicate of #480

[sherlock-admin]

Escalate for 10 USDC

This issue shows that when a user makes a deposit toward the end of the epoch and the epoch is closed in the same block before the deposit is made, the deposit will still execute successfully.

This then leads to a loss of funds for the user since claimTVL is set to zero.

Since there is a loss of funds (and it's not the fault of the user when the funds are lost. It's because the epoch start / epoch end is not well defined in the modifiers) this issue should be medium.

In fact it is a duplicate of #480

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 #480

[sherlock-admin]

Escalation accepted

Valid duplicate of #480

This issue's escalations have been accepted!

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