Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

4gontuk - LPs will withdraw more value than deposited during pegged token de-peg events #52

Open
sherlock-admin2 opened this issue Sep 9, 2024 · 5 comments
Labels
Escalation Resolved This issue's escalations have been approved/rejected Has Duplicates A valid issue with 1+ other issues describing the same vulnerability Medium A Medium severity issue. Reward A payout will be made for this issue Sponsor Disputed The sponsor disputed this issue's validity Won't Fix The sponsor confirmed this issue will not be fixed

Comments

@sherlock-admin2
Copy link
Contributor

sherlock-admin2 commented Sep 9, 2024

4gontuk

Medium

LPs will withdraw more value than deposited during pegged token de-peg events

Summary

The CONTEXT function in gl-sherlock/contracts/api.vy uses the <quote-token>/USD price for valuation, assuming a 1:1 peg between the quote token and USD. This assumption can fail during de-peg events, leading to incorrect valuations and potential exploitation.

Root Cause

The CONTEXT function calls the price function from the oracle contract to get the price of the quote token. This price is adjusted based on the quote_decimals, implying it is using the <quote-token>/USD price for valuation.

Detailed Breakdown

  1. CONTEXT Function in api.vy:
    The CONTEXT function calls the price function from the oracle contract to get the price of the quote token.
def CONTEXT(
    base_token : address,
    quote_token: address,
    desired    : uint256,
    slippage   : uint256,
    payload    : Bytes[224]
) -> Ctx:
  base_decimals : uint256 = convert(ERC20Plus(base_token).decimals(), uint256)
  quote_decimals: uint256 = convert(ERC20Plus(quote_token).decimals(), uint256)
  # this will revert on error
  price         : uint256 = self.ORACLE.price(quote_decimals,
                                              desired,
                                              slippage,
                                              payload)
  return Ctx({
    price         : price,
    base_decimals : base_decimals,
    quote_decimals: quote_decimals,
  })
  1. price Function in oracle.vy:
    The price function in oracle.vy uses the extract_price function to get the price from the oracle.
########################################################################
TIMESTAMP: public(uint256)

@internal
def extract_price(
    quote_decimals: uint256,
    payload       : Bytes[224]
) -> uint256:
  price: uint256 = 0
  ts   : uint256 = 0
  (price, ts) = self.EXTRACTOR.extractPrice(self.FEED_ID, payload)

  # Redstone allows prices ~10 seconds old, discourage replay attacks
  assert ts >= self.TIMESTAMP, "ERR_ORACLE"
  self.TIMESTAMP = ts

  # price is quote per unit base, convert to same precision as quote
  pd   : uint256 = self.DECIMALS
  qd   : uint256 = quote_decimals
  s    : bool    = pd >= qd
  n    : uint256 = pd - qd if s else qd - pd
  m    : uint256 = 10 ** n
  p    : uint256 = price / m if s else price * m
  return p

########################################################################
PRICES: HashMap[uint256, uint256]

@internal
def get_or_set_block_price(current: uint256) -> uint256:
  """
  The first transaction in each block will set the price for that block.
  """
  block_price: uint256 = self.PRICES[block.number]
  if block_price == 0:
    self.PRICES[block.number] = current
    return current
  else:
    return block_price

########################################################################
@internal
@pure
def check_slippage(current: uint256, desired: uint256, slippage: uint256) -> bool:
  if current > desired: return (current - desired) <= slippage
  else                : return (desired - current) <= slippage

@internal
@pure
def check_price(price: uint256) -> bool:
  return price > 0

# eof
  1. extract_price Function in oracle.vy:
    The extract_price function adjusts the price based on the quote_decimals, which implies it is using the <quote-token>/USD price for valuation.
