bughuntoor - User can sandwich their own position close to get back all of their position fees

[sherlock-admin2]

bughuntoor

High

User can sandwich their own position close to get back all of their position fees

Summary

User can sandwich their own position close to get back all of their position fees

Vulnerability Detail

Within the protocol, borrowing fees are only distributed to LP providers when the position is closed. Up until then, they remain within the position.
The problem is that in this way, fees are distributed evenly to LP providers, without taking into account the longevity of their LP provision.

This allows a user to avoid paying fees in the following way:

1. Flashloan a large sum and add it as liquidity
2. Close their position and let the fees be distributed (with most going back to them as they've got majority in the pool)
3. WIthdraw their LP tokens
4. Pay back the flashloan

Impact

Users can avoid paying borrowing fees.

Code Snippet

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

Tool used

Manual Review

Recommendation

Implement a system where fees are gradually distributed to LP providers.

[KupiaSecAdmin]

Escalate

This is invalid, FEES.update function is called after every action, so when the liquidity of flashloan is added, the accrued fees are distributed to prev LP providers.

[sherlock-admin3]

Escalate

This is invalid, FEES.update function is called after every action, so when the liquidity of flashloan is added, the accrued fees are distributed to prev LP providers.

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]

Comment above is simply incorrect. Fees.update doesn't actually make a change on the LP token value - it simply calculates the new fee terms and makes a snapshot at current block.

The value of LP token is based on the pool reserves. Fees are actually distributed to the pool only upon closing the position, hence why the attack described is possible.

[rickkk137]

invalid
it is intended design

def f(mv: uint256, pv: uint256, ts: uint256) -> uint256:
  if ts == 0: return mv
  else      : return (mv * ts) / pv

def g(lp: uint256, ts: uint256, pv: uint256) -> uint256:
  return (lp * pv) / ts

Pool contract uses above formola to compute amount of LP token to mint and also burn value for example:
pool VEL-STX:
VEL reserve = 1000
STX reserve = 1000
VEL/STX price = $1
LP total_supply = 1000
pv stand for pool value and ts stand for total_supply and mv stand for mint value
LP_price = ts / pv = 1000 / 1000 = $1
its mean if user deposit $1000 into pool in result get 1000 LP token
and for burn
burn_value = lp_amount * pool_value / total_supply and based on above example
total_supply=2000
pool_value=2000
because user deposit $1000 into pool and mint 1000lp token

the issue want to say mailicious user with increase lp price can retrieve borrowing_fee ,but when mailicious users and other users closes their profitable positions pool_value will be decreased[the protocol pay users' profit from pool reserve],hence burn_value become less than usual state

requirment internal state for attack path:

• mailicious user's position be in loss[pool value will be decreased because user collateral will be added to pool reserve]
• other user don't close their profitable positions in that block
• flashloan's cost be low

[WangSecurity]

I see how it can be viewed as the intended design, but still, the user can bypass the fees essentially and LPs lose them, when they should've been to receive it. Am I missing anything here?

[rickkk137]

attack path is possible when user's position is in lose and loss amount has to be enough to increase the LP price , note that if loss amount be large enough the position can be eligible for liquidation and the user has to close his position before liquidation bot

[spacegliderrrr]

@WangSecurity Your comment is correct. Anytime the user is about to close their position (whether it be unprofitable, neutral or slightly profitable), they can perform the attack to avoid paying the borrowing fees, essentially stealing them from the LPs.

[WangSecurity]

attack path is possible when user's position is in lose and loss amount has to be enough to increase the LP price , note that if loss amount be large enough the position can be eligible for liquidation and the user has to close his position before liquidation bot

So these are the constraints:

1. Loss should be relatively large so it increases the LP price.
2. The attacker has to perform the attack before the liquidation bot liquidates their position which is also complicated by the private mempool on Bob, so we cannot see the transaction of the liquidation bot.

Are the above points correct and is there anything missing?

[deadrosesxyz]

No, loss amount does not need to be large. Attack can be performed on any non-profitable position, so the user avoids paying fees.

[WangSecurity]

In that case, I agree that it's an issue that the user can indeed bypass the fees and prevent the LPs from receiving it. Also, I don't see the pre-condition of the position being non-profitable as an extensive limitation. Moreover, since it can be any non-profitable position, then there is also no requirement for executing the attack before the liquidation bot (unless the position can be liquidated as soon as it's non-profitable).

Thus, I see that it should be high severity. If something is missing or these limitations are extensive in the context of the protocol, please let me know. Planning to reject the escalation, but increase severity to high.

[WangSecurity]

Result:
High
Has duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• KupiaSecAdmin: rejected

[sherlock-admin2]

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