search

sherlock-audit / 2023-12-ubiquity-judging

2 stars 2 forks source link

cergyk - UbiquityPool::redeemDollar DoS on redeeming if USDT/USDC depegs #59

Closed sherlock-admin closed 7 months ago

sherlock-admin commented 8 months ago

cergyk

medium

UbiquityPool::redeemDollar DoS on redeeming if USDT/USDC depegs

Summary

uAD is algorithmically dependent on its accepted collateral, which are supposed to be LUSD and DAI at first. However there is a mechanism blocking withdrawal in case the TWAP price of uAD in the metapool uAD/3CRV is too high. Since 3CRV also contains USDT and USDC, in case any of USDC or USDT depegging, 3CRV price against uAD will be lower. Redeeming uAD may end up temporarily locked.

Vulnerability Detail

We can see that calling redeemDollar reverts if the price of uAD is above a given threshold: https://github.com/sherlock-audit/2023-12-ubiquity/blob/main/ubiquity-dollar/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol#L418-L420

This means that an event which causes the price of 3CRV to be below the threshold such as USDT or USDC depegging will cause redeeming to revert effectiely causing a DOS on withdrawals.

Impact

Temporary DOS on withdrawal because of a depeg of USDT/USDC which should be unrelated.

Code Snippet

https://github.com/sherlock-audit/2023-12-ubiquity/blob/main/ubiquity-dollar/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol#L418-L420

Tool used

Manual Review

Recommendation

Remove the reliance on 3CRV, create a tricrypto pool which contains collateral tokens

sherlock-admin2 commented 8 months ago

1 comment(s) were left on this issue during the judging contest.

auditsea commented:

The issue describes about the protocol insolvancy in case of collateral depeg. It's not avoidable, that's why the protocol has borrowing function to get yield, take fees on mint and redeem, these features will hedge the risk from protocol insolvancy

sherlock-admin2 commented 8 months ago

1 comment(s) were left on this issue during the judging contest.

auditsea commented:

The issue describes about the protocol insolvancy in case of collateral depeg. It's not avoidable, that's why the protocol has borrowing function to get yield, take fees on mint and redeem, these features will hedge the risk from protocol insolvancy

nevillehuang commented 8 months ago

See also #61 for a coded PoC, both talking about UAD price reliant on 3CRV

rndquu commented 8 months ago

Ok, we've already discussed it here so the issue seems to be valid and "will fix".

Not sure about the possible solution, but the following seem worth to check:

0x3agle commented 7 months ago

Issue #221 is incorrectly set as a duplicate of this one.

This issue is invalid:

osmanozdemir1 commented 7 months ago

Escalate

  1. I wanted to use my escalation for the comment above.
  2. Even if this issue is valid and the above comment is wrong, this issue should be a medium not high since it requires a major external factor and the Dos is temporary.
sherlock-admin2 commented 7 months ago

Escalate

  1. I wanted to use my escalation for the comment above.
  2. Even if this issue is valid and the above comment is wrong, this issue should be a medium not high since it requires a major external factor and the Dos is temporary.

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.

0xArz commented 7 months ago

Escalate

  1. I wanted to use my escalation for the comment above.
  2. Even if this issue is valid and the above comment is wrong, this issue should be a medium not high since it requires a major external factor and the Dos is temporary.

What is the major external factor if 3CRV is already depegged and its not exactly $1.00 but $1.03? Correct me if im wrong but when the pool is created and the balances are equal the price of uAD will be $1.03 because the price of 3CRV is $1.03, this will cause the pool to be rebalanced so that for 1 uAD you get 0.97 3CRV and the price of uAD is actually $1.00 not $1.03. The twap will then return 0.97 and minting will be DoSed

osmanozdemir1 commented 7 months ago

External factor is something else being depegged. “already” doesn’t mean anything.

There is no issue without an external problem.

0x3agle commented 7 months ago

@0xArz You are correct.

