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

7 stars 3 forks source link

0x37 - Last LP in each pool may lose a few amount of reserve. #15

Closed sherlock-admin4 closed 1 month ago

sherlock-admin4 commented 2 months ago

0x37

Medium

Last LP in each pool may lose a few amount of reserve.

Summary

Last Lp's burn() may be reverted if malicious users leave 1 wei share in this pool.

Vulnerability Detail

At the end of each core function, we will trigger Fee's update. In the fee's update, we need to calculate the Lp tokens' utilization. The problem is that the utilization calculation will be reverted if reserves is not zero but less than 100.

This scenario is possible, especially when malicious users intent to do this.

The last LP holder can still burn his most share via leaving some amount of share(reserves) into the pool. Although this amount is not large, considering that this case could happen in every market, more LPs may take some loss because of this finding.

@internal
@pure
def utilization(reserves: uint256, interest: uint256) -> uint256:
    # Then the last LP want to withdraw, will be reverted.
@>    return 0 if (reserves == 0 or interest == 0) else (interest / (reserves / 100))
@external
@view
def calc_mint(
  id          : uint256,
  base_amt    : uint256,
  quote_amt   : uint256,
  total_supply: uint256,
  ctx         : Ctx) -> uint256:
  pv: uint256 = self.MATH.value(Pools(self).total_reserves(id), ctx).total_as_quote
  mv: uint256 = self.MATH.value(Tokens({base: base_amt, quote: quote_amt}), ctx).total_as_quote
  return Pools(self).f(mv, pv, total_supply)

# ts --> total supply
@external
@pure
def f(mv: uint256, pv: uint256, ts: uint256) -> uint256:
  if ts == 0: return mv
  else      : return (mv * ts) / pv

Impact

Last LP holder in each pool may have to leave a few amount of reserve in this market.

Code Snippet

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

Tool used

Manual Review

Recommendation

Make sure the utilization calculation will not be reverted.

spacegliderrrr commented 1 month ago

Escalate.

Issue is not realistic. If interest is 0, the else statement will not be entered and transaction will not revert. For the interest to be non-zero amount, it would have to be above the MINIMUM_SIZE, which also invalidates the issue (as there'd be significant amount of funds in positions which cannot be withdrawn at the moment).

Issue should be invalidated

sherlock-admin3 commented 1 month ago

Escalate.

Issue is not realistic. If interest is 0, the else statement will not be entered and transaction will not revert. For the interest to be non-zero amount, it would have to be above the MINIMUM_SIZE, which also invalidates the issue (as there'd be significant amount of funds in positions which cannot be withdrawn at the moment).

Issue should be invalidated

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

Regardless the funds at stake are minimal if this attack were even successful, since both reserves and interest have some value below 100 wei.

0xweb333 commented 1 month ago

https://github.com/sherlock-audit/2024-08-velar-artha-judging/issues/97 is a duplicate, has the same root cause

rickkk137 commented 1 month ago

invalid

def burn(

  ...
@>>  self.INVARIANTS(id, base_token, quote_token)

  log Burn(user, ctx, pool, total_supply, lp_amt, base_amt, quote_amt)

  return amts
def INVARIANTS(id: uint256, base_token: address, quote_token: address):

@>>>  assert pool.base_reserves  >= pool.base_interest,  ERR_INVARIANTS
@>>>  assert pool.quote_reserves >= pool.quote_interest, ERR_INVARIANTS

after every action INVARIANTS will be called which checks reserve be greater than open_interest and when just 1 wei remain in pool its mean there isn't open position ,hence utilization will be zero

def utilization(reserves: uint256, interest: uint256) -> uint256:
@>    return 0 if (reserves == 0 or interest == 0) else (interest / (reserves / 100))
WangSecurity commented 1 month ago

As I understand, based on this and this comments, this issue cannot happen.

Planning to accept the escalation and invalidate the issue.

WangSecurity commented 1 month ago

Result: Invalid Unique

sherlock-admin4 commented 1 month ago

Escalations have been resolved successfully!

Escalation status: