sherlock-audit / 2023-04-hubble-exchange-judging

7 stars 6 forks source link

0x3e84fa45 - User with high PnL can avoid liquidation fee #180

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

0x3e84fa45

medium

User with high PnL can avoid liquidation fee

Summary

Hubble enforces margin restriction to ensure sufficient collateral is available to cover potential losses. It incentivizes responsible trading by charging a liquidation fee on every liquidated account. A user can withdraw all of their capital to avoid paying any liquidation fee.

Vulnerability Detail

The protocol accounts all unrealized profit and loss toward the users margin in getNotionalPositionAndMarginVanilla. With a large and positive PnL the user can remove all of margin from the protocol.

Impact

With a sudden price drops, as common in the volatile cryptocurrency market, the PnL goes down and the users is getting liquidated. The liquidated user cannot pay the liquidation fee as they no longer have collateral left. The loss has to be covered by the insurance instead.

Code Snippet

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

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

Tool used

Manual Review

Recommendation

The user must have enough collateral in their balance to cover any liquidation fee. The margin model should be changed to not account a positive pnL towards the user balance.

function getNotionalPositionAndMarginVanilla(address trader, bool includeFundingPayments, Mode mode)
        public
        view
        returns(uint256 notionalPosition, int256 margin)
    {
        int256 unrealizedPnl;
        margin = marginAccount.getNormalizedMargin(trader);
        if (includeFundingPayments) {
            margin -= getTotalFunding(trader); // -ve fundingPayment means trader should receive funds
        }
        (notionalPosition, unrealizedPnl) = getTotalNotionalPositionAndUnrealizedPnl(trader, margin, mode);
-       margin += unrealizedPnl;
+       margin += min(unrealizedPnl, 0);
     }
markus0x1 commented 1 year ago

Escalate

The user does not have to pay the liquidation fee, instead the fee is paid by the insurance. The exploit prevents important risk parameters ("liquidationFee") from getting applied, so it should be a medium issue based on historical decision making.

sherlock-admin2 commented 1 year ago

Escalate

The user does not have to pay the liquidation fee, instead the fee is paid by the insurance. The exploit prevents important risk parameters ("liquidationFee") from getting applied, so it should be a medium issue based on historical decision making.

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.

0xshinobii commented 1 year ago

Unrealized PNL is considered as part of the margin to reflect the actual leverage of the trader. But when removing margin, traders cannot withdraw if the leverage increases more than 5x after removing the margin. Since traders are liquidated at 10x leverage, there is enough time to execute liquidation before bad debt is created. PS: the scenario described above can happen even if unrealized pnl is not part of margin. Say there is huge loss in a position and liquidation doesn't happen on time, they'll not have funds to pay for liquidation fee and bad debt will be created.

markus0x1 commented 1 year ago

It is true that late liquidation cannot be entirely prevented, but from the trader's perspective the two scenarios are very different

A) Positive Margin:

The trader holds a margin of 100 USDC.

B) Positive PnL

Their Profit and Loss (PnL) is 100.

When liquidated, the remaining balance is as follows:

For positive margin:

100 USDC - liquidationFee + pnl which can be either greater than 0 or less than 0.

For positive PnL:

0 - liquidationFee + pnl, which is less than 0.

In scenario A, the trader's balance can be positive or negative. If it turns out to be positive, the trader will have some capital left, but they will need to deduct the liquidation fee. Avoiding liquidation is preferred in this case due to the presence of the liquidation fee.

In scenario B, regardless of the initial PnL, the trader will have a negative balance after the position is liquidated. He does not care about the liquidation fee, since he will not pay it either way.

Traders can abuse the asymmetric payout and should remove all their collateral once they have high enough PnL.

IAm0x52 commented 1 year ago

The user is liquidated in either scenario when their margin drops under 10%. In the scenario where there position is backed completely by their PNL they would be liquidated before their PNL got to 0 which means that the PNL would cover their liquidation fee. This is because their VUSD balance would be negative after the liquidation but then their PNL would be realized here which would convert their PNL to VUSD and restore their balance.

Only way for it to stay negative would be to have a late liquidation which a known issue.

hrishibhat commented 1 year ago

@MarkuSchick

markus0x1 commented 1 year ago

The user is liquidated in either scenario when their margin drops under 10%. In the scenario where there position is backed completely by their PNL they would be liquidated before their PNL got to 0 which means that the PNL would cover their liquidation fee. This is because their VUSD balance would be negative after the liquidation but then their PNL would be realized here which would convert their PNL to VUSD and restore their balance.

Only way for it to stay negative would be to have a late liquidation which a known issue.

That is a fair point. In practice, you would face slippage in realizing your profits, so 1 USD pnl is not interchangeable with 1 USD margin, but that is another issue.

hrishibhat commented 1 year ago

Result: Low Unique Considering this a low based on the above comments.

sherlock-admin2 commented 1 year ago

Escalations have been resolved successfully!

Escalation status: