Skip to content
This repository has been archived by the owner on Jul 14, 2024. It is now read-only.

cergyk - UbiquityPool::mintDollar/redeemDollar collateral depeg will encourage using UbiquityPool to swap for better collateral #17

Open
sherlock-admin opened this issue Jan 10, 2024 · 25 comments
Labels
Disagree With Severity The sponsor disputed the severity of this issue 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 valid 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-admin
Copy link
Contributor

sherlock-admin commented Jan 10, 2024

cergyk

high

UbiquityPool::mintDollar/redeemDollar collateral depeg will encourage using UbiquityPool to swap for better collateral

Summary

In the case of a depeg of an underlying collateral, UbiquityPool mechanism incentivises users to fill it up with the depegging collateral and taking out the better collateral. This means uAD ultimately depegs as well.

Vulnerability Detail

Chainlink price may be slightly outdated with regards to actual Dex state, and in that case users holding a depegging asset (let's consider DAI depegging) will use uAD to swap for the still pegged collateral: LUSD. By doing that they expect a better execution than on Dexes, because they swap at the price of chainlink minus the uAD fee:
https://github.com/sherlock-audit/2023-12-ubiquity/blob/main/ubiquity-dollar/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol#L358-L364

This in turn fills the reserves of UbiquityPool with the depegging collateral and depletes the reserves of good collateral.

Impact

A depegging collateral will cause uAD to depeg because users are incentivised to use the pool to swap for the better asset

Code Snippet

Tool used

Manual Review

Recommendation

Multiple solutions may be studied:

  • Enforce a ratio between different collateral reserves (somewhat like GMX pricing algo which also enables users to swap with zero slippage using chainlink feeds)
  • Use a safety minting ratio (LTV mechanism similar to borrowing protocols)
  • Force chainlink feeds to stay within acceptable thresholds for stable coins (revert operations on collateral if price is out of range)
@github-actions github-actions bot added the Excluded Excluded by the judge without consulting the protocol or the senior label Jan 14, 2024
@sherlock-admin2
Copy link
Contributor

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
Copy link
Contributor

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

@gitcoindev
Copy link

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

Should we also add 'Sponsor Disputed' label in this issue as well? @rndquu @pavlovcik @molecula451

@0x4007
Copy link
Contributor

0x4007 commented Jan 16, 2024

It probably makes more sense to ask @AuditSea (not sure if this is the corresponding GitHub handle.)

@rndquu
Copy link

rndquu commented Jan 17, 2024

As far as I understand this issue describes the following scenario:

  1. User1 mints 100 Dollar tokens and provides 100 DAI collateral
  2. User2 mints 100 Dollar tokens and provides 100 LUSD collateral
  3. DAI depegs
  4. User1 (who initially deposited DAI) redeems 100 Dollar tokens for 100 LUSD
  5. User2 (who initially deposited LUSD) is left only with depegged DAI pool

If DAI depegs then the Dollar token will also depeg mainly because DAI is used as an underlying collateral in the Dollar-3CRVLP curve's metapool. We shouldn't limit users in redeeming (i.e. burning) Dollar tokens anyhow because Dollar redeems bring back the Dollar/USD quote to $1.00 peg. So it seems to be fine that users should be able to burn Dollars until the pools are empty no matter where they initially deposited to because this is the only way to maintain the Dollar token's USD peg.

In case of a collateral depeg the only way to hedge Dollar depeg to some extent is to acquire fees and yield from AMO minters. This is not a 100% guarantee but I guess that if chainlink works fine (i.e. provides not too stale data) and we have 5% overcollateralization (from fees and yield) the Dollar token should not depeg too much.

Force chainlink feeds to stay within acceptable thresholds for stable coins (revert operations on collateral if price is out of range)

I don't understand how reverting on minting and redeeming helps. Reverting in this case means acquiring bad debt since all operations are paused and abritragers are not able to bring the Dollar token back to the USD peg by burning Dollars which makes the Dollar token to depeg with greater force.

@0x4007
Copy link
Contributor

0x4007 commented Jan 17, 2024

@rndquu as a heads up we should only start with accepting LUSD and then maybe, eventually, add sUSD next in case of future liquidity issues. DAI isn't part of the plan yet.

@rndquu
Copy link

rndquu commented Jan 23, 2024

Initially we plan to use only LUSD as collateral so there won't be the case with bad (i.e. depegging) and good collateral.

I think we should:

  1. Create a separate issue for this one in our repo and fix it later since it is not critical
  2. Mark the current issue as valid and "won't fix"

@molecula451 @gitcoindev Help

@sherlock-admin sherlock-admin changed the title Radiant Charcoal Horse - UbiquityPool::mintDollar/redeemDollar collateral depeg will encourage using UbiquityPool to swap for better collateral cergyk - UbiquityPool::mintDollar/redeemDollar collateral depeg will encourage using UbiquityPool to swap for better collateral Jan 24, 2024
@sherlock-admin sherlock-admin added the Reward A payout will be made for this issue label Jan 24, 2024
@0x3agle
Copy link

0x3agle commented Jan 24, 2024

In the case of a depeg of an underlying collateral, UbiquityPool mechanism incentivises users to fill it up with the depegging collateral and taking out the better collateral.

  • If the uAD price is stable at $1 in the uAD-3CRV pool, then the mints (open if uAD > $1.01) or redeems (open if uAD < 0.99) won't be enabled.
  • The chainlink oracle - is used to get the price of collateral. This price is then used to determine the amount of uAD to mint or burn, proportional to the collateral
  • The TWAP price from the curve pool will decide whether to enable mints or enable redeems
  • Hence the collateral de-pegging doesn't affect the uAD price, because we get the uAD price from the Curve pool (ref)

@molecula451 molecula451 added Disagree With Severity The sponsor disputed the severity of this issue Won't Fix The sponsor confirmed this issue will not be fixed labels Jan 24, 2024
@molecula451
Copy link

molecula451 commented Jan 24, 2024

The above sceneario makes much more sense on the current issue, a fix won't happen, this is most likely an invalid

@0xLogos
Copy link

0xLogos commented Jan 26, 2024

Escalate
Should be invalid
Reverting redemption if one of the collateral depegs even greater evil because now no one can redeem uD as noticed by rndquu. I think collateral depeg is a very unpleasant event and you can’t simply get away with it without loss.
Enforcing a ratio between different collateral reserves is not good solution as it has bad consequences for market efficiency throughout the life of the protocol and will barely (arguably) mitigate losses in case of depeg.

@sherlock-admin2
Copy link
Contributor

Escalate
Should be invalid
Reverting redemption if one of the collateral depegs even greater evil because now no one can redeem uD as noticed by rndquu. I think collateral depeg is a very unpleasant event and you can’t simply get away with it without loss.
Enforcing a ratio between different collateral reserves is not good solution as it has bad consequences for market efficiency throughout the life of the protocol and will barely (arguably) mitigate losses in case of depeg.

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-admin sherlock-admin added the Escalated This issue contains a pending escalation label Jan 26, 2024
@evmboi32
Copy link

In my issue #144, I described that the pool can run out of collateral if the collateral gets de-pegged. This means that users cannot redeem their uAD tokens for the underlying collateral.

@0x4007
Copy link
Contributor

0x4007 commented Jan 29, 2024

I really feel like collateral performance is out of scope for the audit. For whatever its worth, the reason why we are starting with LUSD is because it seems to have the least points of failure out of all the stablecoins I'm aware of. The tradeoff is its slight volatility and limited ability to scale.

To me it comes across that you're saying that we need to include Liquity's entire protocol within our audit scope in order to confirm that a depeg isn't possible, and that our protocol will not fail.

If this is in scope, why not proceed with Ethereum blockchain failures? If the network gets taken over, then fraudulent transactions can be generated to withdraw all of our collateral, rendering the protocol insolvent.

etc

@rndquu
Copy link

rndquu commented Jan 29, 2024

I really feel like collateral performance is out of scope for the audit. For whatever its worth, the reason why we are starting with LUSD is because it seems to have the least points of failure out of all the stablecoins I'm aware of. The tradeoff is its slight volatility and limited ability to scale.

To me it comes across that you're saying that we need to include Liquity's entire protocol within our audit scope in order to confirm that a depeg isn't possible, and that our protocol will not fail.

If this is in scope, why not proceed with Ethereum blockchain failures? If the network gets taken over, then fraudulent transactions can be generated to withdraw all of our collateral, rendering the protocol insolvent.

etc

Collateral depeg does harm the ubiquity protocol hence it is considered a valid issue (not sure what severity though)

@0xLogos
Copy link

0xLogos commented Jan 30, 2024

In my issue #144, I described that the pool can run out of collateral if the collateral gets de-pegged. This means that users cannot redeem their uAD tokens for the underlying collateral.

@evmboi32 Ok, depeg happens, collateral's price is now out of min/max range and redemptions are now failing (proposed in #144 solution). Then what? Wait and hope the collateral to repeg?

@nevillehuang
Copy link
Collaborator

nevillehuang commented Feb 5, 2024

@CergyK Are you aware if Ubiquity has a circuit breaker for depeg events?

To me if they lack one, this will constitute as a valid medium severity finding as a depeg does directly undermine the protocols available collateral. This is in addition to the depeg scenario not being stated as an accepted risk by the protocol in the contest details (to my knowledge, most of the time, a stablecoin protocol would acknowledge the risks of a depeg scenario and even have a circuit breaker in place).

@molecula451
Copy link

no we don't have circuit breaker @nevillehuang

@Czar102
Copy link

Czar102 commented Feb 12, 2024

The maximum divergence of the market price from the oracle feed should be within the fee charged. Planning to accept the escalation and invalidate the issue.

@0x4007
Copy link
Contributor

0x4007 commented Feb 12, 2024

We have our staking contract that allows us to manipulate single sided liquidity on our metapool to directly change the price right now.

@Czar102 Czar102 removed the Medium A valid Medium severity issue label Feb 14, 2024
@sherlock-admin sherlock-admin added Non-Reward This issue will not receive a payout and removed Reward A payout will be made for this issue labels Feb 14, 2024
@Czar102 Czar102 closed this as completed Feb 14, 2024
@Czar102
Copy link

Czar102 commented Feb 14, 2024

Result:
Invalid
Has duplicates

@sherlock-admin2 sherlock-admin2 removed the Escalated This issue contains a pending escalation label Feb 14, 2024
@sherlock-admin sherlock-admin added the Escalation Resolved This issue's escalations have been approved/rejected label Feb 14, 2024
@sherlock-admin2
Copy link
Contributor

Escalations have been resolved successfully!

Escalation status:

@gstoyanovbg
Copy link

gstoyanovbg commented Feb 16, 2024

@Czar102 I don't understand the argument for invalidating this report, could you explain a little more?

From my point of view, I submitted report #217 because when calculating the amount of collateral a user will receive when redeeming, the real price of the dollar token is not used, but it is assumed to always be $1. Let me give an example:

  1. User A mints 120 dollar tokens for 120 DAI
  2. User B mints 120 dollar tokens for 120 LUSD
  3. DAI depegs with 20%. Now collateral to dollar tokens ratio is (120 + 120*0.8) / 240 = 0.9
  4. User A redeems 100 dollar tokens for 120 DAI (because the price of DAI is $0.8) and 20 dollar tokens for 20 LUSD. After this operation, there are 120 dollar tokens in circulation and 100 LUSD collateral. Now the collateral to dollar tokens ratio is 100/120 = 0.83, less than the ratio at step 3. If there are more users, those who are last would not be able to get anything for their tokens because there will be no remaining collateral. This scenario is harmful to both the protocol and the users. I want to note that in redeemDollar() we only have upper bound for the dollar token price.

Wouldn't it be better to use the real price of the dollar token when determining the amount of collateral to be received in exchange for the dollar tokens? This way, each user will receive a proportional share of the available collateral, and there will not be a situation where there are dollar tokens in circulation without collateral behind them.

@molecula451
Copy link

@Czar102 I don't understand the argument for invalidating this report, could you explain a little more?

From my point of view, I submitted report #217 because when calculating the amount of collateral a user will receive when redeeming, the real price of the dollar token is not used, but it is assumed to always be $1. Let me give an example:

  1. User A mints 120 dollar tokens for 120 DAI
  2. User B mints 120 dollar tokens for 120 LUSD
  3. DAI depegs with 20%. Now collateral to dollar tokens ratio is (120 + 120*0.8) / 240 = 0.9
  4. User A redeems 100 dollar tokens for 120 DAI (because the price of DAI is $0.8) and 20 dollar tokens for 20 LUSD. After this operation, there are 120 dollar tokens in circulation and 100 LUSD collateral. Now the collateral to dollar tokens ratio is 100/120 = 0.83, less than the ratio at step 3. If there are more users, those who are last would not be able to get anything for their tokens because there will be no remaining collateral. This scenario is harmful to both the protocol and the users. I want to note that in redeemDollar() we only have upper bound for the dollar token price.

Wouldn't it be better to use the real price of the dollar token when determining the amount of collateral to be received in exchange for the dollar tokens? This way, each user will receive a proportional share of the available collateral, and there will not be a situation where there are dollar tokens in circulation without collateral behind them.

we will leavy it as invalid

@Czar102
Copy link

Czar102 commented Feb 19, 2024

@gstoyanovbg thank you for the comment. We had an extensive internal discussion about this issue and some of the other ones, considerations about the exact behavior you are describing. There is a chance we may revert this escalation's resolution, if we determine that this has been a misjudgment.

I would recommend looking at #60 as a core cause of this issue.

@Czar102
Copy link

Czar102 commented Feb 19, 2024

Will consider this a valid Medium and change the escalation resolution status.

@Czar102 Czar102 added the Medium A valid Medium severity issue label Feb 19, 2024
@sherlock-admin sherlock-admin added Reward A payout will be made for this issue and removed Non-Reward This issue will not receive a payout labels Feb 19, 2024
@sherlock-admin2 sherlock-admin2 added the Sponsor Disputed The sponsor disputed this issue's validity label Feb 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Disagree With Severity The sponsor disputed the severity of this issue 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 valid 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