ubiquity / ubiquity-dollar

Ubiquity Dollar (uAD) smart contracts and user interface.
https://uad.ubq.fi
Apache License 2.0
31 stars 82 forks source link

Pairing Dollar with 3CRV will still make Dollar follow a price of $1.03, even if StableSwapMetaNG pools are used #931

Closed 0xadrii closed 2 months ago

0xadrii commented 2 months ago
          Pairing Dollar with the tripool will still cause [this issue](https://github.com/sherlock-audit/2023-12-ubiquity-judging/issues/15), even if the pool used is a StableSwapMetaNG and not the old curve pools. 

Recommendation: The best solution to achieve an actual peg to $1 would be to use Chainlink price feeds instead. or to pair Dollar with a different asset that is actually pegged to $1 rather than 3CRV, given that currently the pool price will tend to follow 3CRV's price, which currently trades at $1.03.

_Originally posted by @0xadrii in https://github.com/ubiquity/ubiquity-dollar/pull/893#discussion_r1572711362_

rndquu commented 2 months ago

Check the following Curve's metapools:

All of them contain a pair of a USD pegged stablecoin + 3CRV LP.

The price_oracle() method returns ~$1.00 for all of the pools. Shouldn't it be ~$0.97 if 3CRV is $1.03 ?

0xadrii commented 2 months ago

Yeah I checked them. So StableSwap Meta NG pools will initially hardcode prices at 1 for both assets in the pool. This can be seen in this line of code inside the ng metapool's __init__() function:

def __init__(
    ...
):
  self.last_prices_packed = self.pack_2(10**18, 10**18)
...

This is why pools will always return a price of 1, even if they are empty.

Every time an interaction that updates the oracle (exchange/add_liquidity_remove_liquidity...) with the pool takes place, the pool invariant will be computed with a combination of the amplifier, the pool's balances and the _stored_rates. For example, add_liquidity will fetch amp, old_balances and rates to compute the invariant D0:

def add_liquidity(
    _amounts: uint256[N_COINS],
    _min_mint_amount: uint256,
    _receiver: address = msg.sender
) -> uint256:
  ...

    amp: uint256 = self._A()
    old_balances: uint256[N_COINS] = self._balances()
    rates: uint256[N_COINS] = self._stored_rates()

    # Initial invariant
    D0: uint256 = self.get_D_mem(rates, old_balances, amp)

  ...

The _stored_rates is a crucial value in the pool. It consists in an array of two positions, where each position will be multiplied by the balances of each token to derive xp (this can be seen in the _xp_mem function). xp is crucial for oracles because it is later used to update them via upkeep_oracles:

def add_liquidity(
    _amounts: uint256[N_COINS],
    _min_mint_amount: uint256,
    _receiver: address = msg.sender
) -> uint256:
  ...
        xp: uint256[N_COINS] = self._xp_mem(rates, new_balances)
        ...
        # we do unsafe div here because we already did several safedivs with D0
        mint_amount = unsafe_div(total_supply * (D1 - D0), D0)
        self.upkeep_oracles(xp, amp, D1)

Knowing that _stored_rates is important, we need to know what are the values for our specific pool. For metapools (case of Dollar/3CRV), the value of _stored_rates will simply be fetched like this:

def _stored_rates() -> uint256[N_COINS]:
    """
    @notice Gets rate multipliers for each coin.
    @dev If the coin has a rate oracle that has been properly initialised,
         this method queries that rate by static-calling an external
         contract.
    """
    rates: uint256[N_COINS] = [rate_multiplier, StableSwap(BASE_POOL).get_virtual_price()]
   ...

As we can see, rates[0] (the rate multiplied by the Dollar's pool balance) is harcoded to rate_multiplier (which is 10**18, as hardcoded in the stableswap factory). However, as shown in the previous code snippet, the rate for 3CRV (rates[1], which is multiplied by the 3CRV balance currently held in the pool) is set by calling the 3CRV'S get_virtual_price function, which returns 1031791970976582816, or $1.03 (you can check it here). Fetching the rate from virtual price is hardcoded, meaning that Curve assumes that pools trading with 3CRV (or any other base pool) won't be considered to be pegged to $1.

This means that every time the pool invariant or xp is computed, rates will use the 3CRV's virtual price (not set at $1) to derive the ideal state of the pool. As per the prices in the pools you showed, probably they are still set at prices near 1 because there haven't been many trades on them, but if they were to be traded frequently, price should be encouraged to follow 3CRV's price.

rndquu commented 2 months ago

@0xadrii

If we move to CurveStableSwapNG then is it ok to use collateral tokens from LibUbiquityPool? So we'll: 1) Use LUSD as collateral in LibUbiquityPool 2) Create a new CurveStableSwapNG pool with Dollar/LUSD pair

Or it is better to create Dollar/SUSD pool for fetching USD quote?

0xadrii commented 2 months ago

I wouldn't create the pool with the same collateral tokens. I'd rather go with USDC, but if you want to avoid centralized stables then sUSD is also a good option. The main problem here is you need to have a 100% trust that the stablecoin you are pairing with will keep its peg, and become 100% reliant on it. I would query a Chainlink oracle to check sUSD's price as well, and compare it after querying the curve pool price to properly appraise Dollar

rndquu commented 2 months ago

I would query a Chainlink oracle to check sUSD's price as well, and compare it after querying the curve pool price to properly appraise Dollar

You mean:

  1. Fetch SUSD/USD from chainlink
  2. Fetch Dollar/SUSD from curve
  3. If both values are in the same threshold (like in 10%) then proceed with mint and redeems

Right?

0xadrii commented 2 months ago

You don't really need to verify that they're in the same threshold, but rather extract the $ value for sUSD from the response received by Chainlink.

For example, let's say the curve pool returns 1 Dollar = 1.01 sUSD.

However, querying chainlink returns 1 sUSD = $0.99.

This will mean that Dollar is not worth $1.01, but $0.9999 (extracted from the multiplication between sUSD and CL price, which is 1.01*0.99).

rndquu commented 2 months ago

You don't really need to verify that they're in the same threshold, but rather extract the $ value for sUSD from the response received by Chainlink.

For example, let's say the curve pool returns 1 Dollar = 1.01 sUSD.

However, querying chainlink returns 1 sUSD = $0.99.

This will mean that Dollar is not worth $1.01, but $0.9999 (extracted from the multiplication between sUSD and CL price, which is 1.01*0.99).

As far as I understand if we multiply chainlink price by curve price we get more actual price compared to the price from Curve's oracle. Is it ok if right now we'll simply use the Curve's oracle price and add "chainlink multiplication" price later?

We have an epic of using multiple oracles for fetching prices and this "chainlink multiplication" fits into that issue.

0xadrii commented 2 months ago

As far as I understand if we multiply chainlink price by curve price we get more actual price compared to the price from Curve's oracle

Hmm, do you have an example of such a case where more price was obtained?

We have an epic of using multiple oracles for fetching prices and this "chainlink multiplication" fits into that issue.

Yes, I think that for the initial phase of the protocol that should do, it could be added later although highly recommend to add it in the beginning.

rndquu commented 2 months ago

You don't really need to verify that they're in the same threshold, but rather extract the $ value for sUSD from the response received by Chainlink.

For example, let's say the curve pool returns 1 Dollar = 1.01 sUSD.

However, querying chainlink returns 1 sUSD = $0.99.

This will mean that Dollar is not worth $1.01, but $0.9999 (extracted from the multiplication between sUSD and CL price, which is 1.01*0.99).

I got what you mean. Yes, you're right, so if SUSD depegs then: 1) Chainlink's SUSD/USD returns $0.9 (for example) 2) Curve's Dollar/SUSD price oracle returns ~$1.11 3) So the Dollar price is 0.9 * 1.11 = ~0.99

@0x4007 Is it ok if we create a new Curve's pool with Dollar/SUSD pair to use it for fetching Dollar token price in USD?

I read the spec. Why not just use LUSD instead of sUSD?

rndquu commented 2 months ago

@0xadrii

One more question regarding price oracle indexes, just want to make it clear. So we need to: 1) Create a new CurveStableSwapNG pool with 2 coins (index 0: SUSD, index 1: Dollar token) 2) Call StableSwap.price_oracle(0) which will return the Dollar/SUSD quote

