0xbranded - Funding Paid != Funding Received

[sherlock-admin3]

0xbranded

Medium

Funding Paid != Funding Received

Summary

Due to special requirements around receiving funding fees for a position, the funding fees received can be less than that paid. These funding fee payments are still payed, but a portion of them will not be withdrawn, and become stuck funds. This also violates the contract specification that sum(funding_received) = sum(funding_paid).

Vulnerability Detail

In calc_fees there are two special conditions that impact a position's receipt of funding payments:

  # When there are negative positions (liquidation bot failure):
  avail           : uint256       = pool.base_collateral if pos.long else (
                                    pool.quote_collateral )
  # 1) we penalize negative positions by setting their funding_received to zero
  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) )

If the position has run out of collateral by the time it is being closed, he will receive none of his share of funding payments. Additionally, if the available collateral is not high enough to service the funding fee receipt, he will receive only the greatest amount that is available.

These funding fee payments are still always made (deducted from remaining collateral), whether they are received or not:

  c1              : Val           = self.deduct(c0,           fees.funding_paid)

When a position is closed under most circumstances, the pool will have enough collateral to service the corresponding fee payment:

# longs
base_collateral : [self.MATH.MINUS(fees.funding_received)],
quote_collateral: [self.MATH.PLUS(fees.funding_paid),
                       self.MATH.MINUS(pos.collateral)],
...
# shorts
base_collateral : [self.MATH.PLUS(fees.funding_paid), # <-
                       self.MATH.MINUS(pos.collateral)],
quote_collateral: [self.MATH.MINUS(fees.funding_received)],

When positions are closed, the original collateral (which was placed into the pool upon opening) is removed. However, the amount of funding payments a position made is added to the pool for later receipt. Thus, when positions are still open there is enough position collateral to fulfill the funding fee payment and when they close the funding payment made by that position still remains in the pool.

Only when the amount of funding a position paid exceeded its original collateral, will there will not be enough collateral to service the receipt of funding fees, as alluded to in the comments. However, it's possible for users to pay the full funding fee, but if the borrow fee exceeds the collateral balance remaining thereafter, they will not receive any funding fees. As a result, it's possible for funding fees to be paid which are never received.

Further, even in the case of funding fee underpayment, setting the funding fee received to 0 does not remedy this issue. The funding fees which he underpaid were in a differing token from those which he would receive, so this only furthers the imbalance of fees received to paid.

Impact

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

#    * funding payments match
#        sum(funding_received) = sum(funding_paid)

This invariant is clearly broken as some portion of paid funding fees will not be received under all circumstances, so code is not to spec. This will also lead to some stuck funds, as a portion of the paid funding fees will never be deducted from the collateral. This can in turn lead to dilution of fees for future funding fee recipients, as the payments will be distributed evenly to the entire collateral including these stuck funds which will never be removed.

Code Snippet

Tool used

Manual Review

Recommendation

Consider an alternative method of accounting for funding fees, as there are many cases under the current accounting where fees received/paid can fall out of sync.

For example, include a new state variable that explicitly tracks unpaid funding fee payments and perform some pro rata or market adjustment to future funding fee recipients, specifically for that token.

[spacegliderrrr]

Escalate

Issue should be invalidated. It does not showcase any Medium-severity impact, but rather just a slightly incorrect code comment. Contest readme did not include said invariant as one that must always be true, so simply breaking it slightly does not warrant Medium severity.

[sherlock-admin3]

Escalate

Issue should be invalidated. It does not showcase any Medium-severity impact, but rather just a slightly incorrect code comment. Contest readme did not include said invariant as one that must always be true, so simply breaking it slightly does not warrant Medium severity.

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]

If the protocol team provides specific information in the README or CODE COMMENTS, that information stands above all judging rules.

The included code comment was an explicit invariant outlined by the team:

#  - the protocol handles the accounting needed to maintain the sytem invariants:
#    * funding payments match
#        sum(funding_received) = sum(funding_paid)

