sherlock-audit / 2024-08-velar-artha-judging

5 stars 2 forks source link

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

Open sherlock-admin2 opened 1 week ago

sherlock-admin2 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 6 days ago

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 commented 2 days ago

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:

WangSecurity commented 19 hours ago

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 commented 12 hours ago

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 commented 10 hours ago

@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 commented 20 minutes ago

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?