Correct?

0xadrii commented 2 months ago

Yes, that is correct! Just make sure index 0 will be set to sUSD and index 1 to Dollar when creating the pool

rndquu commented 2 months ago

It seems there is no SUSD/USD price feed, there is this one https://etherscan.io/address/0xad35bd71b9afe6e4bdc266b345c198eadef9ad94 but it is not live.

I guess we'll have to fetch SUSD/ETH (which does have a live price feed) and then ETH/USD to get a SUSD/USD quote.

0xadrii commented 2 months ago

True, I checked it wrong and thought the feed was for USD. In that case yes, that's the only solution I believe.

0x4007 commented 2 months ago

You don't really need to verify that they're in the same threshold, but rather extract the $ value for sUSD from the response received by Chainlink.

For example, let's say the curve pool returns 1 Dollar = 1.01 sUSD.

However, querying chainlink returns 1 sUSD = $0.99.

This will mean that Dollar is not worth $1.01, but $0.9999 (extracted from the multiplication between sUSD and CL price, which is 1.01*0.99).

I got what you mean. Yes, you're right, so if SUSD depegs then:

1) Chainlink's SUSD/USD returns $0.9 (for example)

2) Curve's Dollar/SUSD price oracle returns ~$1.11

3) So the Dollar price is 0.9 * 1.11 = ~0.99

