0xbranded - Loss of Funds From Profitable Positions Running Out of Collateral

[sherlock-admin4]

0xbranded

Medium

Loss of Funds From Profitable Positions Running Out of Collateral

Summary

When profitable positions run out of collateral, they receive no payout, even if they had a positive PnL. This is not only disadvantageous to users, but it critically removes all liquidation incentive. These zero'd out positions will continue to underpay fees until someone liquidates the position for no fee, losing money to gas in the process.

Vulnerability Detail

When calculating the pnl of either a long or short position that is to be closed, if the collateral drops to zero due to fee obligations then they do not receive a payout:

# remaining first calculating in `calc_fees`
  c0              : uint256       = pos.collateral
  c1              : Val           = self.deduct(c0,           fees.funding_paid)
  c2              : Val           = self.deduct(c1.remaining, fees.borrowing_paid)
  # Funding fees prioritized over borrowing fees.
  funding_paid    : uint256       = c1.deducted
  borrowing_paid  : uint256       = c2.deducted
  remaining       : uint256       = c2.remaining

...
# later passed to `calc_pnl`
  final  : uint256       = 0 if remaining == 0 else (
...
# longs
  payout : uint256       = self.MATH.quote_to_base(final, ctx)
...
# shorts
  payout   : uint256 = final

However, it's possible for a position to run out of collateral yet still have a positive PnL. In these cases, no payout is received. This payout is what's sent to the user or liquidator when a position is closed:

# longs
    base_transfer   : [self.MATH.PLUS(pnl.payout),
# shorts
    quote_transfer  : [self.MATH.PLUS(pnl.payout),

In these cases neither the user closing his position, nor the liquidator receives any payment.

Impact

While it may be intended design to penalize users for letting a position run out of collateral, this is a dangerous design choice. It's possible to end up in a situation where a position has run negative due to the funding fees and now has no incentive for liquidation. This will be the case even if it could have been profitable to liquidate this position due to the PnL of the position.

This scenario is dependent on liquidation bots malfunctioning since the liquidatability of a position does not factor in profit (only losses). However, it is acknowledged as a possibility that this scenario may occur throughout the codebase as safeguards are introduced to protect against this scenario elsewhere. In this particular scenario, no particular safeguard exists.

These positions will continue to decay causing further damage to the system until someone is willing to liquidate the position for no payment. It is unlikely that a liquidation bot would be willing to lose money to do so and would likely require admin intervention. By the time admin intervenes, it's most likely that further losses would have already resulted from the decay of the position.

Code Snippet

Tool used

Manual Review

Recommendation

During liquidations, provide the liquidator with the remaining PnL even if the position has run negative due to fees. This will maintain the current design of penalizing negative positions while mitigating the possibility of positions with no liquidation incentive.

Alternatively, include a user's PnL in his payout even if the collateral runs out. This may not be feasible due to particular design choices to ensure the user doesn't let his position run negative.

[Waydou21]

Known Issue in the report

[KupiaSecAdmin]

Escalate

This is low/info at most. In general, the competition between liquidation bots is intense, so most likely the position will be liquidated before it goes running out of collateral. Also it's mentioned that the issue is a known issue in the report.

[sherlock-admin3]

Escalate

This is low/info at most. In general, the competition between liquidation bots is intense, so most likely the position will be liquidated before it goes running out of collateral. Also it's mentioned that the issue is a known issue in the report.

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.

[ami0x226]

This is low severity because the position should be liquidated before collateral becomes zero by liquidation bot and it is known issue

[msheikhattari]

This is low/info at most. In general, the competition between liquidation bots is intense, so most likely the position will be liquidated before it goes running out of collateral. Also it's mentioned that the issue is a known issue in the report.

This is low severity because the position should be liquidated before collateral becomes zero by liquidation bot and it is known issue

As pointed out, these positions will NOT be eligible for liquidation before running out of collateral in the event that the PnL offsets the fee decay. When the position runs out of collateral, the positive PnL is totally wiped out at that point even if it was offsetting the fee decline.

This is describing an edge case that invalidates the assumption made in the report.

[rickkk137]

invalid it is intended design https://github.com/sherlock-audit/2024-08-velar-artha/blob/18ef2d8dc0162aca79bd71710f08a3c18c94a36e/gl-sherlock/contracts/positions.vy#L308

this lets us bound position lifetimes (which lets LPs estimate how long reserves will be locked) and unless fees are very high shouldn't make too much of a difference for users.

[msheikhattari]

The issue describes an edge case that invalidates the assumptions made in that design intention.

The design choice can still be maintained while addressing the described vulnerability: by making positions eligibile for liquidation once fee decay has crossed a certain threshold, regardless of their profitability.

Currently these profitable positions are not eligible for liquidation until the moment that the position completely runs out of collateral, at which point it is treated as if it were worthless. This is independent of any intended design decision; this scenario results in unmitigatable loss which could otherwise easily be addressed without significant design changes.

[WangSecurity]

As report says itself this design is dependent on the liquidation bots malfunctioning, which is an off-chain mechanic, that we should assume to function correctly. Hence, this situation won't happen, if I'm not mistaken.

Planning to accept the escalation and invalidate the report.

[msheikhattari]

@WangSecurity Agreed that liquidation bots malfunctioning is a known issue that should be excluded. However this vulnerability also does encapsulate an edge case that is not dependent on malfunctioning liquidation bots.

Profitable positions which are running out of collateral would not be eligible for liquidation if the profit offsets the fee decay. In this case, these positions would only be liquidatable once the funding/borrow fees exceed the initial collateral, at which point they are marked down as a total loss (despite the overall profitability).

Critically, these profitable positions were NOT eligible for liquidation during this entire period, up until the moment that fee decay causes their value to instantly hit 0. Now the liquidator receives no incentive for liquidating an otherwise profitable position which he was not permitted to liquidate at any time before this. Additionally the user doesn't receive any money back from what should have been a profitable liquidation. And the protocol faces some risk from a dead position that must now be cleaned up for no incentive.

If the profit is not included in the liquidation calculation, to address this aspect of the vulnerability the client should atleast consider adding some way to liquidate positions based on some fee threshold as well (not incorporating pnl).

[rickkk137]

def is_liquidatable(position: PositionState, pnl: PnL) -> bool:
    """
    A position becomes liquidatable when its current value is less than
    a configurable fraction of the initial collateral, scaled by
    leverage.
    """
    # Assume liquidation bots are able to check and liquidate positions
    # every N seconds.
    # We would like to avoid the situation where a position's value goes
    # negative (due to price fluctuations and fee obligations) during
    # this period.
    # Roughly, the most a positions value can change is
    #   leverage * asset price variance + fees
    # If N is small, this expression will be ~the price variance.
    # E.g. if N = 60 and we expect a maximal price movement of 1%/minute
    # we could set LIQUIDATION_THRESHOLD to 1 (* some constant to be on the
    # safe side).
    percent : uint256 = self.PARAMS.LIQUIDATION_THRESHOLD * position.leverage
    required: uint256 = (position.collateral * percent) / 100
    return not (pnl.remaining > required)

admin can increase LIQUIDATION_THRESHOLD to ovoid such situtions

[msheikhattari]

LIQUIDATION_THRESHOLD applies to the current liquidation logic which incorporates the pnl of the position in addition to fees into its present value.

The issue is referring to an additional threshold that also checks only the fees, making a position eligible for liquidation below some threshold of its original collateral (say 5-10%) regardless of pnl. This would avoid the edge case described above.

[rickkk137]

Agreed that liquidation bots malfunctioning is a known issue that should be excluded. However this vulnerability also does encapsulate an edge case that is not dependent on malfunctioning liquidation bots.

Profitable positions which are running out of collateral would not be eligible for liquidation if the profit offsets the fee decay. In this case, these positions would only be liquidatable once the funding/borrow fees exceed the initial collateral, at which point they are marked down as a total loss (despite the overall profitability).

Critically, these profitable positions were NOT eligible for liquidation during this entire period, up until the moment that fee decay causes their value to instantly hit 0. Now the liquidator receives no incentive for liquidating an otherwise profitable position which he was not permitted to liquidate at any time before this. Additionally the user doesn't receive any money back from what should have been a profitable liquidation. And the protocol faces some risk from a dead position that must now be cleaned up for no incentive.

If the profit is not included in the liquidation calculation, to address this aspect of the vulnerability the client should atleast consider adding some way to liquidate positions based on some fee threshold as well (not incorporating pnl).

Profitable positions which are running out of collateral would not be eligible for liquidation if the profit offsets the fee decay. In this case, these positions would only be liquidatable once the funding/borrow fees exceed the initial collateral, at which point they are marked down as a total loss (despite the overall profitability).

def calc_pnl_long(id: uint256, ctx: Ctx, remaining: uint256) -> PnL:
  pos    : PositionState = Positions(self).lookup(id)
  ctx0   : Ctx           = Ctx({price         : pos.entry_price,
                                base_decimals : ctx.base_decimals,
                                quote_decimals: ctx.quote_decimals})
  vtokens: uint256       = pos.interest
  val0   : uint256       = self.MATH.base_to_quote(vtokens, ctx0)
  val1   : uint256       = self.MATH.base_to_quote(vtokens, ctx)
  loss   : uint256       = val0 - val1 if val0 > val1 else 0
  profit : uint256       = val1 - val0 if val1 > val0 else 0
  # Positions whose collateral drops to zero due to fee obligations
  # are liquidated and don't receive a payout.
  final  : uint256       = 0 if remaining == 0 else (
                             0 if loss > remaining else (
                               remaining - loss if loss > 0 else (
                               remaining + profit ) ) )
  # Accounting in quote, payout in base.
  payout : uint256       = self.MATH.quote_to_base(final, ctx)
  assert payout <= pos.interest, ERR_INVARIANTS
  return PnL({
    loss     : loss,
    profit   : profit,
    # Used to determine liquidatability.
    # Could use final instead to account for positive pnl,
    # which would allow positions in profit to be kept open longer
    # but this lets us bound position lifetimes (which lets LPs estimate how
    # long reserves will be locked) and unless fees are very high shouldn't
    # make too much of a difference for users.
 @>>>>   remaining: final - profit if final > profit else final,
    payout   : payout,
  })

profit doesn't have effect on position's status [is_liquidable]

[WangSecurity]

To clarify, in fact, the code doesn't take the profit into consideration, when defining whether the positions is liquidatable or not. Hence, there can't be a scenario where the position is not liquidatable until the collateral reaches 0 with positive PnL. Thus, the position can be liquidated before reaching the 0 collateral?

[msheikhattari]

Yes the watson above is correct. The issue can be closed 👍

[WangSecurity]

Planning to accept the escalation and invalidate the issue.

[WangSecurity]

Result: Invalid Has duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• KupiaSecAdmin: accepted