BugBusters - User can be liquidated while fund_account is paused

[sherlock-admin]

BugBusters

high

User can be liquidated while fund_account is paused

Summary

User can be unfairly liquidated while adding margin is paused.

Vulnerability Detail

The function fund_account or fund_account_eth can be paused while the function liquidate can never be paused which means user can be unfailry liquidated when adding margin to position is paused since account cannot be funded, which could have saved the user position from liquidating, but in pause scenario user will be liquidated.

def fund_account(_token: address, _amount: uint256):
    """
    @notice
        Allows a user to fund his _token margin.
    """
    assert self.is_accepting_new_orders, "funding paused"
    assert self.is_whitelisted_token[_token], "token not whitelisted"
    self.margin[msg.sender][_token] += _amount
    self._safe_transfer_from(_token, msg.sender, self, _amount)
    log AccountFunded(msg.sender, _amount, _token)

@external
def add_margin(_position_uid: bytes32, _amount: uint256):
    """
    @notice
        Allows to add additional margin to a Position and 
        reduce the leverage.
    """
    assert self.is_whitelisted_dex[msg.sender], "unauthorized"

    position: Position = self.positions[_position_uid]

    assert (self.margin[position.account][position.debt_token] >= _amount), "not enough margin"

    self.margin[position.account][position.debt_token] -= _amount
    position.margin_amount += _amount

    self.positions[_position_uid] = position
    log MarginAdded(_position_uid, _amount)

def liquidate(_position_uid: bytes32):
    """
    @notice
        Liquidates a position that exceeds the maximum allowed
        leverage for that market.
        Charges the account a liquidation penalty.
    """
    assert self.is_whitelisted_dex[msg.sender], "unauthorized"
    assert self._is_liquidatable(_position_uid), "position not liquidateable"

    position: Position = self.positions[_position_uid]
    debt_amount: uint256 = self._debt(_position_uid)

    min_amount_out: uint256 = self._market_order_min_amount_out(
        position.position_token, position.debt_token, position.position_amount
    )

    amount_out_received: uint256 = self._close_position(_position_uid, min_amount_out)

    # penalize account
    penalty: uint256 = debt_amount * self.liquidation_penalty / PERCENTAGE_BASE

    if amount_out_received > debt_amount:
        # margin left
        remaining_margin: uint256 = amount_out_received - debt_amount
        penalty = min(penalty, remaining_margin)
        self.margin[position.account][position.debt_token] -= penalty
        self._distribute_trading_fee(position.debt_token, penalty)

    log PositionLiquidated(position.account, _position_uid, position)

So for above code snippets following scenerio can play out:

1. User opens a position with the intended leverage.
2. User is about to liquidate and want to fund account, add margin and save position.
3. Protocol is paused.
4. User can't fund and add margin and is liquidated unfairly.

Impact

User is unfailry liqudiated.

Code Snippet

https://github.com/sherlock-audit/2023-06-unstoppable/blob/94a68e49971bc6942c75da76720f7170d46c0150/unstoppable-dex-audit/contracts/margin-dex/Vault.vy#L659-L668
https://github.com/sherlock-audit/2023-06-unstoppable/blob/94a68e49971bc6942c75da76720f7170d46c0150/unstoppable-dex-audit/contracts/margin-dex/Vault.vy#L346-L375

For more reference read the following:

https://dacian.me/defi-slippage-attacks

Tool used

Manual Review

Recommendation

Pause the liquidation mechanism too when funding is paused.

[141345]

expected behavior

[Nabeel-javaid]

Escalate for 10 USDC

This is a valid medium. Such issues have been accepted as valid before. While the state of contract is paused, liquidation is not paused but adding margin is paused, so user cannot save the position even if they want to. Closing position is not the ideal case, as it results in the loss of oppertunity for the end user.

And lastly all big cex and dex in their perpetual trading have the feature to add more margin anytime to save the position from going underwater.

Other example of similar issue being valid accepted is from the blueberry contest:

sherlock-audit/2023-02-blueberry-judging#290

[sherlock-admin2]

Escalate for 10 USDC

This is a valid medium. Such issues have been accepted as valid before. While the state of contract is paused, liquidation is not paused but adding margin is paused, so user cannot save the position even if they want to. Closing position is not the ideal case, as it results in the loss of oppertunity for the end user.

And lastly all big cex and dex in their perpetual trading have the feature to add more margin anytime to save the position from going underwater.

Other example of similar issue being valid accepted is from the blueberry contest:

sherlock-audit/2023-02-blueberry-judging#290

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.

[141345]

recommendation:
keep the original judging.

Escalate for 10 USDC

This is a valid medium. Such issues have been accepted as valid before. While the state of contract is paused, liquidation is not paused but adding margin is paused, so user cannot save the position even if they want to. Closing position is not the ideal case, as it results in the loss of oppertunity for the end user.

And lastly all big cex and dex in their perpetual trading have the feature to add more margin anytime to save the position from going underwater.

Other example of similar issue being valid accepted is from the blueberry contest:

sherlock-audit/2023-02-blueberry-judging#290

User can repay to close position, no loss.

The reason this is expected behavior is, when the system is in defensive mode, most activities should be paused, especially adding new or expanding the current position, adding additional fund, etc. In another word, the system should be in "reduce-only" mode. Users are encouraged to exit. So the repay and close is still funcitoning. Users should take their own responsibility for the potential change on their positions, including liquidation in such special situation.

But in this kind of mode, liquidation is not likely to be paused, otherwise the exchange or the counter party will take too much risk.

This kind of mode is the protocol's design choice. Disallow/Allow to add margin are both ok. Not bug.

Refer to GME news in 2021, robinhood announced a "reduce-only" mode.
https://www.fxstreet.com/news/gamestop-gme-tumbles-over-25-as-robinhood-puts-shares-on-reduce-only-mode-202101281424

[Nabeel-javaid]

Robin hood example don't apply here. In Unstoppable when the adding margin is paused, it is paused across all the assets in the system. Not a single asset can be moved into reduce-only state. So the impact of such behavior is is different and greater than the robin hood example. Case mentioned in original escalation still stays.

[141345]

Robin hood example don't apply here. In Unstoppable when the adding margin is paused, it is paused across all the assets in the system. Not a single asset can be moved into reduce-only state. So the impact of such behavior is is different and greater than the robin hood example. Case mentioned in original escalation still stays.

My point of the robinhood example is, there exists precedent of similar protection mode. Here Unstoppable team's setup might be too defensive for many people. But pause mode is common in DeFi, it's just a matter of the degree. Some protocol might pause everything, even more strict. So I think this is design choice, not bug. The suggestion might help the team to deliever a smoother user experience though.

[hrishibhat]

Result:
Low
Unique
Agree with the Lead Judge, user can close position if needed and additional comment from sponsor on the functioning.

if system went into defensive mode something went wrong that shouldn't have happened. Until this is reviewed we don't want users to put more funds into the protocol. add_margin is still available with any funds that are already in the system, but otherwise system should be in winddown mode reducing potential exposure.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• Nabeel-javaid: rejected