shealtielanz - Unlimited slippage during emergencyExit()

[sherlock-admin]

shealtielanz

medium

Unlimited slippage during emergencyExit()

Summary

In the emergencyExit() of singleSidedLPvault, the min amount out is set to complete zero, based on the assumption of proportional trades do not allow frontrunning however not all trades in pools will be proportional allowing for an unlimited slippage during emergency exits.

Impact

This will lead to emergency exits returning up to zero amount out when unstaking and exiting a pool, leading to complete loss of funds during emergency exits

Code Snippet

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

Tool used

Manual Review

Recommendation

Allow for a min amount when ever calling emergency exit

[nevillehuang]

Invalid, reason to use zero min amounts (no slippage) is clearly stated in the above comments here, that is no trade will occur during a proportional exit in the first place.

[shealtielanz]

Escalate

Hello @nevillehuang
This is a valid duplicate of #87 due to time, I found this late and wrote it haphazardly at the dying minute, I didn’t have time to explain, with tests I found that emergencyExit() trades where not proportional as it allowed for slippage.

• Point 1:
  You can see in my report

however not all trades in pools will be proportional allowing for an unlimited slippage

Here is the point of #87 where the Watson stated that

Single-sided instead of proportional exit is performed during emergency exit

Though the Watson point is based on setting as true instead of false, will make it not proportional.

we both stated the same issue that The trade performed isn’t proportional

