bughuntoor - User could have impossible to close position if funding fees grow too big.

[sherlock-admin4]

bughuntoor

High

User could have impossible to close position if funding fees grow too big.

Summary

User could have impossible to close position if funding fees grow too big.

Vulnerability Detail

In order to prevent positions from becoming impossible to be closed due to funding fees surpassing collateral amount, there's the following code which pays out funding fees on a first-come first-serve basis.

    # 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) )

However, this wrongly assumes that the order of action would always be for the side which pays funding fees to close their position before the side which claims the funding fee.

Consider the following scenario:

1. There's an open long (for total collateral of X) and an open short position. Long position pays funding fee to the short position.
2. Eventually the funding fee grows larger than the whole long position (X + Y). it is due liquidation, but due to bot failure is not yet liquidated (which based on comments is expected and possible behaviour)
3. A new user opens a new long position, once with X collateral. (total quote collateral is currently 2X)
4. The original long is closed. This does not have an impact on the total quote collateral, as it is increased by the funding_paid which in our case will be counted as exactly as much as the collateral (as in these calculations it cannot surpass it). And it then subtracts that same quote collateral.
5. The original short is closed. funding_received is calculated as X + Y and therefore that's the amount the total quote collateral is reduced by. The new total quote collateral is 2X - (X + Y) = X - Y.
6. Later when the user attempts to close their position it will fail as it will attempt subtracting (X - Y) - X which will underflow.

Marking this as High, as a user could abuse it to create a max leverage position which cannot be closed. Once it is done, because the position cannot be closed it will keep on accruing funding fees which are not actually backed up by collateral, allowing them to double down on the attack.

Impact

Loss of funds

Code Snippet

https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/positions.vy#L250
https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/positions.vy#L263
https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/positions.vy#L211

Tool used

Manual Review

Recommendation

Fix is non-trivial.

[KupiaSecAdmin]

Escalate

The underflow does not happen by nature of deduct function. Thus this is invalid.

[sherlock-admin3]

Escalate

The underflow does not happen by nature of deduct function. Thus this is invalid.

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.

[spacegliderrrr]

Escalate

Severity should be High. Issue above describes how a user could open risk-free max leverage positions, basically making a guaranteed profit from the LPs.

Regarding @KupiaSecAdmin escalation above - please do double check the issue above. The underflow does not happen in deduct but rather in the MATH.eval operations. The problem lies within the fact that if order of withdraws is reversed, funding receiver can receive more fees than the total collateral (as long as it is available by other users who have said collateral not yet eaten up by funding fees). Then, some of the funding paying positions will be impossible to be closed.

[sherlock-admin3]

Escalate

Severity should be High. Issue above describes how a user could open risk-free max leverage positions, basically making a guaranteed profit from the LPs.

Regarding @KupiaSecAdmin escalation above - please do double check the issue above. The underflow does not happen in deduct but rather in the MATH.eval operations. The problem lies within the fact that if order of withdraws is reversed, funding receiver can receive more fees than the total collateral (as long as it is available by other users who have said collateral not yet eaten up by funding fees). Then, some of the funding paying positions will be impossible to be closed.

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 invalid.

4. The original long is closed. This does not have an impact on the total quote collateral, as it is increased by the funding_paid which in our case will be counted as exactly as much as the collateral (as in these calculations it cannot surpass it). And it then subtracts that same quote collateral.

When original long is closed, total quote collateral is changed.

File: gl-sherlock\contracts\positions.vy
209:     quote_reserves  : [self.MATH.PLUS(pos.collateral), #does not need min()
210:                        self.MATH.MINUS(fees.funding_paid)],
211:     quote_collateral: [self.MATH.PLUS(fees.funding_paid),
212:                        self.MATH.MINUS(pos.collateral)],

Heres, pos.collateral = X, fees.funding_paid = X + Y.
Then,
quote_collateral <- quote_collateral + X + Y - X = quote_collateral + Y = 2X + Y, and
quote_reserves <- quote_reserves + X - X - Y = quote_reserves - Y.

When original short is closed in step5, new total quote collateral is 2X + Y - (X + Y) = X and there is no underflow in step6.
As a result, the scenario of the report is wrong.
The loss causes in quote_reserves, but, in practice, Y is enough small by the frequent liquidation and it should be assumed that the liquidation is done correctly.
Especially, because the report does not mention about this vulnerability, I think this is invalid

[ami0x226]

Also, Funding paid cannot exceed collateral of a position from the apply function.

File: gl-sherlock\contracts\math.vy
167: def apply(x: uint256, numerator: uint256) -> Fee:
172:   fee      : uint256 = (x * numerator) / DENOM
173:   remaining: uint256 = x - fee if fee <= x else 0
174:   fee_     : uint256 = fee     if fee <= x else x
175:   return Fee({x: x, fee: fee_, remaining: remaining})

File: gl-sherlock\contracts\fees.vy
265: def calc(id: uint256, long: bool, collateral: uint256, opened_at: uint256) -> SumFees:
269:     P_f   : uint256 = self.apply(collateral, period.funding_long) if long else (
270:                       self.apply(collateral, period.funding_short) )
274:     return SumFees({funding_paid: P_f, funding_received: R_f, borrowing_paid: P_b})

[spacegliderrrr]

Heres, pos.collateral = X, fees.funding_paid = X + Y.

Here's where you're wrong. When the user closes their position, funding_paid cannot exceed pos.collateral. So fees.funding_paid == pos.collateral when closing the original long. Please re-read the issue and code again.

[ami0x226]

Heres, pos.collateral = X, fees.funding_paid = X + Y.

Here's where you're wrong. When the user closes their position, funding_paid cannot exceed pos.collateral. So fees.funding_paid == pos.collateral when closing the original long. Please re-read the issue and code again.

That's true. I mentioned about it in the above comment

Also, Funding paid cannot exceed collateral of a position from the apply function.

I just use fees.funding_paid = X + Y to follow the step2 of bughuntoor's scenario:

2. Eventually the funding fee grows larger than the whole long position (X + Y). it is due liquidation, but due to bot failure is not yet liquidated (which based on comments is expected and possible behaviour)

[rickkk137]

invalid
funding_paid cannot exceed than collateral also funding_received cannot be greater funding_paid

[WangSecurity]

@spacegliderrrr can you make a coded POC showcasing the attack path from the report?

[WangSecurity]

We've got the POC from the sponsor:

POC

from ape import chain
import pytest
from conftest import d, ctx

# https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/96

# 1) There's an open long (for total collateral of X) and an open short position. Long position pays funding fee to the short position.
# 2) Eventually the funding fee grows larger than the whole long position (X + Y). it is due liquidation, but due to bot failure is not yet liquidated (which based on comments is expected and possible behaviour)
# 3) A new user opens a new long position, once with X collateral. (total quote collateral is currently 2X)
# 4) The original long is closed. This does not have an impact on the total quote collateral, as it is increased by the funding_paid which in our case will be counted as exactly as much as the collateral (as in these calculations it cannot surpass it). And it then subtracts that same quote collateral.
# 5) The original short is closed. funding_received is calculated as X + Y and therefore that's the amount the total quote collateral is reduced by. The new total quote collateral is 2X - (X + Y) = X - Y.
# 6) Later when the user attempts to close their position it will fail as it will attempt subtracting (X - Y) - X which will underflow.

# OR
# 1. Consider the original long and short positions, long pays funding fee to short.
# 2. Time goes by, liquidator bots fails and funding fee makes 100% of the long collateral (consider collateral is X)
# 3. Another long position is opened again with collateral X.
# 4. Time goes by, equivalent to funding fee of 10%. Total collateral at this moment is still 2X, so the new total funding paid is 1.2X
# 5. Short position closes and receives funding paid of 1.2X. Quote collateral is now reduced from 2X to 0.8X.
# 6. (Optional) Original long closes position. For them funding paid is capped at their collateral, so funding paid == collateral, so closing does not make a difference on the quote collateral.
# 7. The next long position holder tries to close. They're unable because their collateral is 1x, funding paid is 0.1x. Collateral calculation is 0.8X + 0.1X - 1X and underflow reverts

PARAMS = {
  'MIN_FEE'               : 1_00000000,
  'MAX_FEE'               : 1_00000000, # 10%/block
  'PROTOCOL_FEE'          : 1000,
  'LIQUIDATION_FEE'       : 2,
  'MIN_LONG_COLLATERAL'   : 1,
  'MAX_LONG_COLLATERAL'   : 1_000_000_000,
  'MIN_SHORT_COLLATERAL'  : 1,
  'MAX_SHORT_COLLATERAL'  : 1_000_000_000,
  'MIN_LONG_LEVERAGE'     : 1,
  'MAX_LONG_LEVERAGE'     : 10,
  'MIN_SHORT_LEVERAGE'    : 1,
  'MAX_SHORT_LEVERAGE'    : 10,
  'LIQUIDATION_THRESHOLD' : 1,
}

PRICE = 400_000

def test_issue(core, api, pools, positions, fees, math, oracle, params,
            VEL, STX, LP,
            mint, burn, open, close,
            long, short, lp_provider, long2, owner,
            mint_token):

    # setup
    core.fresh("VEL-STX", VEL, STX, LP, sender=owner)
    mint_token(VEL, d(100_000), lp_provider)
    mint_token(STX, d(100_000), lp_provider)
    mint_token(VEL, d(10_000) , long)
    mint_token(STX, d(10_000) , long)
    mint_token(VEL, d(10_000) , long2)
    mint_token(STX, d(10_000) , long2)
    mint_token(VEL, d(10_000) , short)
    mint_token(STX, d(10_000) , short)
    VEL.approve(core.address, d(100_000), sender=lp_provider)
    STX.approve(core.address, d(100_000), sender=lp_provider)
    VEL.approve(core.address, d(10_000) , sender=long)
    STX.approve(core.address, d(10_000) , sender=long)
    VEL.approve(core.address, d(10_000) , sender=long2)
    STX.approve(core.address, d(10_000) , sender=long2)
    VEL.approve(core.address, d(10_000) , sender=short)
    STX.approve(core.address, d(10_000) , sender=short)
    mint(VEL, STX, LP, d(10_000), d(4_000), price=PRICE, sender=lp_provider)
    params.set_params(PARAMS, sender=owner)         # set 10 % fee / block

    START_BLOCK = chain.blocks[-1].number
    print(f"Start block: {START_BLOCK}")

    # 1) There's an open long (for total collateral of X) and an open short position. Long position pays funding fee to the short position.
    # open pays funding when long utilization > short utilization (interest/reserves)
    X  = d(100)
    p1 = open(VEL, STX, True  , X   , 10, price=PRICE, sender=long)
    p2 = open(VEL, STX, False , d(5),  2, price=PRICE, sender=short)
    assert not p1.failed, "open long"
    assert not p2.failed, "open short"

    fees = params.dynamic_fees(pools.lookup(1))
    print(f"Pool fees: {fees}")

    # 2. Time goes by, liquidator bots fails and funding fee makes 100% of
    # the long collateral (consider collateral is X)
    chain.mine(10)

    # fees/value after
    value = positions.value(1, ctx(PRICE))
    print(value['fees'])
    print(value['pnl'])
    assert value['fees']['funding_paid']         == 99900000
    assert value['fees']['funding_paid_want']    == 99900000
    assert value['fees']['borrowing_paid']       == 0
    # assert value['fees']['borrowing_paid_want']  == 99900000
    assert value['pnl']['remaining']             == 0

    value = positions.value(2, ctx(PRICE))
    print(value['fees'])
    print(value['pnl'])
    assert value['fees']['funding_paid']            == 0
    assert value['fees']['funding_received']        == 99900000
    assert value['fees']['funding_received_want']   == 99900000
    # assert value['fees']['borrowing_paid']          == 4995000
    assert value['pnl']['remaining']                == 4995000

    # 3. Another long position is opened again with collateral X.
    p3 = open(VEL, STX, True, X, 10, price=PRICE, sender=long2)
    assert not p3.failed

    # 4. Time goes by, equivalent to funding fee of 10%.
    # Total collateral at this moment is still 2X, so the new total funding paid is 1.2X
    chain.mine(1)

    print(f"Pool: {pools.lookup(1)}")

    value = positions.value(1, ctx(PRICE))
    print(f"Long 1: {value['fees']}")
    print(f"Long 1: {value['pnl']}")

    value = positions.value(3, ctx(PRICE))
    print(f"Long 2: {value['fees']}")
    print(f"Long 2: {value['pnl']}")

    assert value['fees']['funding_paid'] == 9990000     #TODO: value with mine(2) is high?
    assert value['pnl']['remaining']     == 89910000

    print(f"Blocks: {chain.blocks[-1].number - START_BLOCK}")

    # 5. Short position closes and receives funding paid of 1.2X. Quote collateral is now reduced from 2X to 0.8X.
    tx = close(VEL, STX, 2, price=PRICE, sender=short)
    print(core.Close.from_receipt(tx)[0]['value'])
    # fees: [0, 0, 139860000, 139860000, 0, 0, 4995000]
    assert not tx.failed

    print(f"Blocks: {chain.blocks[-1].number - START_BLOCK}")

    pool = pools.lookup(1)
    print(f"Pool: {pool}")
    # assert pool['quote_collateral'] == 9990000 * 0.8  #59940000
    value = positions.value(3, ctx(PRICE))
    print(f"Long 2: {value['fees']}")
    print(f"Long 2: {value['pnl']}")
    print(f"Long 2: {value['deltas']}")

    # 7. The next long position holder tries to close. They're unable because their collateral is 1x, funding paid is 0.1x.
    # Collateral calculation is 0.8X + 0.1X - 1X and underflow reverts
    tx = close(VEL, STX, 3, price=PRICE, sender=long2)
    print(core.Close.from_receipt(tx)[0]['value'])
    assert not tx.failed, "close 2nd long"

    # 6. (Optional) Original long closes position. For them funding paid is capped at their collateral,
    # so funding paid == collateral, so closing does not make a difference on the quote collateral.
    tx = close(VEL, STX, 1, price=PRICE, sender=long)
    print(core.Close.from_receipt(tx)[0]['value'])
    assert not tx.failed, "close original"

    print(f"Blocks: {chain.blocks[-1].number - START_BLOCK}")

    pool = pools.lookup(1)
    print(f"Pool: {pool}")


    assert False

Hence, the issue is indeed valid. About the severity, as I understand, it's indeed high, since there are no extensive limitations, IIUC. Anyone is free to correct me and the POC, but from my perspective it's indeed correct.

But for now, planning to reject @KupiaSecAdmin escalation, accept @spacegliderrrr escalation and upgrade severity to high.

[KupiaSecAdmin]

@WangSecurity - No problem with having this issue as valid but the severity should be Medium at most I think.
Because, 1) Usually in real-world, funding fees don't exceed the collateral. 2) When positions get into risk, liquidation bots will work most of times.

[WangSecurity]

As far as I understand, this issue can be triggered intentionally, i.e. the first constraint can be reached intentionally, as explained at the end of the Vulnerability Detail section:

as a user could abuse it to create a max leverage position which cannot be closed

But you're correct that it depends on the liquidation bot malfunctioning, which is also mentioned in the report:

Eventually the funding fee grows larger than the whole long position (X + Y). it is due liquidation, but due to bot failure is not yet liquidated (which based on comments is expected and possible behaviour)

I agree that this is indeed an external limitation. Planning to reject both escalations and leave the issue as it is.

[WangSecurity]

Result:
Medium
Has duplicates

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• KupiaSecAdmin: rejected
• spacegliderrrr: rejected

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
https://github.com/Velar-co/gl-sherlock/pull/2