BugBusters - User will be forced liquidated

[sherlock-admin]

BugBusters

high

User will be forced liquidated

Summary

The addMargin function have a vulnerability that can potentially lead to issues when the contract is paused. Users are unable to add margin during the paused state, which can leave them vulnerable to liquidation if their collateral value falls below the required threshold. Additionally, after the contract is unpaused, users can be subject to frontrunning, where their margin addition transactions can be exploited by other users.

Vulnerability Detail

To better understand how this vulnerabilities could be exploited, let's consider a scenario:

1): The contract owner pauses the contract due to some unforeseen circumstances or for maintenance purposes.

2): During the paused state, User A wants to add margin to their account. However, they are unable to do so since the contract prohibits margin addition while paused.

3): Meanwhile, the price of the collateral supporting User A's account experiences significant fluctuations, causing the value of their collateral to fall below the required threshold for maintenance.

4): While the contract is still paused, User A's account becomes eligible for liquidation.

5): After some time, the contract owner decides to unpause the contract, allowing normal operations to resume.

6): User A tries to add margin to their account after the contract is unpaused. However, before their transaction is processed, User B, who has been monitoring the pending transactions, notices User A's margin addition transaction and quickly frontruns it by submitting a higher gas price transaction to liquidate User A's account instead.

Now userA will be forcefully liquidated even tho he wants to add the margin.

You can read more from this link

Impact

The identified impact could be

1): Unfair Liquidations: Users can be unfairly liquidated if their margin addition transactions are frontrun by other users after the contract is unpaused. This can result in the loss of their collateral.

Code Snippet

https://github.com/sherlock-audit/2023-04-hubble-exchange/blob/main/hubble-protocol/contracts/MarginAccount.sol#L136-L156

Tool used

Manual Review

Recommendation

Implement a Fair Liquidation Mechanism: Introduce a delay or waiting period before executing liquidation transactions. This waiting period should provide sufficient time for users to address their collateral issues or add margin.

[Nabeel-javaid]

escalate for 10 USDC

I think this issue should be considered as valid. The validity of issue should be accepted. The report clearly shows how a protocol owner actions (pause) will result in unfair liquidations causing loss of funds to users.

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

For reference, similar issues were consider valid in the recent contests on sherlock, you can check it here and here. Maintaining a consistent valid/invalid classification standard will be ideal here.

[sherlock-admin2]

escalate for 10 USDC

I think this issue should be considered as valid. The validity of issue should be accepted. The report clearly shows how a protocol owner actions (pause) will result in unfair liquidations causing loss of funds to users.

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

For reference, similar issues were consider valid in the recent contests on sherlock, you can check it here and here. 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]

I mean if the liquidation cannot be paused then clearly a medium (there are similar past finding)

https://github.com/sherlock-audit/2023-04-hubble-exchange/blob/1f9a5ed0ca8f6004bbb7b099ecbb8ae796557849/hubble-protocol/contracts/MarginAccount.sol#L322

    function liquidateExactRepay(address trader, uint repay, uint idx, uint minSeizeAmount) external whenNotPaused {

But The liquidation is paused as well

so recommend severity is low

a optional fix is adding a grace period for user to add margin when unpause the liquidation and addMargin

[Nabeel-javaid]

Hey @ctf-sec
Adding more context, the problem is not that the liquidation functions can be paused or not, it is more of a front-running problem.

Liquidation functions cannot be paused individually, whole contract is paused which means that other functions with the whenNotPaused modifier cannot be accessed either.

So when the contract is un-paused, a user can be front-run before making the position healthy and is unfairly liquidated. For more reference see the following issue that are exactly the same .

Perrenial Contest Issue 190
Notional Contest Issue 203

I hope that added context will further help in judging.

[hrishibhat]

@ctf-sec
I understand this is how it is design but there is a loss due front-run during unpause.

[ctf-sec]

sherlock-audit/2023-04-blueberry-judging#118

seems like this issue has been historically judged as a medium

then a medium is fine

[hrishibhat]

Result:
Medium
Unique
After further discussions with Lead Watson and Sponsor, considering this a valid medium based on the above comments

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• Nabeel-javaid: accepted

[0xshinobii]

This issue has more chances of occurrence when there are volatile assets as collateral. As we are launching with single collateral (USDC) initially, we'll fix this later.