sherlock-admin commented 1 year ago

AkshaySrivastav

medium

Liquidators can prevent users from making their positions healthy during an unpause

Summary

The Symmetrical protocol has various paused states in which different operations are paused. The protocol operations can be unpaused by privileged accounts. But when this unpause happens the liquidators can frontrun and liquidate user positions before those users get a chance to make their positions healthy.

https://github.com/sherlock-audit/2023-06-symmetrical/blob/main/symmio-core/contracts/facets/control/ControlFacet.sol#L242-L295

Vulnerability Detail

The privileged addresses can pause the symmetrical protocol fully/partially using the pausability functions in ControlFacet. Once the protocol is paused all these operations cannot be done by users:

deposit
withdraw
allocate
deallocate
sendQuote
lockQuote
openPositions
liquidations
etc

Though, real prices from oracles will surely move up or down during this paused period. If the oracle prices go down, the users won't be allowed to allocate more collateral to their positions or close their positions. Hence their positions will get under-collateralized (based upon real prices).

Once the protocol is unpaused the liquidators can front-run most users and liquidate their positions. Most users will not get a chance to make their position healthy.

This results in loss of funds for the users.

Ref: https://github.com/sherlock-audit/2023-03-notional-judging/issues/203

Impact

By front-running any collateral allocation or position closure of a legitimate user which became under-collateralized during the paused state, the liquidator can unfairly liquidate user positions and collect liquidation profit as soon as the protocol is unpaused. This causes loss of funds to the user.

Also, on unpause, it is unlikely that any human users will be able to secure their positions before MEV/liquidation bots capture the available profit. Hence the loss is certain.

Code Snippet

https://github.com/sherlock-audit/2023-06-symmetrical/blob/main/symmio-core/contracts/facets/control/ControlFacet.sol#L242-L295

Tool used

Manual Review

Recommendation

Consider adding a grace period after unpausing during which liquidation remains blocked to allow users to avoid unfair liquidation by closing their positions or allocating additional collateral. The protocol team can also think of any other way which mitigates the unfair liquidations of users.

MoonKnightDev commented 1 year ago

These pauses are designed for emergency scenarios. When we do initiate a pause, we should formulate specific policies before it's unpaused. As per your example, one strategy could be to temporarily pause liquidations, providing users with an opportunity to distance themselves from a potential liquidation event. This is just one example; any policy can be considered. Therefore, we don't view this as a bug, rather it is an integral part of our system.

akshaysrivastav commented 1 year ago

Escalate for 10 USDC

I think this issue should be considered as valid. As per the sponsor comment above it is evident that the current protocol implementation does not contain any safeguard mechanism against this issue and an additional code change (temporary policies) will be needed to prevent users from the loss of funds due to the issue.

The report clearly shows how a protocol owner action (pause) will result in unfair liquidations causing loss of funds to users.

For reference, similar issues were considered valid in the recent Notional V3 and Blueberry contests and was accepted by Sherlock. Maintaining a consistent valid/invalid classification standard will be ideal here.

sherlock-admin2 commented 1 year ago

Escalate for 10 USDC

I think this issue should be considered as valid. As per the sponsor comment above it is evident that the current protocol implementation does not contain any safeguard mechanism against this issue and an additional code change (temporary policies) will be needed to prevent users from the loss of funds due to the issue.

The report clearly shows how a protocol owner action (pause) will result in unfair liquidations causing loss of funds to users.

For reference, similar issues were considered valid in the recent Notional V3 and Blueberry contests and was accepted by Sherlock. Maintaining a consistent valid/invalid classification standard will be ideal here.

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.

ctf-sec commented 1 year ago

These pauses are designed for emergency scenarios. When we do initiate a pause, we should formulate specific policies before it's unpaused. As per your example, one strategy could be to temporarily pause liquidations, providing users with an opportunity to distance themselves from a potential liquidation event. This is just one example; any policy can be considered. Therefore, we don't view this as a bug, rather it is an integral part of our system.

I have to side with sponsor on this one

there is no way to prevent frontrunning pause or unpause even adding grace period..

In fact, there are a lot of similar finding in recent contest about unpause / pause,

they just end up "won't fix"

https://github.com/sherlock-audit/2023-03-notional-judging/issues/203

The blueberry issue talks about repay is paused when liquidation is active, which is not the same as frontrunning pause or unpause

akshaysrivastav commented 1 year ago

Hey @ctf-sec I totally respect your decision but just want to state my points in a much clearer way as there seems to be a misunderstanding.

The issue is not about frontrunning the admin's pause/unpause txn,
The issue is about, when admin pauses the operations the users are prevented from changing their positions, these positions can become unheathy during paused state, and as soon as admin unpauses the protocol, mev actors can liquidate the users before those users get a chance to save their positions. In short, a protocol owner action (pause) will result in unfair liquidations for users causing loss of funds.
As the sponsors already confirmed that they will need to introduce temporary policies to mitigate this issue. This confirms two things:
- The bug is real, loss of funds is real, and new code changes will be needed for mitigation.
- Since sponsors will be forced to implement new policies, this issue will not technically end up in the "won't fix" category.
Agree on blueberry, actual similar issues to this one are NotionalV3:203 and Perennial:190, both were considered as valid by Sherlock.

ctf-sec commented 1 year ago

Hi Akshay,

I think you are trying to refer to this issue?

https://github.com/sherlock-audit/2023-04-blueberry-judging/issues/117

looks like exact the same issue,

I think a valid medium is ok

hrishibhat commented 1 year ago

Result: Medium Unique Considering this a valid medium based on historical decisions and the issue is still valid from smart contract POV.

Also, however, given that this clearly can be design decision for some protocols, agree that there should be a better rule around these protocol-pausing situations.

sherlock-admin2 commented 1 year ago

Escalations have been resolved successfully!

Escalation status:

akshaysrivastav: accepted