• Point 2:
  The Impact of the 2 Issues ( #87 and #120 my issue) is the same.
  You can agree that in #87 the watsons point was that it allowed for slippage and front running attacks.

The following are some of the impacts of this issue, which lead to loss of assets:
_
Redeeming LP tokens one-sided incurs unnecessary slippage as tokens have to be swapped internally to one specific token within the pool, resulting in fewer assets received.
_
Per the source code comment below, in other words, unless a proportional exit is performed, the emergency exit will be subjected to front-run attack and slippage.

Same with the Impact I stated in my issue that

allowing for an unlimited slippage during emergency exits.
_
Impact
_
This will lead to emergency exits returning up to zero amount out when unstaking and exiting a pool, leading to complete loss of funds during emergency exits

Though his report is better detailed than mine the two issues have stated that:
The trade in emergencyExit() will allow for slippage attacks, where the out minimum is set to zero

• Point 3:
  The mitigation I gave in my issue also prevents the issue as well has his issue with even tho his is more direct.
  My mitigation -> Allow for a min amount when ever calling emergency exit
  This would still protect against the underlying issue at hand, at it protects the trade that would be made against slippage and front running attacks.

His mitigation -> Set the isSingleSided parameter to false when calling the _unstakeAndExitPool function to ensure that the proportional exit is performed.

His mitigation solve the issue from the root of the matter, so that the trade will be a proportional trade that won’t allow for slippage and front running attacks.

With the following points I have give above, I kindly say that this issue(#120) is a correct duplicate of #87.

Thank you so much for your time, I really appreciate it

[sherlock-admin2]

Escalate

Hello @nevillehuang
This is a valid duplicate of #87 due to time, I found this late and wrote it haphazardly at the dying minute, I didn’t have time to explain, with tests I found that emergencyExit() trades where not proportional as it allowed for slippage.

• Point 1:
  You can see in my report

however not all trades in pools will be proportional allowing for an unlimited slippage

Here is the point of #87 where the Watson stated that

Single-sided instead of proportional exit is performed during emergency exit

Though the Watson point is based on setting as true instead of false, will make it not proportional.

we both stated the same issue that The trade performed isn’t proportional

• Point 2:
  The Impact of the 2 Issues ( #87 and #120 my issue) is the same.
  You can agree that in #87 the watsons point was that it allowed for slippage and front running attacks.

The following are some of the impacts of this issue, which lead to loss of assets:
_
Redeeming LP tokens one-sided incurs unnecessary slippage as tokens have to be swapped internally to one specific token within the pool, resulting in fewer assets received.
_
Per the source code comment below, in other words, unless a proportional exit is performed, the emergency exit will be subjected to front-run attack and slippage.

Same with the Impact I stated in my issue that

allowing for an unlimited slippage during emergency exits.
_
Impact
_
This will lead to emergency exits returning up to zero amount out when unstaking and exiting a pool, leading to complete loss of funds during emergency exits

Though his report is better detailed than mine the two issues have stated that:
The trade in emergencyExit() will allow for slippage attacks, where the out minimum is set to zero

• Point 3:
  The mitigation I gave in my issue also prevents the issue as well has his issue with even tho his is more direct.
  My mitigation -> Allow for a min amount when ever calling emergency exit
  This would still protect against the underlying issue at hand, at it protects the trade that would be made against slippage and front running attacks.

His mitigation -> Set the isSingleSided parameter to false when calling the _unstakeAndExitPool function to ensure that the proportional exit is performed.

His mitigation solve the issue from the root of the matter, so that the trade will be a proportional trade that won’t allow for slippage and front running attacks.

With the following points I have give above, I kindly say that this issue(#120) is a correct duplicate of #87.

Thank you so much for your time, I really appreciate it

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]

Imo, this submission should be low severity based on the following sherlock rule, as the root cause and attack path is not sufficiently described and extremely vague with lack of details.

Then the submissions with valid attack paths and higher vulnerability are considered valid. If the submission is vague or does not identify the attack path with higher severity clearly it will be considered low.

Additionally, as far as I see it, the zero amount for slippage is not the issue here, but instead is due to this mentioned by @xiaoming9090:

However, it was found that the _unstakeAndExitPool function is executed with the isSingleSided parameter set to true.

[shealtielanz]

My report showed the attack path as the way to exploit this issue is front running/sandwich attacks allowing for the exit trade to be prone to massive slippage

the devs assumed that it is proportional though it is single sided by setting to true however the underlying issues is that the min amounts out for the trade are set to default(zero) which allow the trade to settle at a large slippage.
Thats what made the issue Exploitable.

• I showed the root cause that trades are not proportional
• Attack path as the trade can settle with slippages
• Also my mitigation solves the issue.

This issue is clearly a duplicate of you get to understand the underlying issue submitted by the Watson in #87

[nevillehuang]

I am still not convinced, since the original submission is really very vague. But will call in @jeffywu @xiaoming9090 for further review and insights.

[shealtielanz]

The devs assumed the trade would not be prone front running or sandwich attach because the thought the trade performed would be proportional so the set the min amounts out to zero, now since the trades aren’t proportional the allow for slippage as the min amounts out are to zero.

[shealtielanz]

you can see here

File: SingleSidedLPVaultBase.sol
486:         // By setting min amounts to zero, we will accept whatever tokens come from the pool
487:         // in a proportional exit. Front running will not have an effect since no trading will
488:         // occur during a proportional exit.
489:         _unstakeAndExitPool(claimToExit, new uint256[](NUM_TOKENS()), true);

new uint256[](NUM_TOKENS()) is zero by default, that's what prompted the comment

 // By setting min amounts to zero, we will accept whatever tokens come from the pool
487:         // in a proportional exit. Front running will not have an effect since no trading will
488:         // occur during a proportional exit.

where the min amounts out(new uint256[](NUM_TOKENS())) is set to a reasonable amount that wouldn't allow for slippage the attack wouldn't be visible

[jeffywu]

I believe boolean being set to true is a bug and that was already reported in an existing issue. If the concern is with the non-proportionality of the trades, then this issue should be a duplicate of that.

In a realistic scenario, if the pool is moving rapidly during an emergency exit it is practically very difficult to set slippage limits. The idea here is to ensure that the vault is able to rapidly exit whatever funds it can. A sandwich trade would not be profitable if the boolean flag is set to false.

[Czar102]

From the submission:

however not all trades in pools will be proportional

This is not explained at all. Why is that? From the wording it seems some would be and others wouldn't, which is not the case in the code quoted by the submission.
This report is highly speculative. It looks like the Watson did not understand the issue and the report is based on very few observations more than that the zero slippage is set, which can be a valid setting in many integrations.

The mitigation proposed in this report shows the lack of understanding of the issue: it's not the lack of slippage control that is lacking, but it is a wrongly set argument. Even though the same exploit is possible, it shows a lack of understanding of the Watson.

The Watson even failed to justify why are the withdrawals not proportional despite a comment right above the line of code mentioned. Quoting it can invalidate all parts of noted "properties" described in the issue, hence the Watson did not provide sufficient explanation of the bug per rule mentioned by @nevillehuang.

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

[shealtielanz]

The mitigation I proposed might not be the best mitigation, but it is common mitigation against this kind of issues
It is due to time constraint as I said before, i wasn't able to give a very well detailed report, however I strongly believe that this issue is a duplicate of #87 as it states the issues and attack pathway, it is a bug in the protocol,
that's why #87 should be selected for report because it is better written than this but that doesn't make this report invalid in any sense.
The statement:

however not all trades in pools will be proportional

I wrote this report on my phone about 2 mins before the contest ended, it was meaning to saying the trades are not at all proportional due to issues with language, time, my keyboard auto-correct. (but it still means the same thing as the context is still in place).
And it still states the problems that:

• trades are not proportional.(underlying issue)
• frontrunning and sandwich attacks leading to slippage.(attack pathway)
• my mitigation is not the best in this scenario, however that doesn't make it invalid.(mitigation to the attack)

My report complies to the Sherlock guideline

Then the submissions with valid attack paths and higher vulnerability are considered valid. If the submission is vague or does not identify the attack path with higher severity clearly it will be considered low.

The submission(My report) clearly defines the attack path and ponders more on it as you can see here.

• This will lead to emergency exits returning up to zero amount out when unstaking and exiting a pool, leading to complete loss of funds during emergency exits.(slippage issues)

in #87 the attack pathway is also shown:

• Redeeming LP tokens one-sided incurs unnecessary slippage as tokens have to be swapped internally to one specific token within the pool, resulting in fewer assets received.
• Per the source code comment below, in other words, unless a proportional exit is performed, the emergency exit will be subjected to front-run attack and slippage.
• After the emergency exit, the vault only held one of the pool tokens. To re-enter the pool, the vault has to either swap the token to other pool tokens on external DEXs to maintain the proportion or perform a single-sided join. Both of these methods will incur unnecessary slippage, resulting in fewer LP tokens received at the end.

I strongly believe that this issue is a valid duplicate of #87.

[shealtielanz]

Also another thing to note is the sponsors comment here

If the concern is with the non-proportionality of the trades, then this issue should be a duplicate of that.

My report states this as the concern

[nevillehuang]

Agree with @Czar102, and is also along the thoughts of why I invalidated this issue in the first place.

[shealtielanz]

I understand your reasoning for disregarding this in the first place however, with the comments I and the sponsor presented above I don’t see why this escalation should not be accepted.

[Czar102]

Hey @shealtielanz, I don't think you managed to bring up any new points in your response.

It is due to time constraint as I said before, i wasn't able to give a very well detailed report

Everyone has the same amount of time and it's within the rules of the contest. The fact that you didn't manage to describe this issue properly means that you didn't create a valid submission per contest rules.
I will reiterate my past point. I think both I and the Lead Judge agree that:

• the Watson did not fully understand the issue at the time of submission,
• the Watson improperly understood protocol intentions,
• the Watson did not understand in which situations is a single-sided exit performed,
• the report is vague and does not present sufficient proof to validate the finding.

Then the submissions with valid attack paths and higher vulnerability are considered valid. If the submission is vague or does not identify the attack path with higher severity clearly it will be considered low.

You have mentioned the attack path, but the report is extremely vague, which is supported by the above mentioned points. This is why I am planning keep it invalid.

Let this be a lesson for the future to focus on the codebase earlier to submit high-effort submissions to be rewarded for them.

The Lead Judge made a good call by not including it as a duplicate.

[Czar102]

Result:
Invalid
Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• shealtielanz: rejected

[shealtielanz]

Une autre leçon apprise