0xbranded - Positions Can Fail to Close Due to Accounting Failure

[sherlock-admin3]

0xbranded

Medium

Positions Can Fail to Close Due to Accounting Failure

Summary

Due to how funding fee payments are accounted for in the system, closing a position can fail due to insufficient collateral balance. This violates one of the system invariants that any open position can always be closed. It can also interfere with liquidations and cause loss of user funds in the event that the position moves unfavorably in the meantime.

Vulnerability Detail

Due to the asynchronous nature of how funding fees are paid from one side of a pool to the other, it's possible to end up in a situation where there is not a large enough collateral balance to satisfy the return of collateral to a user's positions. In fact, this is even acknowledged and partially accounted for within calc_fees:

  fees            : SumFees       = self.FEES.calc(
                                    pos.pool, pos.long, pos.collateral, pos.opened_at)
...
  avail           : uint256       = pool.base_collateral if pos.long else (
                                    pool.quote_collateral )
...
  funding_received: uint256       = 0 if remaining == 0 else (
                                    min(fees.funding_received, avail) )

This funding received is in turn deducted from the collateral of the other side of the pool:

# long
    base_collateral : [self.MATH.MINUS(fees.funding_received)],
...
# short
    quote_collateral: [self.MATH.MINUS(fees.funding_received)],

However, as acknowledged above due to negative positions the funding receiving can exceed the paid amount, which can result in 0 collateral tokens remaining in the pool for the other side.

At this stage if a position on the other side of the pool tried to close, it would almost always fail:

# long
    quote_collateral: [self.MATH.PLUS(fees.funding_paid),
                       self.MATH.MINUS(pos.collateral)],
...
# short
    base_collateral : [self.MATH.PLUS(fees.funding_paid), 
                       self.MATH.MINUS(pos.collateral)],

since the position's collateral which is subtracted is always greater or equal to the funding fees it paid. Ironically the only positions which can still be withdrawn at this time are those which have underpaid their funding fees, so fees.funding_paid added equals pos.collateral subtracted.

Impact

