ge6a - restoreVault() lacks grace period

[sherlock-admin2]

ge6a

medium

restoreVault() lacks grace period

Summary

Vulnerability Detail

Notional has a functionality that allows the withdrawal of assets from the pools and the locking of the vault in case of an emergency. While the vault is locked, users cannot open or close positions (_depositFromNotional, _redeemFromNotional are locked and cannot be executed). A user's position can become eligible for liquidation while the vault is locked. The user will not be able to top up the position, and they also won't be able to exit it before it becomes eligible for liquidation. When the vault is unlocked, the user can be front ran, for example, and the position can be liquidated before they can react. This will lead to losses for the user and contradicts the documentation:

If you get too close to liquidation, you can exit the vault or top up your position to reduce your liquidation risk at any time

Impact

Loss of funds for a user due to inability to top up or exit a position.

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/SingleSidedLPVaultBase.sol#L501-L525

Tool used

Manual Review

Recommendation

Add a grace period after unlocking the vault during which users can top up their positions before liquidators have the right to liquidate them.

[nevillehuang]

Invalid according to sherlock rules here and part of the known responsibilities of actor here

3. An admin action can break certain assumptions about the functioning of the code. Example: Pausing a collateral causes some users to be unfairly liquidated or any other action causing loss of funds. This is not considered a valid issue.

This account may potentially cause a denial of service to account holders but should not be able to move funds off the contract outright.

[gstoyanovbg]

@nevillehuang I am familiar with this Sherlock rule. I submitted this issue because of the Notional's documentation.

If you get too close to liquidation, you can exit the vault or top up your position to reduce your liquidation risk at any time

It promises that a user will be able to exit/top up their position at ANY time, which is not true in this case. The key word here is ANY. I believe that we can consider Notional's documentation as part of the README because there is a link to it, right? And according to the 'Hierarchy of truth' README > Sherlock rules, isn't it?

Hierarchy of truth: Contest README > Sherlock rules for valid issues > Historical decisions.

This account may potentially cause a denial of service to account holders but should not be able to move funds off the contract outright

This statement from the README is correct and does not conflict with documentation. The problem is that even after the vault is restored to normal state users won't have the time to top up their accounts.

P.S I would like to note that there is a simple solution for this issue that Notional could implement to protect their users, it's not a case where nothing can be done.

[gstoyanovbg]

Escalate

@nevillehuang I am familiar with this Sherlock rule. I submitted this issue because of the Notional's documentation.

If you get too close to liquidation, you can exit the vault or top up your position to reduce your liquidation risk at any time

It promises that a user will be able to exit/top up their position at ANY time, which is not true in this case. The key word here is ANY. I believe that we can consider Notional's documentation as part of the README because there is a link to it, right? And according to the 'Hierarchy of truth' README > Sherlock rules, isn't it?

Hierarchy of truth: Contest README > Sherlock rules for valid issues > Historical decisions.

This account may potentially cause a denial of service to account holders but should not be able to move funds off the contract outright

This statement from the README is correct and does not conflict with documentation. The problem is that even after the vault is restored to normal state users won't have the time to top up their accounts.

P.S I would like to note that there is a simple solution for this issue that Notional could implement to protect their users, it's not a case where nothing can be done.

I didn't receive further response on the judging criteria questions in my previous comment this is why i escalate the report to be reviewed again. I expect a clarification on why the incorrect information in the Notional's documentation is not taken into consideration and what is the correct source of truth order in this case. If the judge's decision turns out to be wrong the validity of the report should be changed. I tag the sponsor @jeffywu in case he wants to follow this thread.

[sherlock-admin2]

Escalate

@nevillehuang I am familiar with this Sherlock rule. I submitted this issue because of the Notional's documentation.

If you get too close to liquidation, you can exit the vault or top up your position to reduce your liquidation risk at any time

It promises that a user will be able to exit/top up their position at ANY time, which is not true in this case. The key word here is ANY. I believe that we can consider Notional's documentation as part of the README because there is a link to it, right? And according to the 'Hierarchy of truth' README > Sherlock rules, isn't it?

Hierarchy of truth: Contest README > Sherlock rules for valid issues > Historical decisions.

This account may potentially cause a denial of service to account holders but should not be able to move funds off the contract outright

This statement from the README is correct and does not conflict with documentation. The problem is that even after the vault is restored to normal state users won't have the time to top up their accounts.

P.S I would like to note that there is a simple solution for this issue that Notional could implement to protect their users, it's not a case where nothing can be done.

I didn't receive further response on the judging criteria questions in my previous comment this is why i escalate the report to be reviewed again. I expect a clarification on why the incorrect information in the Notional's documentation is not taken into consideration and what is the correct source of truth order in this case. If the judge's decision turns out to be wrong the validity of the report should be changed. I tag the sponsor @jeffywu in case he wants to follow this thread.

You've created a valid escalation!

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.

[nevillehuang]

@gstoyanovbg

It is also mentioned in the contest READ.ME that an admin calling emergencyExit() can DOS users from actions. But happy to hear sponsors comment @jeffywu

2. Emergency Exit: this account can execute an emergency exit on the SingleSidedLP vault. An emergency exit is RESTRICTED to withdrawing funds proportionally from the LP pool and locking the vault. This account should not be able to do anything other than call this function. This account may potentially cause a denial of service to account holders but should not be able to move funds off the contract outright.

[gstoyanovbg]

@nevillehuang

@gstoyanovbg

It is also mentioned in the contest READ.ME that an admin calling emergencyExit() can DOS users from actions. But happy to hear sponsors comment @jeffywu

2. Emergency Exit: this account can execute an emergency exit on the SingleSidedLP vault. An emergency exit is RESTRICTED to withdrawing funds proportionally from the LP pool and locking the vault. This account should not be able to do anything other than call this function. This account may potentially cause a denial of service to account holders but should not be able to move funds off the contract outright.

@nevillehuang This is true, and I'm not disputing it. The user may not be able to use some of the functions while the vault is in an emergency state (paused). But after the vault is restored to a normal state (after restoreVault), shouldn't the user have time to top up their positions, as promised in the documentation?

[jeffywu]

The entire vault is locked during emergency exit, the user cannot top up their position. I think the point is moot.

[gstoyanovbg]

The entire vault is locked during emergency exit, the user cannot top up their position. I think the point is moot.

@jeffywu I don't dispute this. I fully understand that when you say in the documentation that a user will be able to top up their position at any time, it is meant at any time except during the time when the vault is locked. However, after it is unlocked with restoreVault, the user still won't be able to top up their position (if it is already eligible for liquidation) because a liquidator may frontrun them and liquidate them before the top-up transaction is executed. In my opinion, this contradicts what is promised in the documentation. The solution is either to explicitly mention this or, in my opinion, a better option would be to add a grace period after restoreVault so that for a certain period of time, positions cannot be liquidated, allowing users who wish to top up their positions. This way, there will be no contradiction with the documentation, and users will not suffer financial losses due to liquidations. Ultimately, users are not at fault for the events that necessitated an emergency exit.

[Czar102]

From the docs, for trusted admins:

An admin action can break certain assumptions about the functioning of the code.

Also, this is a known issue per README (additional protocol roles section):

This account may potentially cause a denial of service to account holders but should not be able to move funds off the contract outright.

It is clear that the denial of service may cause a liquidation because topping up will be forbidden. It seems that the protocol team expressed their lack of interest in fixing this. It is a design choice that the protocol liquidates the underwater positions ASAP instead of waiting for a top-up in this scenario.

Hence, I am planning to reject the escalation and leave the issue as is.

[gstoyanovbg]

@Czar102 I presented my arguments in the previous comments and have nothing new to add. This is your decision, and I will respect it regardless of my opinion.

@jeffywu Regarding Notional, I sincerely hope that the apparent contradiction between documentation and reality is not intentional misleading of users. This is a trivial issue that can not remain hidden. As a user who actively uses similar protocols, I would never risk my assets if there is a chance that my positions could be liquidated in such a way. I believe it's not just me. My advice to you is to be honest with your users and explicitly describe in the project documentation that such a scenario exists. This way, each user can assess for themselves whether this risk is acceptable to them or not.

[Czar102]

I believe that we can consider Notional's documentation as part of the README because there is a link to it, right?

Any information directly within README takes precedence over linked resources.

If you would like me to elaborate on any other points you made, please let me know. We can talk about them here or on DMs. Please believe me that all of them are taken into account when the final judgement is made.

It may be useful to mention this property in the docs, apart from the README, so that users would be aware of the property. @jeffywu

[Czar102]

Result: Invalid Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• gstoyanovbg: rejected