def extract_price(
    quote_decimals: uint256,
    payload       : Bytes[224]
) -> uint256:
  price: uint256 = 0
  ts   : uint256 = 0
  (price, ts) = self.EXTRACTOR.extractPrice(self.FEED_ID, payload)

  # Redstone allows prices ~10 seconds old, discourage replay attacks
  assert ts >= self.TIMESTAMP, "ERR_ORACLE"
  self.TIMESTAMP = ts

  # price is quote per unit base, convert to same precision as quote
  pd   : uint256 = self.DECIMALS
  qd   : uint256 = quote_decimals
  s    : bool    = pd >= qd
  n    : uint256 = pd - qd if s else qd - pd
  m    : uint256 = 10 ** n
  p    : uint256 = price / m if s else price * m
  return p

Impact

During a de-peg event, LPs can withdraw more value than they deposited, causing significant losses to the protocol.

Attack Path

  1. Deposit: Attacker deposits 1 BTC and 50,000 USDT when 1 BTC = 50,000 USD.
  2. De-peg Event: The pegged token (USDT) de-pegs to 0.70 USD.
  3. Withdraw: Attacker withdraws their funds, exploiting the incorrect assumption that 1 USDT = 1 USD.

Proof of Concept (PoC)

  1. Deposit:
@external
def mint(
  base_token  : address, #ERC20
  quote_token : address, #ERC20
  lp_token    : address, #ERC20Plus
  base_amt    : uint256,
  quote_amt   : uint256,
  desired     : uint256,
  slippage    : uint256,
  payload     : Bytes[224]
) -> uint256:
  """
  @notice            Provide liquidity to the pool
  @param base_token  Token representing the base coin of the pool (e.g. BTC)
  @param quote_token Token representing the quote coin of the pool (e.g. USDT)
  @param lp_token    Token representing shares of the pool's liquidity
  @param base_amt    Number of base tokens to provide
  @param quote_amt   Number of quote tokens to provide
  @param desired     Price to provide liquidity at (unit price using onchain
                     representation for quote_token, e.g. 1.50$ would be
                     1500000 for USDT with 6 decimals)
  @param slippage    Acceptable deviaton of oracle price from desired price
                     (same units as desired e.g. to allow 5 cents of slippage,
                     send 50000).
  @param payload     Signed Redstone oracle payload
  """
  ctx: Ctx = self.CONTEXT(base_token, quote_token, desired, slippage, payload)
  return self.CORE.mint(1, base_token, quote_token, lp_token, base_amt, quote_amt, ctx)
  1. De-peg Event: The pegged token de-pegs to 0.70 USD (external event).

  2. Withdraw:

def burn(
  base_token  : address,
  quote_token : address,
  lp_token    : address,
  lp_amt      : uint256,
  desired     : uint256,
  slippage    : uint256,
  payload     : Bytes[224]
) -> Tokens:
  """
  @notice            Withdraw liquidity from the pool
  @param base_token  Token representing the base coin of the pool (e.g. BTC)
  @param quote_token Token representing the quote coin of the pool (e.g. USDT)
  @param lp_token    Token representing shares of the pool's liquidity
  @param lp_amt      Number of LP tokens to burn
  @param desired     Price to provide liquidity at (unit price using onchain
                     representation for quote_token, e.g. 1.50$ would be
                     1500000 for USDT with 6 decimals)
  @param slippage    Acceptable deviaton of oracle price from desired price
                     (same units as desired e.g. to allow 5 cents of slippage,
                     send 50000).
  @param payload     Signed Redstone oracle payload
  """
  ctx: Ctx = self.CONTEXT(base_token, quote_token, desired, slippage, payload)
  return self.CORE.burn(1, base_token, quote_token, lp_token, lp_amt, ctx)
  1. Incorrect Price Calculation:
def CONTEXT(
    base_token : address,
    quote_token: address,
    desired    : uint256,
    slippage   : uint256,
    payload    : Bytes[224]
) -> Ctx:
  base_decimals : uint256 = convert(ERC20Plus(base_token).decimals(), uint256)
  quote_decimals: uint256 = convert(ERC20Plus(quote_token).decimals(), uint256)
  # this will revert on error
  price         : uint256 = self.ORACLE.price(quote_decimals,
                                              desired,
                                              slippage,
                                              payload)
  return Ctx({
    price         : price,
    base_decimals : base_decimals,
    quote_decimals: quote_decimals,
  })