`core.vy includes a specification for one of the invariants for this protocol:

#  - the protocol handles the accounting needed to maintain the sytem invariants:
#    * any open position can always be closed
#        close(open(collateral)) = collateral + pnl - fees

This invariant is broken as there are some edge cases where positions are unable to close (they are not always able to). Thus the current code is not to spec. Additionally, closing positions is a time-sensitive operation, especially related to liquidations, and there is the possibility for a position to move unfavorably during the period in which it cannot be closed.

This can be alleviated by additional depositors on the other side of the pool bringing in additional collateral. Additionally, it's not likely to occur, however it is acknowledged by the team as a possibility due to the handling of negative positions in funding fee payments.

Code Snippet

Tool used

Manual Review

Recommendation

In order to avoid this scenario the funding payment a user is receiving should never exceed the collateral balance of the pool. This accounting mismatch should be accounted for far in advance of this occurring.

Consider modifying the funding payment / receipt structure, either by changing its accounting or making more synchronous payments with receipts.

These approaches can be implemented in a number of ways. For example, when funding fees are underpaid, track the exact quantity of underpayment and apply a pro rata / market adjustment to future receipts of that token funding payment.

[spacegliderrrr]

Escalate

Issue is not a valid dup of #96. Issue suggests and underflow, due to funding_paid exceeding the position's collateral. However, this cannot possibly happen as funding_paid is calculated via deduct function which cannot possibly exceed the collateral. Issue should be invalidated.

def deduct(x: uint256, y: uint256) -> Val:
  if x >= y: return Val({remaining: x - y, deducted: y})
  else     : return Val({remaining: 0,     deducted: x})

[sherlock-admin3]

Escalate

Issue is not a valid dup of #96. Issue suggests and underflow, due to funding_paid exceeding the position's collateral. However, this cannot possibly happen as funding_paid is calculated via deduct function which cannot possibly exceed the collateral. Issue should be invalidated.

def deduct(x: uint256, y: uint256) -> Val:
  if x >= y: return Val({remaining: x - y, deducted: y})
  else     : return Val({remaining: 0,     deducted: x})

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.

[msheikhattari]

this issue is not referring to funding paid exceeding collateral of individual positions.

due to negative positions the funding receiving can exceed the paid amount

as described in the note above, this issue refers to the scenario where global funding received exceeds global funding paid

• a position has gone negative due to funding fees paid exceeding collateral
• funding paid will be capped at collateral for that position
• however, funding received was calculated higher in the meantime and was not based on the capped value of funding paid

the base/quote collateral was decreased by the higher funding received. But the payer increases it by a lesser amount. So a net decrease in collateral results for the side of the pool getting paid funding.

on that side of the pool, now positions could fail to close due to this delta:

    quote_collateral: [self.MATH.PLUS(fees.funding_paid),
                       self.MATH.MINUS(pos.collateral)],

note that these deltas are used in pool.close which calls eval1 from math.vy, with no such underflow precautions.

so there are scenarios where overpaid funding results in insufficient collateral for a position to close, if the overpayment received - paid exceeds the necessary collateral for withdrawal (collateral - funding_paid)

similar to #96

[spacegliderrrr]

the base/quote collateral was decreased by the higher funding received. But the payer increases it by a lesser amount. So a net decrease in collateral results for the side of the pool getting paid funding.

If there's no new positions, the collateral will be decreased by maximum the current collateral

on that side of the pool, now positions could fail to close due to this delta:

Also incorrect, funding_paid can never exceed pos.collateral, as I've mentioned above - deduct is used to make sure this invariant holds true.

#96 describes a much more sophisticated set of transactions allowing for the state in which underflow occurs. Your statements above are incorrect and would not lead to underflow.

[msheikhattari]

Funding paid cannot exceed collateral of a position. This was not only acknowledged but formed the basis of this issue.

In the event that funding paid for a given position is capped at its collateral value, funding received by the counterparty will not reflect this capped value but can increase unboundedly.

on that side of the pool, now positions could fail to close due to this delta

Also incorrect, funding_paid can never exceed pos.collateral, as I've mentioned above - deduct is used to make sure this invariant holds true.

Yes, this is precisely why closing the position can fail. This net change will always be ≤ 0. This issue specifically occurs when the change is negative, and the remaining collateral in the pool is less than the subtracted value (can occur bc funding_received is uncapped and may not reflected capped value of funding_paid). deduct is not called in this case, and eval1 will underflow.

The issue described is the same. The recommended mitigation reflects this as well. Apologies if it's still unclear, I'm happy to create a PoC.

[rickkk137]

invalid

funding_received cannot be greater than funding_paid

  paid_long_term      : uint256 = self.apply(fs.long_collateral, fs.funding_long * new_terms)
  received_short_term : uint256 = self.divide(paid_long_term,    fs.short_collateral)

let's assume
1-userA opens a long position with 1000 collateral
2-userB opens a short position with 100 collateral
3-funding_paid be greater than 1000 collateral
4-userA just pay 1000 collateral as a funding_paid becuase of max_funding_paid cannot be greater than total_user_collateral
5-userB's funding_receieved
funding_recieved = (funding_paid / short_collateral_total) * short_position_collateral = (1000 / 100) * 100 = 1000

consider add this test to tests/test_positions.py

def test_funding_fee_greater_than_collateral(setup, VEL, STX, pools, open, long, close, short, positions):
    setup()
    open(VEL, STX, True, d(1000), 10, price=d(1), sender=long)#position 1

    open(VEL, STX, False, d(10), 10, price=d(1), sender=short)

    chain.mine(12000)#funding_paid exceed user collateral

    fee = positions.calc_fees(1)
    assert fee.funding_paid == 999_000_000 #its not exactly becuase of protocol fee

    close(VEL, STX, 1, price=d(1), sender=long)
    close(VEL, STX, 2, price=d(1), sender=short)

    pool = pools.lookup(1)

    assert pool.quote_collateral == 0

[msheikhattari]

funding_recieved = (funding_paid / short_collateral_total) * short_position_collateral

This calculation does not reflect what is done in the code.

As you mention, the calculation is:

  paid_long_term      : uint256 = self.apply(fs.long_collateral, fs.funding_long * new_terms)
  received_short_term : uint256 = self.divide(paid_long_term,    fs.short_collateral)

so received_short_term = [(long_collateral * (funding_long * new_terms))/DENOM] * ZEROS / short_collateral

The numerator is not funding_paid, it is long_collateral times funding_long rate. As described in the vulnerability, it's possible for positions to go negative but long_collateral and funding_long have not yet been updated to reflect this.

[rickkk137]

paid_short_term = (long_collateral * (funding_long * new_terms))/DENOM

received_short_term = paid_short_term * ZEROS / short_collateral

funding_received = received_short_term * single_short_position_collateral / ZEROS

and paid_short_term also is greater than received_short_term becuase

received_short_term = paid_short_term / short_collateral

[WangSecurity]

Here's my understanding of the situation where funding paid would exceed the position's collateral:

1. c0 = position's collateral.
2. c1 = c0 deduct funding_paid. In that case, looking at the deduct function, c1.remaining equals 0 (since funding paid > pos.collateral) and c1 deducted = pos.collateral.
3. c2, after the deduct function, c2.remaining and c2.deducted both equal 0, since c1.remaining equals 0.
4. Then, remaining equals 0, cause c2.remaining also equals 0.
5. Then, funding_received also equals 0, cause remaining equals 0.

Hence, funding_received won't exceed the actual funding paid. Am I missing anything?

[msheikhattari]

That's not quite what this issue was describing.

funding_paid for that position will be capped at the position's collateral.
However, funding_received for the other side of the pool uses ps.long_collateral * fs.funding_long * new_terms to determine the global amount of funding that side of the pool receives from the side that was making payments.

The point is that it's possible for funding_received to increase unboundedly while funding_paid was capped. This imbalance will cause positions to fail to close due to more funds being taken from the side that is being paid funding, than the side which was making payments would ultimately put in.

[WangSecurity]

Maybe I'm missing something, but I don't understand where funding received would be larger than funding paid in this case. Could you send the exact link where it would happen, cause I don't see it in the report (or at least doesn't explain it well enough)?

[msheikhattari]

The problem lies in this logic (WLOG consider longs paying shorts):

  paid_long_term      : uint256 = self.apply(fs.long_collateral, fs.funding_long * new_terms)
  received_short_term : uint256 = self.divide(paid_long_term,    fs.short_collateral)

The funding rate paid by longs is multiplied by the total long collateral, and this is divided between all of the short collateral. This is how the funding received by shorts is calculated.

However, negative positions will still be counted in this long_collateral as long as they have not been liquidated yet. So they will still contribute to the global funding fee receipts by shorts.

This portion of funding_received is incorrect, as the total payment made by longs at the time of closing is capped by the position's collateral. But as shown above, since the global long collateral is used for calculating funding_received, the collateral of these positions should not be included in the calculation. Thus there will be some amount of funding_received which was never accounted for in the total funding_paid of all longs. This will cause insolvency of positions and prevent closing some short positions as less funds were paid by longs than was withdrawn by shorts.

While the issue was originally pointing out that the code was not to spec (hence medium), upon reinspection I am requesting consideration for high severity for this issue, since it disrupts a core functionality and potentially causes loss of funds.

[WangSecurity]

@msheikhattari can you make a numerical POC with exact numbers that would lead to this situation happening (I see there's already a numerical POC from another Watson, but it was about this issue being invalid. Hence, I would like to see a POC for a valid case, since I'm not completely convinced this scenario is indeed possible.

[WangSecurity]

If no answer is provided, planning to accept the escalation, de-dup and invalidate this issue.

[msheikhattari]

Just as an example, assume a long has collateral of 1e20, two shorts have collateral of 1e20 each. Now assume the long has gone negative, the funding payment of the long is capped at 1e20 in (calc_fees):

  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

However, when determining the funding received by the shorts, the funding is multiplied by the long collateral which continues to accrue with passage of time:

  paid_long_term      : uint256 = self.apply(fs.long_collateral, fs.funding_long * new_terms)
  received_short_term : uint256 = self.divide(paid_long_term,    fs.short_collateral)

For the sake of simplicity, funding_long = 1, and 2 blocks pass:

paid_long_term = 1e20 + 2, which is divided by the total short_collateral and later distributed (in calc of fees.vy):

def calc(id: uint256, long: bool, collateral: uint256, opened_at: uint256) -> SumFees:
    period: Period  = self.query(id, opened_at)
...
    R_f   : uint256 = self.multiply(collateral, period.received_long) if long else (
                      self.multiply(collateral, period.received_short) )

    return SumFees({funding_paid: P_f, funding_received: R_f, borrowing_paid: P_b})

So whichever short closes first gets 5e19 + 1 of the long collateral. When closing the position this amount will ultimately be subtracted from the collateral of the pool:

  }) if pos.long else  Deltas({
    base_interest   : [],
    quote_interest  : [self.MATH.MINUS(pos.interest)],

    base_transfer   : [],
    base_reserves   : [self.MATH.PLUS(pos.collateral),
                       self.MATH.MINUS(fees.funding_paid)],
    base_collateral : [self.MATH.PLUS(fees.funding_paid), 
                       self.MATH.MINUS(pos.collateral)],

    quote_transfer  : [self.MATH.PLUS(pnl.payout),
                       self.MATH.PLUS(fees.funding_received)],
    quote_reserves  : [self.MATH.MINUS(pnl.payout)],
    quote_collateral: [self.MATH.MINUS(fees.funding_received)], # <-
  })

Leaving 1e20 - (5e19 + 1) collateral in the pool. When the other short tries to cover, it will also try to subtract this same amount which will fail due to underflow.

Note that this was a generic example to go through the flow, in practice there will be many more positions and the numbers will not be so clean, but the principle stands.

[spacegliderrrr]

Leaving 1e20 - (5e19 + 1) collateral in the pool. When the other short tries to cover, it will also try to subtract this same amount which will fail due to underflow.

Incorrect, the withdraw will not fail due to the following lines of code within calc_fees.

  funding_received: uint256       = 0 if remaining == 0 else (
    # 2) funding_received may add up to more than available collateral, and
    #    we will pay funding fees out on a first come first serve basis
                                    min(fees.funding_received, avail) )

[WangSecurity]

I believe the comment above is correct and that is what I was trying to say before. The code works correctly and there would be no issue.

The decision remains, accept the escalation, de-dup and invalidate the issue.

[WangSecurity]

Result:
Invalid
Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• spacegliderrrr: accepted