@0x4007 Is it ok if we create a new Curve's pool with Dollar/SUSD pair to use it for fetching Dollar token price in USD?

Why are we concerned with implementing sUSD features if it's not even planned for the mid term? I expected we can get far with only LUSD.

rndquu commented 2 months ago

You don't really need to verify that they're in the same threshold, but rather extract the $ value for sUSD from the response received by Chainlink.

For example, let's say the curve pool returns 1 Dollar = 1.01 sUSD.

However, querying chainlink returns 1 sUSD = $0.99.

This will mean that Dollar is not worth $1.01, but $0.9999 (extracted from the multiplication between sUSD and CL price, which is 1.01*0.99).

I got what you mean. Yes, you're right, so if SUSD depegs then:

  1. Chainlink's SUSD/USD returns $0.9 (for example)
  2. Curve's Dollar/SUSD price oracle returns ~$1.11
  3. So the Dollar price is 0.9 * 1.11 = ~0.99

@0x4007 Is it ok if we create a new Curve's pool with Dollar/SUSD pair to use it for fetching Dollar token price in USD?

Why are we concerned with implementing sUSD features if it's not even planned for the mid term? I expected we can get far with only LUSD.

@0x4007 We want to move from Dollar/3CRV Curve's pool to either Dollar/LUSD either to Dollar/SUSD because there is a difference in price oracles between Curve's metapools (i.e. Dollar/3CRV) and plain pools (i.e. Dollar/LUSD). I just want you to confirm this shift.

@0xadrii Back to this question, as far as I understand we could use LUSD as collateral for LibUbiquityPool in the meantime fetching Dollar price in USD from Curve's LUSD/Dollar pool. The only concern, as you've already mentioned, is a 100% trust in a stablecoin used as a pair for the Dollar token. So overall we could use LUSD both as collateral and in the Curve's LUSD/Dollar pool for fetching Dollar/USD quote, right?

0xadrii commented 2 months ago

The only concern, as you've already https://github.com/ubiquity/ubiquity-dollar/issues/931#issuecomment-2067697635, is a 100% trust in a stablecoin used as a pair for the Dollar token.

Exactly, if you're willing to take the risk and only consider LUSD initially then the mechanics should work as if the collateral token was different or if the pool was paired with a different token. Still, the Chainlink check should be done to ensure the actual $ price is correct.

0x4007 commented 2 months ago

to either Dollar/LUSD either to Dollar/SUSD because there is a difference in price oracles between Curve's metapools (i.e. Dollar/3CRV) and plain pools (i.e. Dollar/LUSD). I just want you to confirm this shift.

Your sentence is not clear. Using Dollar/LUSD is fine. We do not need to use 3CRV for launch. I wouldn't worry about sUSD at this stage.

The benefit of using 3CRV is that we have better liquidity for the other popular stablecoins. But if we control the liquidity then in essence it is a form of our collateral. It would be more "pure" to only support Dollar/LUSD but for better swap rates for most users (who hold USDT/USDC/DAI) it would be nice to support 3CRV some day.

ubiquibot[bot] commented 2 months ago
! action has an uncaught error