bughuntoor - Funding rate will be lost if there's no positions on one of the sides

[sherlock-admin3]

bughuntoor

Medium

Funding rate will be lost if there's no positions on one of the sides

Summary

Funding rate will be lost if there's no positions on one of the sides

Vulnerability Detail

The way funding rate works is that the side which has larger utilization pays a funding fee to the other side's position holders. The exact amount is based on the delta of both utilization percentages.

@internal
@view
def funding_fee(base_fee: uint256, col1: uint256, col2: uint256) -> uint256:
  imb: uint256 = self.imbalance(col1, col2)
  if imb == 0: return 0
  else       : return self.check_fee(self.scale(base_fee, imb))

The problem lies in the situation where there's only position on one side. In this case said position holders will pay the funding fees, but no one will actually receive them and they'll be permanently stuck in the contract, marked as collateral

Impact

Loss of funds

Code Snippet

https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/params.vy#L84

Tool used

Manual Review

Recommendation

Do not calculate funding fees if either side has no position holders

[spacegliderrrr]

Escalate

Issue should be validated as it showcases a realistic scenario where funds will be lost. It is a situation which will ALWAYS happen for new pools for first depositors (until there are people opening on the other side). It is also something which could occasionally occur on its own. Issue should be of Medium severity.

[sherlock-admin3]

Escalate

Issue should be validated as it showcases a realistic scenario where funds will be lost. It is a situation which will ALWAYS happen for new pools for first depositors (until there are people opening on the other side). It is also something which could occasionally occur on its own. Issue should be of 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]

This is a duplicate of #93

The same issue of undistributed or stuck funding_received is described, just with a different impact (which can be combined)

[ami0x226]

This is not a duplicate of #93.
The root causes are different.
#92 is here: when fs.short_collateral = 0, it returns 0 in divide function.

  received_short_term : uint256 = self.divide(paid_long_term,    fs.short_collateral)

#93 is here: multiply 0.

    R_f   : uint256 = self.multiply(collateral, period.received_long) if long else (
                      self.multiply(collateral, period.received_short) )

As a result, they are different and #92 is valid medium severity.

[spacegliderrrr]

@msheikhattari Not sure how could they be dups given that in #92 the pre-condition is that there must be no funds on one side and in #93 funds are lost if position goes in the negative due to fees and failing liquidation bots.

[msheikhattari]

The differing causes boil down to the same root issue of funding payment vs receipt mismatch due to asynchronous accounting. I don't disagree that the specific causes/impacts differ, but fundamentally these issues are pointing to the same thing. Up to HoJ if they should be combined or remain as distinct issues.

#18
#83
#93

[WangSecurity]

I see the impact is loss of funds, but who should've got these funds? For example, there's one long position and no short positions. The long position has to pay the funding fee, so it's not a loss for them. But, there's no one on the other side, so no one loses these tokens, since there's no one who should've receive them. I see that it leads to stuck tokens, but just stuck tokens (when no one lost them) is not medium severity by itself. Do I miss anything?

[WangSecurity]

Since no answer is provided, planning to reject the escalation and leave the issue as it is.

[spacegliderrrr]

I don't believe the logic is correctly applied here - when tokens unintentionally get stuck, they're ultimately lost as no one can retrieve them.

When there was no one that should've received them, they shouldn't have been paid. So the loss is for the user paying the funding fee.

[ami0x226]

I agree with @spacegliderrrr. When there was no one that should've received them, they shouldn't have been paid.
Alternatively, it should accumulate the paid imbalance fee until the opposite position is created.
Locking tokens causes unnecessary loss of funds.

[WangSecurity]

Then, I think there was a misunderstanding on my side. Even if the user opens only one position, shouldn't they pay the funding fee regardless of existing positions on the other side?

[WangSecurity]

bumping @spacegliderrrr and @ami0x226 to clarify.

[spacegliderrrr]

@WangSecurity Funding fee is intended to be paid to the other side positions. If there's no positions on the other side, there's no one to pay to, hence it should not be paid.

[WangSecurity]

Thank you for that clarification. My assumption was that if there is an open position, it has to pay funding anyway. Planning to accept the escalation and validate with medium severity. I see there are several findings mentioned as duplicates, but they all share the same root cause and are/were escalated, so they were/will be treated separately. If there are any other dupes, let me know. Planning to accept the escalation and validate with medium severity.

[rickkk137]

invalid
Users get more funding_received when there is funding_paid in protocol from the past in comparison to when there isn't funding_paid,its means funding_paid will be distributed to future positions

consider add this test to tests/test_positions.py and run tests with pytest -k test_when_there_is_not_funding_paid_in_portocol -s and pytest -k test_when_there_is_funding_paid_in_portocol -s

Provided PoC

@pytest.fixture
def create_token8(project, owner):
    def create_token18(name):
        return owner.deploy(project.ERC20, name, name, 8, btc(1000000))
    return create_token18

@pytest.fixture
def BTC(create_token8): return create_token8("BTC")

def test_when_there_is_funding_paid_in_portocol(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)

    chain.mine(1)

    position = positions.value(1, ctx(d(50_000)))
    print("position.fees.funding_paid:", position.fees.funding_paid)

    close(BTC, STX, 1, price=d(50000), sender=long)
    pool = pools.lookup(1)
    fs = fees.lookup(1)
    print("fee_fs_long:", fs.long_collateral)
    print("pool_collateral:", pool.quote_collateral)

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

    chain.mine(4)
    fs = fees.lookup(1)
    print("fee_fs_long:", fs.long_collateral)

    position2 = positions.value(2, ctx(d(50_000)))
    position3 = positions.value(3, ctx(d(50_000)))

#   position2_funding_paid: 25000000
#   @>>>>> position3_funding_received: 20004000
    print("position2_funding_paid:", position2.fees.funding_paid)
    print("position3_funding_received:", position3.fees.funding_received)


def test_when_there_is_not_funding_paid_in_portocol(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)
    

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

    chain.mine(4)
    fs = fees.lookup(1)
    print("fee_fs_long:", fs.long_collateral)

    position1 = positions.value(1, ctx(d(50_000)))
    position2 = positions.value(2, ctx(d(50_000)))

    #position1_funding_paid: 25000000
    #@>>>>> position2_funding_received: 20000000
    print("position1_funding_paid:", position1.fees.funding_paid)
    print("position2_funding_received:", position2.fees.funding_received)

when just one position exist in pool and position's owner deicide to close position and he has to pay funding_fee ,position's funding_fee remain in pool and fee.long_collateral and pool.quote_collateral will be increase accordingly, and when other users want to open position in result they get more funding_received becuase funding_received compute base on this formula:

  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)

its mean fs.long_collateral and paid_long_term have a direct relation together

[WangSecurity]

As I understand, the comment and the POC above is correct. The fee is not stuck in the contract and will be distributed later, thanks to @rickkk137 for correcting this. Planning to reject the escalation and leave the issue as it is.

[WangSecurity]

Result:
Invalid
Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• spacegliderrrr: rejected