The problematic code was actually intended to enforce this invariant, however it does so incorrectly. In the case of a negative position due to fee underpayment, sum(funding_received) > sum(funding_paid). To correct for this, or serve as a deterrent, these positions will not receive their funding payment. However the token in which they underpaid is not the same token that they will not receive funding payment for. As a result the imbalance between funding paid and received is not corrected - it is actually worsened.

Not only that, but users may have paid their full funding payment and the sum(funding_received) = sum(funding_paid) invariant holds. But if the remaining balance was then not enough to cover their borrow fee, they will not receive their funding payment which would actually cause this invariant to break.

This specification is the source of truth, and the code clearly does not abide by it. The issue proposes alternative methods for accounting funding fee payments to ensure this invariant consistently holds.

[msheikhattari]

Also I do think this issue is similar to #18 and #93

Will leave it to HoJ if they are combined since the source of the error is the same, but different impacts are described.

This issue discusses the code being not to spec and breaking an invariant.
The other two issues mention a known issue from the sponsor of stuck/undistributed funds

[WangSecurity]

@spacegliderrrr is correct that indeed breaking the invariant from the code comment doesn't make the issue medium-severity necessarily. But, as I understand, it also works as intended. The position pays its fees, but if there is not enough collateral, then position cannot pay the fee and this fee will be unpaid/underpaid. And, IIUC, in this protocol the borrowing fees comes first and if the remaining collateral cannot pay the funding fee in full, then it shouldn't be. Hence, this is also enough of a contextual evidence that the comment is outdated. Do I miss anything here?

[spacegliderrrr]

@WangSecurity Most of your points are correct. Yes the code works as expected. Funding fee comes before borrowing fee. But for both of them, since due to fail to liquidate position on time, fees can exceed the total position collateral. Because of this, funding fees are paid with priority and if there's anything left, borrowing fees are paid for as much as there's left.

In the case where funding fees are overpaid (for more than the total position collateral), the other side receives these fees in a FIFO order, which is also clearly stated in the comments.

[msheikhattari]

@spacegliderrrr is correct, funding fees are paid before borrow fees. As such there is no evidence that the comment spec is outdated, nor does any other documentation such as the readme appear to indicate so.

In the case where funding fees are overpaid (for more than the total position collateral), the other side receives these fees in a FIFO order, which is also clearly stated in the comments.

To elaborate on this point, the vulnerability describes the case that funding_received from the other side is greater than the funding_paid. While it is indeed acknowledged fees are received in FIFO order, this is a problematic mitigation for this issue. This current approach is a questionable means of correcting the imbalance of funding payments to receipts. There are many cases where it not only doesn't correct for the funding payment imbalance, but actually worsens it, as explained above.

Regardless, this seems to be a clearly defined invariant. Not only from this comment but further implied by the logic of this fee handling, which penalizes negative positions to build up some "insurance" tokens to hedge against the case that funding fees are underpaid. It also intuitively makes sense for this invariant to generally hold as otherwise malfunctions can occur such as failure to close positions; several linked issues reported various related problems stemming from this invariant breaking as well.

[WangSecurity]

Another question I've got after re-reading the report. The impact section says there will be stuck tokens in the contract which will never be paid. But, as I understand the problem is different. The issue is that these tokens are underpaid, e.g. if the funding fee is 10 tokens, but the remaining collateral is only 8, then there are 2 tokens underpaid. So, how the are stuck tokens in the contract, if the funding fee is not paid in full. Or it refers to the funding_received being larger than the funding_received actually?

[WangSecurity]

Since no answer is provided, I assume it's a typo in the report and the contract doesn't have fewer tokens, since the fee is underpaid, not overpaid.
But, the issue here is not medium severity, because the position with collateral less than the funding fees and it cannot pay the full fee. Just breaking the invariant is not sufficient for medium severity. Planning to accept the escalation and invalidate the issue.

[msheikhattari]

Apologies for the delayed response @WangSecurity

Yes, the original statement was as intended. That portion of the report was pointing out that there are two problematic impacts of the current approach that cause sum(funding_paid) != sum(funding_received)

1. It's possible for funding_received by one side of the pool to exceed the funding_paid by the other side, in the case that a position went negative.
2. It's possible for funding_received to be less than funding_paid, even for positions which did not go insolvent due to funding rates.

The outlined invariant is broken which results in loss of funds / broken functionality. It will prevent closing of some positions in the former case, and will result in some funds being stuck in the latter case.

The current approach of penalizing negative positions is intended to 'build up an insurance' as stated by the team. But as mentioned here, not only does it not remediate the issue it actually furthers the imbalance. The funding tokens have already been underpaid in the case of a negative position - preventing those positions from receiving their fees results in underpayment of their funding as well.

[WangSecurity]

It's possible for funding_received by one side of the pool to exceed the funding_paid by the other side, in the case that a position went negative.

Could you elaborate on this? Similar to my analysis here, If the position is negative, but the collateral is not 0, the funding paid is larger than the position's collateral, the funding paid will be decreased to pos. collateral. The funding received will be 0 (if collateral is < funding paid, c1: remaining =0 and deducted =pos.collateral. Then c2: remaining =0 and deducted =0. funding received =0, because remaining =0. So, not sure how funding_received can be > funding paid and I need a more concrete example with numbers.

It's possible for funding_received to be less than funding_paid, even for positions which did not go insolvent due to funding rates.

Agree that it's possible.

The outlined invariant is broken which results in loss of funds / broken functionality. It will prevent closing of some positions in the former case, and will result in some funds being stuck in the latter case.

However, the report doesn't showcase that this broken invariant (when funding received < funding paid) will result in any of this. The report only says that it will lead to a dilution of fees, but there is still no explanation of how.

Hence, without a concrete example of how this leads to positions not being closed, thus leading to losses of these positions, and owners, I cannot verify.

I see that the invariant doesn't hold, but this is not sufficient for Medium severity (only broken invariants from README are assigned Medium, not invariants from code comments). Hence, the decision for now is to invalidate this issue and accept the escalation. But, if there's a POC, how does this lead to loss of funds in case funding received < funding paid, or how does funding received > funding paid and how does this lead to a loss of funds, I will consider changing my decision.

[WangSecurity]

If no answer is provided, planning to accept the escalation and invalidate the issue.

[msheikhattari]

I see that the invariant doesn't hold, but this is not sufficient for Medium severity (only broken invariants from README are assigned Medium, not invariants from code comments)

Doesn't the below rule from the severity categorization apply here? That's what I had in mind when creating the issue, since this comment was the only source of documentation on this specification. There is no indication that this is out of date based on any other specs or code elsewhere.

Hierarchy of truth: If the protocol team provides no specific information, the default rules apply (judging guidelines).

If the protocol team provides specific information in the README or CODE COMMENTS, that information stands above all judging rules. In case of contradictions between the README and CODE COMMENTS, the README is the chosen source of truth.

Nevertheless there are loss of funds from each case, let me first prove that funding_received > funding_paid is possible:

Then c2: remaining =0 and deducted =0. funding received =0, because remaining =0.

This is the crux of the issue I am reporting here. That's true in the case of a single position, the issue is that funding_received of that position being set to 0 does not mitigate the underpayment of funding which it made by going negative - they are opposite tokens in the pair.

While that positions funding_payment is capped at the collateral of the specific position, the other side of the pool will continue to accrue funding receipts from this portion of the collateral until it's liquidated:

  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)

This is because this collateral is not excluded from fs.long_collateral, it must be explicitly subtracted upon liquidation.

Now the issue causing loss of funds on this side is that positions will fail to close. On the other side, where funding_received < funding_paid, this is especially problematic in cases where the full funding payment was made, but the collateral fell short of the borrow fee.

In this case, the balance sum(funding_received) = sum(funding_paid) was not broken by this position, as it made its full funding payment, but it will be excluded from receiving its portion of funding receipts. These tokens will not be directly claimable by any positions in the pool, causing loss of funds in that sense.

Upholding this balance of funding payments to receipts is an important invariant which causes loss of funds and protocol disruptions as outlined above. This is even acknowledged by the team, since this current approach is meant to build up an "insurance" by penalizing negative positions to pay out future negative positions.

What it fails to acknowledge is that the penalized position has already underpaid its funding fee (in token A), and now the balance is further distorted by eliminating its funding payment (in token B). To move closer to equilibrium between the two sides of funding, a direct approach such as socializing fee underpayments is recommended instead.

There are a few different moving parts so let me know if any component of this line of reasoning is unclear and I could provide specific examples if needed.

[WangSecurity]

Doesn't the below rule from the severity categorization apply here? That's what I had in mind when creating the issue, since this comment was the only source of documentation on this specification. There is no indication that this is out of date based on any other specs or code elsewhere

I didn't say the rule applies here, but if there's something said in the code comments and the code doesn't follow it, the issue still has to have at least Medium severity to be valid:

The protocol team can use the README (and only the README) to define language that indicates the codebase's restrictions and/or expected functionality. Issues that break these statements, irrespective of whether the impact is low/unknown, will be assigned Medium severity. High severity will be applied only if the issue falls into the High severity category in the judging guidelines.

So if the issue breaks an invariant from code comments, but it doesn't have Medium severity, then it's invalid.

This is the crux of the issue I am reporting here. That's true in the case of a single position, the issue is that funding_received of that position being set to 0 does not mitigate the underpayment of funding which it made by going negative - they are opposite tokens in the pair.
While that positions funding_payment is capped at the collateral of the specific position, the other side of the pool will continue to accrue funding receipts from this portion of the collateral until it's liquidated:

That still doesn't prove how funding_recevied can be > funding_paid. The function calc_fees which is where this calculation of funding_received and funding_paid happens is called inside value function which is called only inside close and is_liquidatable , which is called only inside liquidate.

Hence, this calculation of funding paid and received will be made only when closing or liquidating the position. So, the following is wrong:

the other side of the pool will continue to accrue funding receipts from this portion of the collateral until it's liquidated

It won't accrue because the position is either already liquidated or closed. Hence, the scenario of funding_received > funding_paid is still not proven.

About the funding_received < funding_paid. As I understand, it's intended that the position doesn't receive any funding fees in this scenario which evident by code comment.

So if the position didn't manage to pay the full funding_paid (underpaid the fees), they're intentionally excluded from receiving any funding fees and it's not a loss of funds and these will be received by other users.

Hence, my decision remains: accept the escalation and invalidate the issue. If you still see that I'm wrong somewhere, you're welcome to correct me. But, to make it easier, provide links to the appropriate LOC you refer to.

[msheikhattari]

Hence, this calculation of funding paid and received will be made only when closing or liquidating the position. So, the following is wrong:

That's not quite correct. Yes, calc_fees is only called upon closing/liquidating the position. But, this only only calculates the user's pro-rate share of the accrued interest:

def calc(id: uint256, long: bool, collateral: uint256, opened_at: uint256) -> SumFees:
    period: Period  = self.query(id, opened_at)
    P_b   : uint256 = self.apply(collateral, period.borrowing_long) if long else (
                      self.apply(collateral, period.borrowing_short) )
    P_f   : uint256 = self.apply(collateral, period.funding_long) if long else (
                      self.apply(collateral, period.funding_short) )
    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})

The terms period.received_long, period.received_short are relevant here and these are continuously, globally updated upon any interaction by any user with the system. As a result, those positions will count towards the total collateral until explicitly closed, inflating funding_received beyond its true value.

and it's not a loss of funds and these will be received by other users.

The point of this issue is that user's will not receive those lost funds. Since the global funding_received included the collateral of positions which were later excluded from receiving their fee, the eventual pro-rata distribution of fees upon closing the position will not be adjusted for this. Thus some portion of fees will remain unclaimed.

The current approach is problematic with loss of funds and disrupted liquidation functionality. There are more direct ways to achieve the correct balance of funding payments to receipts.

[WangSecurity]

To finally confirm, the problem here is that these funds are just locked in the contract, and no one can get them, correct?

[rickkk137]

as I got the main point in this report is funding_received can exceed funding_paid but this isn't correct

It's possible for funding_received by one side of the pool to exceed the funding_paid by the other side, in the case that a position went negative.

provided PoC

    def test_funding_received_cannot_exceed_than_funding_paid(core, BTC,mint_token, STX, lp_provider, LP, open, mint, short, positions, owner, pools, fees, oracle, api, long, close):
    pools.CORE()      == core
    fees.CORE()       == core
    positions.CORE()  == core
    oracle.API()      == api
    core.API()        == api
    mint_token(BTC, btc(1000), lp_provider)
    mint_token(BTC, btc(1000), short)
    mint_token(STX, d(100000), long)


    mint_token(STX, d(100_000), lp_provider)
    assert BTC.balanceOf(lp_provider) == btc(1000)
    assert BTC.balanceOf(short) == btc(1000)

    assert STX.balanceOf(lp_provider) == d(100_000)

    core.fresh("BTC-STX", BTC, STX, LP, sender=owner)
    BTC.approve(core.address, btc(1000), sender=lp_provider)
    STX.approve(core.address, d(100_000), sender=lp_provider)
    mint(BTC, STX, LP, btc(100), d(100_000), price=d(50_000), sender=lp_provider)

    BTC.approve(core.address, 10000000, sender=short)
    STX.approve(core.address, d(100000), sender=long)
    print("stx_balance:", STX.balanceOf(long) / 1e6)

    open(BTC, STX, True, d(50000), 1, price=d(50_000), sender=long)
    open(BTC, STX, False, 10000, 1, price=d(50_000), sender=short)


    chain.mine(20000)

    position = positions.value(1, ctx(d(50_000)))#funding_fee payer
    position2 = positions.value(2, ctx(d(50_000)))#funding_fee receiver

    print("position.remaining_collateral:", position.pnl.remaining / 1e6)
    print("position.fees.funding_paid:", position.fees.funding_paid / 1e6)

    print("position.funding_received:", position2.fees.funding_received / 1e6)
    
    #@>>>>>position.remaining_collateral: 0.0
    #@>>>>>position.fees.funding_paid: 50000.0
    #@>>>>>position.funding_received: 50000.0

[WangSecurity]

Yeah, there isn't a proof it can happen, the problem we are discussing now is that in cases where funding received is larger than funding paid (can happen when the position is negative), the funding received would be stuck in the contract with no one being able to get them. @rickkk137 would the funding fees be distributed to other users in this case?

[rickkk137]

In my poc the position finally become negative but funding received is equal to funding paid
But funding receiving wouldn't stuck in contracts and will be distributed to later positions

[WangSecurity]

Thank you for this correction. With this, the escalation will be accepted and the issue will be invalidated. The decision will be applied in a couple of hours.

[msheikhattari]

Hi @rickkk137 can you specifically clarify the scenario which the PoC is describing? Because from my understanding, it is not showing the case which I described. The nuance is subtle, allow me to explain

Let's say there is one position on the long side, two on the short side, and that longs are paying shorts. Now if one of the short positions goes negative, the other will not receive the total funding fees over the period, it will only get its pro-rata share from the time that the (now liquidated) position was still active.

For comparison, it seems your PoC is showing a single long and short, and you are showing that when the long is liquidated the short should still receive their funding payment. This is a completely different scenario from what is described in this issue.

To answer your earlier question @WangSecurity

To finally confirm, the problem here is that these funds are just locked in the contract, and no one can get them, correct?

Yes, thats correct. That portion of the funding payments is not accessible to any users.

[rickkk137]