The uAD is backed by a token with value of $1.03. And the UbiquityPool considers that uAD is $1.

This discrepancy is really not ideal for a stable-coin which is what was pointed out in #221

0xArz commented 7 months ago

@0x3agle Yep i also pointed it out in https://github.com/sherlock-audit/2023-12-ubiquity-judging/issues/165. I agree that uAD not being pegged to $1.00 because of 3CRV is different from USDT depegging.

ydspa commented 7 months ago

Ok, we've already discussed it here so the issue seems to be valid and "will fix".

Not sure about the possible solution, but the following seem worth to check:

* [0xpiken - Incorrect value was used to represent USD price of Ubiquity Dollar #30](https://github.com/sherlock-audit/2023-12-ubiquity-judging/issues/30)

* [KingNFT - The formula used in `getDollarPriceUsd()` is incorrect #61](https://github.com/sherlock-audit/2023-12-ubiquity-judging/issues/61)

* [evmboi32 - Dollar token won't be pegged to 1 USD. #141](https://github.com/sherlock-audit/2023-12-ubiquity-judging/issues/141)

* [Arz - The TWAP oracle uses 3CRV which is not exactly pegged to $1.00 #165](https://github.com/sherlock-audit/2023-12-ubiquity-judging/issues/165)

* [fugazzi - TWAP oracle returns incorrect price #192](https://github.com/sherlock-audit/2023-12-ubiquity-judging/issues/192)

1) this group of issues are not duplicates of the primary issue, they should be judged separately 2) USDT/USDC depeg risk itself was judged as low in previous contests, e.g. https://github.com/sherlock-audit/2023-07-perennial-judging/issues/97

nevillehuang commented 7 months ago

@CergyK any comments?

evmboi32 commented 7 months ago

@0x3agle Yes I pointed out the same issue in #141. The uAD would not be pegged to $1 as it is backed by a token worth $1.03.

0xLogos commented 7 months ago

I think issues about incorrect peg are invalid (all dubs excluding this one) TWAP oracle uses reserves ratio of uAD/3CRV and yes, it will be slightly incorrect because amount of uAD in pool is greater than 3CRV when uAD is worth exactly 1$. But this oracle only used in mint/redeem to check against admin adjustable threasholds and revert if TWAP above/below threashold. This is by no means "backing" or "pegging". Also I want to mention that issue with arbitrary short TWAP window (critical for oracle) is only medium and this is fair as it turned out that it is not too important in the context of contest.

This issue also invalid as I stated that threasholds are adjustable. Plus you can think of it as issue about of arbitrary short TWAP window (medium) with external condition USDT depegs.

0x3agle commented 7 months ago

@0xLogos Check #221, the devs confirmed that uAD being pegged to $1.03 is an issue.

The issue lies in the fact that Ubiquity pool values uAD at $1 and in the curve pool, the uAD is pegged to a value of $1.03

In terms of stable-coin a slight issue, is an issue because we are not talking about ETH where +/- of $0.03 won't matter.

0xLogos commented 7 months ago

I don't see from #221 why it's pegged to 3CRV

Therefore, from the beginning, the uAD is pegged to $1.03 instead of $1. uAD is pegged to $1.03 instead of $1 during deployment.

It's just initial setup, later pool will balance itself like lusd metapool.

I don't want to argue, so let's leave it for the judges. My point is that this issue and all dubs are invalid. (I should had to escalate it myself, but I didn't manage my time)

CergyK commented 7 months ago

@CergyK any comments?

Would rate it as medium, since depegging of a major stable coin is a rare event. The Dos would be temporary until admin changes the pool address by calling setPool

0xLogos commented 7 months ago

The Dos would be temporary until admin changes the pool address by calling setPool

No need to change pool (deploy new), just adjust mint/redeem thresholds.

Also "Temporary DOS on withdrawal" (from impact) is not really impact as you can only redeem when dollar twap < 0.99$ because of threshold. Redeeming wasn't meant to be always available.

Czar102 commented 7 months ago

It seems USDT/USDC are the measures of value here, and in the event a measure is failing, another measure needs to be set (by the admin), which implies that mint/redeem thresholds need to be updated. There is an ability to react to such an event, so I'm not sure what is the vulnerability severe enough to be considered valid here.

nevillehuang commented 7 months ago

@Czar102 see #61 or #221 for a possible more correct impact. I believe UAD is not supposed to be pegged to 3CRV with current prices of 1.03.

Czar102 commented 7 months ago

I see. In that case, I think this report should be invalidated, and duplicates should be a valid Med (uAD is denominated differently than it aims to be).

0xLogos commented 7 months ago

I believe all dups about incorrect peg should be info

61 described impact is just user mistake/choice

221 also lacks of real impact, just info

Only problem with TWAP returing price slightly below 1$ is that planned initial mint/redeem thresholds are incorrect and will not work as intended. But once adjusted all will be ok.

I agree this is maybe suboptimal solution, but I believe it will work.

Additionally recomendations in 61 is to use spot price and in 221 is rather big design change than meaningful bugfix.

Czar102 commented 7 months ago

I think #61 and #221 are valid.

@nevillehuang could you review if #15, #30, #148 and #192 are correctly judged duplicates of this issue? I think especially the last one may be invalid since it's observing on-chain data?

nevillehuang commented 7 months ago

@Czar102 I believe all are valid issues, even #192, which correctly identifies the root cause, albeit not explicitly mentioning 3CRV

The implementation of the TWAP oracle is using the get_dy() function, which calculate the output (dy) of a swap, but since this is a metapool this amount will be expressed in terms of the LP token, which is not the price of the underlying tokens in the base pool (remember the LP token has the accumulated fees).

This means that the uAD oracle will return the price in terms of the LP token, which will be significantly lower than the real correct price.

0xLogos commented 7 months ago

@Czar102 Could you please take a look

TWAP oracle returns the 3CRV price of per uAD, but 3CRV is only measure of value. This oracle only used in mint/redeem to check against admin adjustable threasholds and revert if TWAP above/below threashold. So only requirements for reference value is peg to some usd amount.

So there's only 2 minor issues

  1. Devs said initial thresholds would be 1.01 and 0.99, but they are inaccurate for 1$ peg. But nothing bad happend, once devs noticed that uAD peg above 1$ they will just adjust thresholds, no need to change all design.
  2. 3CRV price increasing over time (<1 cent per year) so thresholds need to be adjusted maybe once a half of year or so

For auditor looking to code it seems by design because TWAP take advantage of time average balances tracked by Curve pool, you can't fix it just by using get_virtual_price as recomended in #61. It really seems like suboptimal, but working desing.

Thanks

Czar102 commented 7 months ago

@0xLogos but the uAD denomination would change from my understanding – 1 uAD would be e.g. $1.05 instead of $1. There is no direct loss of funds, but core functionality is impacted.

0xLogos commented 7 months ago

It wouldn't. TWAP + thresholds only determine when user can mint/redeem uAD.

When pool twap > mint threshold uAD above peg and users can mint X uAD providing X$ equivalent according to chainlink price amount of collateral. Since uAD above beg X uAD > X$ and users take profit by selling uAD on external markets and eventually lowering twap.

So once redeem/mint open peg target is 1$ according to chailink oracle.

ydspa commented 7 months ago

@0xLogos but the uAD denomination would change from my understanding – 1 uAD would be e.g. $1.05 instead of $1. There is no direct loss of funds, but core functionality is impacted.

Correct, actually the contract would be bricked at the current mainnet environment, as the mint() would always revert due to incorrect return of getDollarPriceUsd().

Furthermore, the front end and users would rely on getDollarPriceUsd() to get uAD/USD price (we can expect there is no chainlink uAD/USD feed in short term), and users might be misled to suffer loss. e.g. if the true uAD/USD is $1.05 but reported $1, users should sell on market instead of redeem().

ydspa commented 7 months ago

@Czar102 I believe all are valid issues, even #192, which correctly identifies the root cause, albeit not explicitly mentioning 3CRV

A vague touch of the issue with low quality reports should be invalid dups. e.g. https://github.com/sherlock-audit/2023-09-Gitcoin-judging/issues/458 there are hundreds submissions identifying the root causes but only some of them are valid.

Czar102 commented 7 months ago

@0xLogos thank you for this argument. Since the issue can be easily remedied by changing the thresholds and doesn't cause any loss of funds, I think it's fair to consider it a low severity issue.

nevillehuang commented 7 months ago

@Czar102 Why would it be a good idea/acceptable to allow adjustment of price thresholds further away from the intended narrow range of 0.99-1.01? TWAP is the only representation of uAD price, isn’t that extremely important to the protocol?

Czar102 commented 7 months ago

TWAP is the only representation of uAD price, isn’t that extremely important to the protocol?

That has been the discussion, and from my understanding TWAP is only used for a sanity check of mint/redeem thresholds. And increasing thresholds would just counter the 3CRV token price increase to make the thresholds denominated in uAD 0.99-1.01.

That's my current understanding of this issue.

nevillehuang commented 7 months ago

@Czar102 @osmanozdemir1 @CergyK

Since uAD are always minted 1:1 to collateral value, and TWAP is the only representation of price with secondary markets being expected based on sponsors comments for arbitrage purposes, consider this scenario:

  1. Sell uAD at $1.03 (since it is pegged to 3CRV at current prices of $1.03) on secondary markets
  2. Buy more collateral
  3. Mint uAD with inflated collateral. Profit because of inaccurate prices (when they are not supposed to, given price should be reflected as $1) and unnecessarily inflate uAD supply.
  4. Repeat steps 1 to 3, since uAD price will never be accurately reflected given its dependency on 3CRV

It is crucial for threshold checks to be within protocols desired price ranges to reflect and keep accurate uAD prices

sherlock-admin commented 7 months ago

The protocol team fixed this issue in PR/commit https://github.com/ubiquity/ubiquity-dollar/pull/893.

0xArz commented 7 months ago

3CRV is quite volatile and the wrong thresholds will prevent arbitrageurs from stablizing the stablecoin which is the whole point of this UbiquityPool.

The curve pool will be launched with 50:50 reserves(there is a check in the twap), this means that the price of uAD will be `$1.03. Because uAD is supposed to be a stablecoin the curve pool should get rebalanced so that for 1 3CRV you get 0.97 uAD. To do this you would need to sell uAD although no one will not be able to mint uAD from the UbiquityPool because of the thresholds and the TWAP returns 1 because the reserves are balanced. The admin would then have to adjust the thresholds - 0.96 for redeeming and 0.98 for minting. If the twap returns 0.97 then the reserves are balances so that the value of both reserves is 1$.

Now lets say if the price of 3CRV goes to $1.04, the curve pool will need to be rebalanced again by minting uAD which will not be possible because of the thresholds which need to be adjusted again.

If the price of 3CRV goes to $1.02 then the abitrage bot will need to buy uAD and then redeem it for collateral but again because of the wrong threshold the arbitrage bot will not be able to redeem.

ydspa commented 7 months ago

TWAP is the only representation of uAD price, isn’t that extremely important to the protocol?

That has been the discussion, and from my understanding TWAP is only used for a sanity check of mint/redeem thresholds. And increasing thresholds would just counter the 3CRV token price increase to make the thresholds denominated in uAD 0.99-1.01.

That's my current understanding of this issue.

It's absolutely not just a simple sanity check, but works as an important protection for uAD stability. As mentioned by @nevillehuang, since uAD are always minted/redeemed 1:1 to collateral ($LUSD). Let's say, at initial market the LUSD = uAD = $1, then if price of LUSD drift to $0.95, then users can mint lots of uAD and sell on the uAD/3CRV pool to get profit, and finally make price uAD drift to $0.95 too. But while the protection works, no more uAD could be minted while uAD price drifts to $0.99. Therefore, we can't simply adjust the mint/redeem threshold to something like 0.96 to solve this problem, doing this would make the security design barely works, as my view, the current 0.99/1.01 threshold is almost the max acceptable config for a stable coin, so a correct getDollarPriceUsd() is important.

0xLogos commented 7 months ago

from my understanding TWAP is only used for a sanity check of mint/redeem thresholds. And increasing thresholds would just counter the 3CRV token price increase to make the thresholds denominated in uAD 0.99-1.01.

Yeap, that's exactly what I wanted to say

3CRV is quite volatile...

First of all, it's different issue. Second I believe it's more suboptimal choice rather than vulnerability bcz you can't say (take into account all market forces) for sure how it will go until deployed. If it's inefficient, nothing bad will happen, just new solution has to be found.

TWAP is the only representation of uAD price

Price is what you pay when buy. TWAP is market indicator and used for special purposes. Maybe it was intended to be used in frontend or something else too, but it's out of scope. In contracts it's only used for (sanity) check.

0x3agle commented 7 months ago

I don't understand why the validity is still being argued upon given that the protocol team confirmed the issue and has already created a PR to fix it.

Czar102 commented 7 months ago

Let's say, at initial market the LUSD = uAD = $1, then if price of LUSD drift to $0.95, then users can mint lots of uAD and sell on the uAD/3CRV pool to get profit, and finally make price uAD drift to $0.95 too.

@ydspa from my understanding, they would need to pay a little more than 1.05 LUSD for every uAD (Chainlink oracle), so the price of uAD would be maintained.

I'm still convinced by @0xLogos's arguments. The impact of changed TWAP due to the initial offset or accruing fees can be easily countered. Planning to consider this a low severity issue.

ydspa commented 7 months ago

@ydspa from my understanding, they would need to pay a little more than 1.05 LUSD for every uAD (Chainlink oracle), so the price of uAD would be maintained.

Nope, uAD always 1:1 minted by LUSD, this is why the threshold protection existing as LUSD might drift from $1, and the protection strop uAD from drifting together with LUSD.

I'm still convinced by @0xLogos's arguments. The impact of changed TWAP due to the initial offset or accruing fees can be easily countered. Planning to consider this a low severity issue.

how to countered, set the mint threshold to 0.96? and uAD has no chainlink oracle, getDollarPriceUsd() is the only representation of uAD price for users and front end. Just this point, it's a break of core feature.

It is also not just a initial offset or accruing fees problem, the difference with real price is dynamic due to uses' action on curve 3pool, such as unbalance of USDC/USDT/DAI in 3pool

0xArz commented 7 months ago

@Czar102 The initial offset might work but this still doesnt solve the issue that uAD is pegged to 3CRV which is not a stablecoin and is volatile as you can see from the charts. Please read my comment again here. Because of the wrong thresholds arbitrage will not be possible and this will prevent uAD from stabilizing which is the whole point of the UbiquityPool and its crucial. Adjusting the thresholds manually will be too slow and problematic when the volatility is high

detectiveking123 commented 7 months ago

I have very little context here, but any issue that relies on an established stable-coin depegging should be a low.

Czar102 commented 7 months ago

When the thresholds are wrong, setting them to proper ones doesn't cause any loss of funds to anyone. The price may be temporarily off, but as soon as the thresholds are changed, it should stabilize without loss of funds.

ubiquibot[bot] commented 7 months ago 
! No price label has been set. Skipping permit generation.
Czar102 commented 7 months ago

Result: Low Has duplicates

sherlock-admin2 commented 7 months ago

Escalations have been resolved successfully!

Escalation status:

sherlock-admin commented 6 months ago

The Lead Senior Watson signed off on the fix.