Mitigation

To mitigate this issue, the protocol should use the <base-token>/<quote-token> price directly if available, or derive it from the <base-token>/USD and <quote-token>/USD prices. This ensures accurate valuations even if the quote token de-pegs from USD.

@github-actions github-actions bot added the Excluded Excluded by the judge without consulting the protocol or the senior label Sep 11, 2024
@sherlock-admin3 sherlock-admin3 changed the title Brilliant Burlap Elephant - LPs will withdraw more value than deposited during pegged token de-peg events 4gontuk - LPs will withdraw more value than deposited during pegged token de-peg events Sep 11, 2024
@sherlock-admin3 sherlock-admin3 added the Non-Reward This issue will not receive a payout label Sep 11, 2024
@mePopye
Copy link

mePopye commented Sep 12, 2024

Escalate

On behalf of the watson

@sherlock-admin3
Copy link
Contributor

sherlock-admin3 commented Sep 12, 2024

Escalate

On behalf of the watson

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.

@sherlock-admin4 sherlock-admin4 added the Escalated This issue contains a pending escalation label Sep 12, 2024
@WangSecurity
Copy link

After additionally considering this issue, here's my understanding. Let's assume a scenario of 30% depeg and USDT = 0.7 USD.

  1. The pool has 10 BTC and 500k USDT.
  2. User deposits 1 BTC and 50k USDT, assuming 1 BTC = 50k USDT = 50k USD.
  3. USDT depegs to 0.7 USD, i.e. 1 USDT = 0.7 USD. Then BTC = 50k USD = ~71.5k USDT.
  4. The user withdraws 1 BTC and 50k USDT. They receive it because the code still considers 1 USDT = 1 USD. There are 10 BTC and 500k USDT left in the contracts.
  5. The protocol didn't lose any funds, the amount of BTC and USDT remained the same as it was before the depeg.
  6. But, in reality, the user has withdrawn 50k worth of BTC and 35k worth of USDT since 1 USDT = 0.7 USD.
  7. Hence, if the protocol accounted for the depeg, there had to be 10 BTC and 515k USDT left in the contract after the user had withdrawn during the depeg.

Hence, even though it's not a direct loss of funds but a loss in value, this should be a valid medium (considering depeg as an extensive limitation). Thus, planning to accept the escalation and validate with medium severity. The duplicate is #113, are there any additional duplicates?

@WangSecurity WangSecurity added Has Duplicates A valid issue with 1+ other issues describing the same vulnerability Medium A Medium severity issue. and removed Excluded Excluded by the judge without consulting the protocol or the senior labels Oct 2, 2024
@sherlock-admin2 sherlock-admin2 added Reward A payout will be made for this issue and removed Non-Reward This issue will not receive a payout labels Oct 2, 2024
@WangSecurity
Copy link

WangSecurity commented Oct 2, 2024

Result:
Medium
Has duplicates

@WangSecurity WangSecurity reopened this Oct 2, 2024
@sherlock-admin2
Copy link
Contributor Author

sherlock-admin2 commented Oct 2, 2024

Escalations have been resolved successfully!

Escalation status:

@sherlock-admin3 sherlock-admin3 removed the Escalated This issue contains a pending escalation label Oct 2, 2024
@sherlock-admin4 sherlock-admin4 added the Escalation Resolved This issue's escalations have been approved/rejected label Oct 2, 2024
@sherlock-admin3 sherlock-admin3 added the Sponsor Disputed The sponsor disputed this issue's validity label Nov 15, 2024
@sherlock-admin3 sherlock-admin3 added the Won't Fix The sponsor confirmed this issue will not be fixed label Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Escalation Resolved This issue's escalations have been approved/rejected Has Duplicates A valid issue with 1+ other issues describing the same vulnerability Medium A Medium severity issue. Reward A payout will be made for this issue Sponsor Disputed The sponsor disputed this issue's validity Won't Fix The sponsor confirmed this issue will not be fixed
Projects
None yet
Development

No branches or pull requests

5 participants