when a position be penalized and funding_received for that position become zero and base or quote collateral's pool will be decrease

@external
def close(id: uint256, d: Deltas) -> PoolState:
...
    base_collateral  : self.MATH.eval(ps.base_collateral,  d.base_collateral),
    quote_collateral : self.MATH.eval(ps.quote_collateral, d.quote_collateral),

its mean other position get more funding_received compared to past because funding_recieved has reserve relation with base or qoute collateral

  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)

[msheikhattari]

The pro rata share of funding received will be correctly adjusted moving forward from liquidation. But the point is that the period.funding_received terms already included the now liquidated collateral, so the other positions do not receive adjusted distributions to account for that.

[rickkk137]

def query(id: uint256, opened_at: uint256) -> Period:
  """
  Return the total fees due from block `opened_at` to the current block.
  """
  fees_i : FeeState = Fees(self).fees_at_block(opened_at, id)
  fees_j : FeeState = Fees(self).current_fees(id)
  return Period({
    borrowing_long  : self.slice(fees_i.borrowing_long_sum,  fees_j.borrowing_long_sum),
    borrowing_short : self.slice(fees_i.borrowing_short_sum, fees_j.borrowing_short_sum),
    funding_long    : self.slice(fees_i.funding_long_sum,    fees_j.funding_long_sum),
    funding_short   : self.slice(fees_i.funding_short_sum,   fees_j.funding_short_sum),
   @>>> received_long   : self.slice(fees_i.received_long_sum,   fees_j.received_long_sum),
  @>>>  received_short  : self.slice(fees_i.received_short_sum,  fees_j.received_short_sum),
  })
both will be updated when liquidable position will be closed
  

[msheikhattari]

That's not quite right. From current_fees:

  paid_short_term     : uint256 = self.apply(fs.short_collateral, fs.funding_short * new_terms)
  received_long_term  : uint256 = self.divide(paid_short_term,    fs.long_collateral)
  received_long_sum   : uint256 = self.extend(fs.received_long_sum,  received_long_term,  1)

So as mentioned in my point above, the collateral at the time of each global fee update is used. When a user later claims his pro-rate share, he will receive his fraction of the total collateral at the time of the fee update. Fee updates are performed continuously after each operation, and the total collateral may no longer be representative due to liquidated positions being removed from this sum. However, they were still included in the received_{short/long}_term, which is added onto the globally stored received_{short/long}_sum

Thus some share of these global fees are not accessible.

[WangSecurity]

As I understand it:
The fee state is checked twice in the liquidate/close. Let's take close for example:

1. The state is checked during the close in the positions. It doesn't update the state and calculates the funding_received and funding_paid (well, in fact, it calls close, then it calls value, which calls calc_fees) which gives us 0 funding_received and thus 0 added to the quote_collateral.
2. Then core::close updates the Pool and then updates the fees.
3. When we update the fees, we use FeeState.base_collateral (assuming the scenario with two shorts) when calculating the fees received by shorts for this term. But, the FeeState.base collateral is changed only after calculating received_short_term/sum

So even though the closed/liquidated position didn't receive any fees and they should go to another short, the received_short_term accounted as there were two shorts opened, and each received their funding fees.

Hence, when the other short position gets the fees, they will add only a portion from that period, not the full fee.

Thus, I agree it should remain valid. However, medium severity should be kept because, in reality, if this situation occurs, the collateral of that closing/liquidatable position would be quite low (lower than funding_paid), there would be many positions, so the individual loss would be smaller (in terms of the amount, not %), the period with incorrectly applied fees would be small (given the fact that fees are updated at each operation). Hence, the loss is very limited. Planning to reject the escalation and leave the issue as it is, it will be applied at 10 am UTC.

[WangSecurity]

Result:
Medium
Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• spacegliderrrr: rejected

[WangSecurity]

#93 and #18 are duplicates of this issue; they both identify that if the negative position is closed, the funding_received for it reset to 0, but these funding fees are not received by any other user and remain stuck in the contract forever.