sherlock-admin commented 8 months ago

cergyk

medium

LibUbiquityPool::mintDollar/redeemDollar reliance on outdated TWAP oracle may be inefficient for preventing depeg

Summary

The ubiquity pool used for minting/burning uAD relies on a twap oracle which can be outdated because the underlying metapool is not updated when calling the ubiquity pool. This would mean that minting/burning will be enabled based on an outdated state when it should have been reverted and inversely

Vulnerability Detail

We can see that LibTWAPOracle has an update function to keep its values up to date according to the underlying metapool: https://github.com/sherlock-audit/2023-12-ubiquity/blob/main/ubiquity-dollar/packages/contracts/src/dollar/libraries/LibTWAPOracle.sol#L61-L102

And that this function is called when minting/burning uADs: https://github.com/sherlock-audit/2023-12-ubiquity/blob/main/ubiquity-dollar/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol#L344

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

But the function update is not called on the underlying metapool, so current values fetched for it may be stale: https://github.com/sherlock-audit/2023-12-ubiquity/blob/main/ubiquity-dollar/packages/contracts/src/dollar/libraries/LibTWAPOracle.sol#L134-L136

Impact

A malicious user can use this to mint/burn heavily in order to depeg the coin further

Code Snippet

Tool used

Manual Review

Recommendation

Call the function:

def remove_liquidity(
    _burn_amount: uint256,
    _min_amounts: uint256[N_COINS],
    _receiver: address = msg.sender
)

On the underlying metapool the twap is based on, with only zero values, to ensure that the values of the pool are up to date when consulted

sherlock-admin2 commented 8 months ago

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

auditsea commented:

The issue describes about TWAP can be manipulated because update function can be called anytime and by anyone, thus TWAP period can be as short as 1 block. It seems like a valid issue but after caeful consideration, it's noticed that the TWAP issue does not come from its period but the logic itself is incorrect, thus marking this as Invalid

sherlock-admin2 commented 8 months ago

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

auditsea commented:

The issue describes about TWAP can be manipulated because update function can be called anytime and by anyone, thus TWAP period can be as short as 1 block. It seems like a valid issue but after caeful consideration, it's noticed that the TWAP issue does not come from its period but the logic itself is incorrect, thus marking this as Invalid

gitcoindev commented 8 months ago

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

auditsea commented:

The issue describes about TWAP can be manipulated because update function can be called anytime and by anyone, thus TWAP period can be as short as 1 block. It seems like a valid issue but after caeful consideration, it's noticed that the TWAP issue does not come from its period but the logic itself is incorrect, thus marking this as Invalid

Shouldn't the issue be marked with 'Sponsor disputed' label then if it is invalid? @rndquu @pavlovcik @molecula451

0x4007 commented 8 months ago

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

rndquu commented 8 months ago

A malicious user can use this to mint/burn heavily in order to depeg the coin further

Economically it doesn't make sense for a malicious user to do so. User is incentivised to arbitrage Dollar tokens hence even when metapool price is stale and a huge trade happens (i.e. a huge amount of Dollar tokens is minted in the Ubiquity pool) all secondary markets (uniswap, curve, etc...) will be on parity (in terms of Dollar-ANY_STABLECLON pair) after some time hence arbitrager will have to call _update() (when adding or removing liquidity) in the curve's metapool to make a profit.

Meanwhile I agree that the metapool's price can be stale but I don't understand how exactly minting "too many" Dollar tokens (keeping in mind that we get collateral in return) or burning "too many" Dollar tokens (keeping in mind that it reduces the Dollar/USD quote) may harm the protocol.

Anyway fresh data is better than stale one and the fix looks like a one liner so perhaps it makes sense to implement it. So here we could remove liquidity from the curve's metapool with 0 values (as mentioned in the "recommendation" section) which updates cumulative balances under the hood.

@gitcoindev @molecula451 What do you think?

gitcoindev commented 8 months ago

A malicious user can use this to mint/burn heavily in order to depeg the coin further

Economically it doesn't make sense for a malicious user to do so. User is incentivised to arbitrage Dollar tokens hence even when metapool price is stale and a huge trade happens (i.e. a huge amount of Dollar tokens is minted in the Ubiquity pool) all secondary markets (uniswap, curve, etc...) will be on parity (in terms of Dollar-ANY_STABLECLON pair) after some time hence arbitrager will have to call _update() (when adding or removing liquidity) in the curve's metapool to make a profit.

Meanwhile I agree that the metapool's price can be stale but I don't understand how exactly minting "too many" Dollar tokens (keeping in mind that we get collateral in return) or burning "too many" Dollar tokens (keeping in mind that it reduces the Dollar/USD quote) may harm the protocol.

Anyway fresh data is better than stale one and the fix looks like a one liner so perhaps it makes sense to implement it. So here we could remove liquidity from the curve's metapool with 0 values (as mentioned in the "recommendation" section) which updates cumulative balances under the hood.

@gitcoindev @molecula451 What do you think?

I have the same impression. Since this is easy to remediate with updating of cumulative balances I agree we could accept and implement it there.

osmanozdemir1 commented 8 months ago

Escalate

Some duplicates of this issue fail to explain the actual root cause of it, which is internal update function being called in the beginning of every action in the underlying metapool.

Only valid duplicates of this issue are #68, #134, and #181. They all explain the core problem about curve metapool updating mechanism and provide recommendations regarding how to update underlying pool to solve the problem.

Issues #34, #84, #92 and #187 are more related to consult() function returning stale value. Some of them mention general things about TWAP (like how low volume pools affect TWAP etc), and provide vague explanations. However, none of them pinpoints the actual cause of this issue. They don't mention underlying metapool's updating mechanism and should not be considered as valid duplicates without finding the root cause.

Last 4 issues might be considered as separate group of valid issues or they might be invalidated depending on the judge's preference, but they should not be duplicates of this one.

Kind regards.

sherlock-admin2 commented 8 months ago

Escalate

Some duplicates of this issue fail to explain the actual root cause of it, which is internal update function being called in the beginning of every action in the underlying metapool.

Only valid duplicates of this issue are #68, #134, and #181. They all explain the core problem about curve metapool updating mechanism and provide recommendations regarding how to update underlying pool to solve the problem.

Issues #34, #84, #92 and #187 are more related to consult() function returning stale value. Some of them mention general things about TWAP (like how low volume pools affect TWAP etc), and provide vague explanations. However, none of them pinpoints the actual cause of this issue. They don't mention underlying metapool's updating mechanism and should not be considered as valid duplicates without finding the root cause.

Last 4 issues might be considered as separate group of valid issues or they might be invalidated depending on the judge's preference, but they should not be duplicates of this one.

Kind regards.

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.

nevillehuang commented 8 months ago

@osmanozdemir1 I have internally discussed this with LSW and initially did deduplicated them as you mentioned. However, the watsons are technically not wrong per say by indicating an update required by using a stale time interval, so I decided to duplicate them.

osmanozdemir1 commented 8 months ago

@nevillehuang Thanks for the response.

I totally agree with you about watsons being technically not wrong. However, this issue is one step deeper than a regular stale price issue. Even non-stale prices (in terms of time interval) will be incorrect because of this issue.

For example let's say staleness threshold is 4 hours:

  /*           T-4 hours         T-3 hours                                         T0 current
                 |-----------------|---------------------------------------------------|
                              Latest action                                   The time Ubiquity 
                             in curve metapool                              updates & checks the price

                                   |----------------------------------------------------|
                                     Latest actions impact until now is not accounted 
*/

In an example above, staleness interval check will not revert because it is inside the staleness threshold. However, the latest action's impact on the price is never accounted. If it is a large swap or deposit, 3 hours worth of impact of this action will be huge.

Staleness check is not a solution for this problem. Only solution is somehow invoking the internal update function of the underlying curve metapool. And only issues I mentioned above explains this problem.

Therefore, I strongly believe those other 4 issues regarding staleness check are valid issues but separate ones.

Kind regards.

0xLogos commented 8 months ago

Escalate

Imho staleness part should be low and another should be invalid because of this

A malicious user can use this to mint/burn heavily in order to depeg the coin further

Economically it doesn't make sense for a malicious user to do so. User is incentivised to arbitrage Dollar tokens hence even when metapool price is stale and a huge trade happens (i.e. a huge amount of Dollar tokens is minted in the Ubiquity pool) all secondary markets (uniswap, curve, etc...) will be on parity (in terms of Dollar-ANY_STABLECLON pair) after some time hence arbitrager will have to call _update() (when adding or removing liquidity) in the curve's metapool to make a profit.

Meanwhile I agree that the metapool's price can be stale but I don't understand how exactly minting "too many" Dollar tokens (keeping in mind that we get collateral in return) or burning "too many" Dollar tokens (keeping in mind that it reduces the Dollar/USD quote) may harm the protocol.

I would also like to ask for a specific numerical example. From my understanding check against TWAP not for prevent depeg but for...

prevent unnecessary redemptions that could adversely affect the Dollar price

i.e. for cases like redeeming uD when it's price above peg 1$ which is simply unprofitable because within the protocol it's still 1$.

sherlock-admin2 commented 8 months ago

Escalate

Imho staleness part should be low and another should be invalid because of this

A malicious user can use this to mint/burn heavily in order to depeg the coin further

Economically it doesn't make sense for a malicious user to do so. User is incentivised to arbitrage Dollar tokens hence even when metapool price is stale and a huge trade happens (i.e. a huge amount of Dollar tokens is minted in the Ubiquity pool) all secondary markets (uniswap, curve, etc...) will be on parity (in terms of Dollar-ANY_STABLECLON pair) after some time hence arbitrager will have to call _update() (when adding or removing liquidity) in the curve's metapool to make a profit.

Meanwhile I agree that the metapool's price can be stale but I don't understand how exactly minting "too many" Dollar tokens (keeping in mind that we get collateral in return) or burning "too many" Dollar tokens (keeping in mind that it reduces the Dollar/USD quote) may harm the protocol.

I would also like to ask for a specific numerical example. From my understanding check against TWAP not for prevent depeg but for...

prevent unnecessary redemptions that could adversely affect the Dollar price

i.e. for cases like redeeming uD when it's price above peg 1$ which is simply unprofitable because within the protocol it's still 1$.

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.

nevillehuang commented 8 months ago

I agree with @osmanozdemir1 analysis. I think this issues can possibly be separated. However, what does a staleness check actually do, isn't it to invoke that particular underlying update function in the first place (which is why I validated them)?

osmanozdemir1 commented 7 months ago

I agree with @osmanozdemir1 analysis. I think this issues can possibly be separated. However, what does a staleness check actually do, isn't it to invoke that particular underlying update function in the first place (which is why I validated them)?

Staleness check might revert or update underlying pool depending on how the protocol implements it. However, even if staleness check updates the underlying pool, it would only do that in case of the last action is outside of the staleness threshold. It won't update the price all the time.

Staleness check won't update the price if it thinks the price is not stale. The thing I was trying to point out here is that the underlying curve pool update function must be invoked all the time regardless of the staleness.

0xLogos commented 7 months ago

181 is nice explanation, but i believe it lacks severe impact

TWAP prices are incorrect.

It is view function used only to check thresholds in mint/redeem

Incorrect amounts of tokens will be minted.

Mint/redeem amounts are based on chainlink oracle, not TWAP

DollarToken's may be minted or redeemed outside of the price threshold.

It can happen, but there's no incentives to do it. Thresholds allow to mint/redeem uAD if it's actually profitable according to TWAP. If allowance is false-posive mint/redeem will not be profitable, if it is false-negative arb can themself update curve pool and thus update TWAP.

So IMO impact here is that there is possible situation when arb is forced to update curve pool themself in order to execute arb. But is this alone medium? And how often these situations will occur?

osmanozdemir1 commented 7 months ago

Hi @0xLogos. Thanks for your comment.

It is view function used only to check thresholds in mint/redeem

Indeed it is used to check thresholds in mint/redeem. This protocol has certain threshold prices and issue #181 explains how tokens can be minted outside of these threshold. It is clearly broken core functionality.

It can happen, but there's no incentives to do it.

I can interpret this sentence in another way. If there is no incentive to do it and if it is not profitable for arbitragers etc, just a regular user may get harmed. A normal user doesn't have to track all these things and doesn't need to check Curve prices since the user expects protocol to revert outside of these thresholds.

minting/redeeming uAD above 1.01 and below 0.99 is the central feature of this protocol and in my opinion it is quite clear that minting/redeeming outside of the pre-determined thresholds is a broken functionality.

CergyK commented 7 months ago

Manipulations of the TWAP oracle have a high impact because they can cause insolvency on the Pool.

Indeed any redeeming/minting is done at the price indicated by chainlink. So what if the TWAP oracle is not aligned with the chainlink value? A malicious user which already has a large amount of uAD can drain the pool of the collateral

0xLogos commented 7 months ago

just a regular user may get harmed

User mistake

I believe that "core functionality" in judging docs refers to something with obvious impact and I don't see it here, but it's for judges to decide.

A malicious user which already has a large amount of uAD can drain the pool of the collateral

Don't see why it's malicious, uAD exchanged for collateral according to chainlink (fair) price, not outdated TWAP. Burning large amount of uAD will raise demand and incentivises to mint uAD for collateral and replenish pool - intended behavior.

osmanozdemir1 commented 7 months ago

User mistake

I disagree with that. User expects the protocol act like it is stated in the docs. User is not doing something wrong or giving incorrect input. For example, with your argument, every sandwich attack would be a user mistake due to not using private mempool or not being cautious enough but we consider it as lack of slippage protection.

Whole mint/redeem can be done outside of the intended thresholds with incorrect price assumptions. I don't understand how more core you want but will leave it to the judge.

nevillehuang commented 7 months ago

@Czar102 See issue #181 for a more complete issue breakdown with more thorough impact analysis (also the report I selected). I believe this is a valid medium severity issue.

0xLogos commented 7 months ago

Sandwitch attack is different. There is attacker, profit for attacker and loss for user. Here user themself making suboptimal choice, but again, he not loosing anything. Also this easier for user to prevent than sandwithcing.

Czar102 commented 7 months ago

I agree with the deduplication proposed – issues #34, #84, #92 and #187 aren't describing this issue to a sufficient extent.

I was planning to consider this a low (it's a low/medium borderline issue imo) because I thought there is just protocol revenue lost due to suboptimal pricing.

Indeed any redeeming/minting is done at the price indicated by chainlink. So what if the TWAP oracle is not aligned with the chainlink value? A malicious user which already has a large amount of uAD can drain the pool of the collateral

@CergyK than you for this comment, this seems to be the case. If the protocol had a single oracle (maybe chainlink and TWAP integrated), there would no issues related to this discrepancy. If there was a single oracle, the uAD would be verifiably solvent (given that the thresholds are configured correctly), but right now, due to the possibilities of these discrepancies, there is no such a guarantee. Tagging @pavlovcik @gitcoindev @molecula451 @rndquu, maybe it will be a valuable modification. Would also like @CergyK to sign off on this reasoning.

In real market scenarios, if the price has been radically changed, there is some market activity that will update the TWAP, so the probability of this issue having real impact is negligible, and this issue will usually have some impact on the price, nevertheless negligible. Hence, I think Medium severity is perfect for this issue.

Planning to separate and invalidate #34, #84, #92 and #187 and leave other duplicates and this issue as they are.

0xLogos commented 7 months ago

Sorry, i think i've lost the point.

So what if the TWAP oracle is not aligned with the chainlink value?

@CergyK TWAP is for uAD but chainlink for collateral. What do you mean they are not aligned?

@Czar102 What verifiably solvent means? User with sufficient uAD can redeem and get all collateral when uAD fresh TWAP < 0.99. Why is't bad?

Czar102 commented 7 months ago

User with sufficient uAD can redeem and get all collateral when uAD fresh TWAP < 0.99. Why is't bad?

@0xLogos If the Chainlink oracle displays price of uAD of $1.02 and the TWAP is $0.99, then one can redeem and they will get more for redeems ($1.02) than pay for some mints ($1.01).

If one oracle was used for checking thresholds and exchange rates, it would be impossible to mint for a smaller cost than profit from redeeming.

CergyK commented 7 months ago

Would also like @CergyK to sign off on this reasoning.

Agreed, as an additional remediation to the individual TWAP issues, it would be reasonable to introduce a deviation check between the two oracles

0xArz commented 7 months ago

@Czar102 The chainlink oracle is not used for uAD, its used for the collateral. There is no uAD chainlink feed so the curve pool twap is used by checking the reserves and the price of that the twap returns does not determine the amount of the collateral tokens that the user receives, it just checks the thresholds

0xLogos commented 7 months ago

@Czar102 But chainlink oracle not used for uAD pricing, there's no such feed. uAD price hardcoded to 1$ in the pool, only collateral chainlink feed used for calculating exchange rate assuming uAD worth 1$.

CergyK commented 7 months ago

@0xArz @0xLogos

By giving the right to users to mint/redeem collateral at the price given by the feed collateral/USD, the price of uAD is actually defined by this feed, so we can safely consider that the feed is also the real price collateral/uAD.

Even though I agree with your point that the chainlink feed is not technically a uAD feed.

0xArz commented 7 months ago

@0xArz @0xLogos

By giving the right to users to mint/redeem collateral at the price given by the feed collateral/USD, the price of uAD is actually defined by this feed, so we can safely consider that the feed is also the real price collateral/uAD.

Even though I agree with your point that the chainlink feed is not technically a uAD feed.

Its an exchange rate that will always be 1 if the collateral is pegged to $1.00 and you will only receive more or less if the collateral price changes not the uAD price. The impact here is that it might be possible to mint/redeem outside of the thresholds, i dont get how you can “drain” the collateral.

osmanozdemir1 commented 7 months ago

In the simplest way:

If A happens
      do B

You are trying to invalidate this issue by saying the price calculation is correct while performing do B.

The bug is in if A happens. This function shouldn't be performing do B at all. It doesn't matter do B is correct or not. The function should not be in that if statement in the first place. That if statement is the most central part of this protocol. It is the decision to mint or not mint this stable token. It is the decision to burn or not burn.

I am genuinely surprised that we are still discussing the validity of a bug that directly impacts the total supply and mint/redeem decision of a stable token. uAD is a stable coin. It has certain rules to mint/burn. That rule is broken.

0xArz commented 7 months ago

@osmanozdemir1 Your report describes this issue really well although this comment https://github.com/sherlock-audit/2023-12-ubiquity-judging/issues/13#issuecomment-1945710223 about the impact is wrong.

uAD is a stable coin. It has certain rules to mint/burn. That rule is broken.

This is true but this still doesnt affect the price of uAD until its sold right? If the reserves were 99 and 101 then everyone is able to mint, the arbitrage bot would only need ~1 uAD to rebalance to the pool but what if an attacker just mints the max(50k uAD), isnt this the same problem? Although this rule is broken, you are not stealing anything, the protocol doesnt become insolvent or anything. Would be great if we could confirm with the sponsor what problem could this cause

osmanozdemir1 commented 7 months ago

Just an example in terms of what problem could this cause:

Let's assume TWAP price is 1.011 but the real price is 0.99.

Normally, the protocol should prevent minting and allow redeeming to maintain the peg. This way token supply will decrease and price will move towards to 1.
But because of the TWAP price is 1.01, it will allow minting more uAD instead of forbidding mint and allowing redeem. It will do the complete opposite, which will increase the total supply more instead of decreasing it. Which will decrease the real price more.

Decision to mint/burn and increasing/decreasing token supply are most crucial things in a stable coin and these crucial actions are performed based on mint/redeem thresholds in this protocol. Possibility to mint/redeem outside of these thresholds is not an innocent issue.

These thresholds will use outdated TWAP prices nearly all the time due to this issue because curve metapools are not highly active pools. They are not like uniswap pools. I provided both previous ubiquity metapool address and lusd metapool address in my issue if you want to check. There are only few exchange actions in weeks. That's why it is quite possible for this protocol to mint uAD instead of burn them or vice versa. People will keep minting until there is an external action in the underlying metapool that updates the TWAP, and that update might take too much time. This protocol should not wait an external action but it must update the underlying pool itself.

0xArz commented 7 months ago

But because of the TWAP price is 1.01, it will allow minting more uAD instead of forbidding mint and allowing redeem. It will do the complete opposite, which will increase the total supply more instead of decreasing it. Which will decrease the real price more.

Minting the token will not decrease the price, minting and then selling the token in the curve pool is what will decrease the price, because this isnt profitable, no one would do this.

If the real price was $0.99 then an arbitrage bot will buy uAD at a discounted price and then redeem and the pool will be updated because he just bought.

Let's assume TWAP price is 1.011

But the lookback window of the twap is 1 block so doesnt it just return the spot price and then use the current reserves? In that case if the price is 1.011 then an arbitrage bot will mint and sell right after the transaction that made the price 1.011. Described here https://github.com/sherlock-audit/2023-12-ubiquity-judging/issues/20

osmanozdemir1 commented 7 months ago

If the real price was $0.99 then an arbitrage bot will buy uAD at a discounted price and then redeem and the pool will be updated because he just bought.

The real price I meant here is the real TWAP price after _update during an exchange, which can not be known without calling an exchange action in the underlying metapool. Arbitrage bot can't know it since there is no feed that actively tracks real price. That's the whole point of the Curve metapool calling the _update function as first thing during any kind of exchange and this way all exchanges are done with the most updated TWAP prices.

But the lookback window of the twap is 1 block so doesnt it just return the spot price and then use the current reserves?

Firstly, no it doesn't have to be 1 block. It is the difference between current timestamp and the last updated timestamp. #20 explains that it will be only 1 block if you update it in consecutive blocks.

Secondly, no it doesn't return the spot price and that's whole point of my submission. It uses currentCumulativePrices function, which returns latest updated prices. If underlying metapool doesn't have any new action, it will return the same cumulative price and the same timestamp even though if you call it now, or 3 hours later, or 1 day later. It directly returns the cumulative price and the timestamp at the point of the latest action in the underlying metapool. You can read the implementation code here: https://etherscan.io/address/0x5f890841f657d90e081babdb532a05996af79fe6#code

0xArz commented 7 months ago

@osmanozdemir1 I see yeah, i somehow thought that the update is done after the tx. Not sure of the impact but ig medium is appropriate for using an incorrect value to check the thresholds.

The comment by @Czar102 about the oracles is still incorrect tho. The impact of this issue is that most of the time it will use outdated twap price which can block or allow minting/redeeming. In https://github.com/sherlock-audit/2023-12-ubiquity-judging/issues/56 you would need a lot of funds just to allow/block minting and redeeming for a small amount of time which is not a problem unlike here where most of the time the wrong price will be used which will allow/block minting and redeeming

0xLogos commented 7 months ago

But because of the TWAP price is 1.01, it will allow minting more uAD instead of forbidding mint and allowing redeem. It will do the complete opposite, which will increase the total supply more instead of decreasing it. Which will decrease the real price more.

Allowing minting won't increase total supply. No one would mint just because it's allowed when it will cause losses. But one can trigger curve _update thus update TWAP and then redeem for profit. uAD is algotithmic stable coin and the market is assumed to operate efficiently. If bad actor wants to grief, arb will drain his funds.

Let's assume TWAP price is 1.011 but the real price is 0.99.

Just bcz TWAP is stale you can't assume whatever you want. What's probability of this state? I think such curve pools indeed can be stale for long time, but only when they balanced.

Sorry, I really do not want to argue, just for @Czar102 to understand and answer my points.

sherlock-admin commented 7 months ago

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

Czar102 commented 7 months ago

After extensive discussions with the LSW and the Lead Judge, planning to resolve the escalation as previously intended (https://github.com/sherlock-audit/2023-12-ubiquity-judging/issues/13#issuecomment-1945710223), as the issue doesn't only cause a DoS, but may allow for an easy value extraction strategy from the protocol, similar to the one described in #17 and duplicates.

Planning to accept the first and reject the second escalation.

0xLogos commented 7 months ago

From #17

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

You can't swap because allowed only either mint or redeem.

Also if pool works as expected it always lose value on mint/redeem, but it takes fee to mitigate this (btw this was my escalation point for #36 that fee actually used). Assume peg within desired range: 1.009, but mint is still open because twap is stale and returns 1.011, now you can mint 1 uAD = 1.009$ for 1$. But you pay same fee as usual. And usual mint expected to be more profitable and pool takes according fees. And with that fee mint is barely profitable.

This is just my thoughts about the strategy and honestly I can't think of anything profitable here.

@CergyK @nevillehuang But if there really is profitable strategy I want to learn it!

0xArz commented 7 months ago

@Czar102 @nevillehuang @CergyK https://github.com/sherlock-audit/2023-12-ubiquity-judging/issues/56 is not a valid issue. You will not be able to extract anything because it DoSes the redeem, not enables. The report also clearly mentions that. You can only make the price of uAD increase because it is impossible for the attacker to have a large amount of uAD if the only way to get it from is the curve pool, he can only make the price increase by providing a large amount of 3CRV.

I really dont undestand why we are trying to make https://github.com/sherlock-audit/2023-12-ubiquity-judging/issues/17 valid again which is a completely seperate issue from this one. As @0xLogos mentioned you are only able to mint or redeem so you cant sandwich the oracle update and it is not profitable.

easy value extraction strategy

Can you also please tell me since when is proposing 2 consecutive blocks considered easy?

osmanozdemir1 commented 7 months ago

@Czar102 @nevillehuang @CergyK #56 is not a valid issue. You will not be able to extract anything because it DoSes the redeem, not enables. The report also clearly mentions that. You can only make the price of uAD increase because it is impossible for the attacker to have a large amount of uAD if the only way to get it from is the curve pool, he can only make the price increase by providing a large amount of 3CRV.

I really dont undestand why we are trying to make #17 valid again which is a completely seperate issue from this one. As @0xLogos mentioned you are only able to mint or redeem so you cant sandwich the oracle update and it is not profitable.

easy value extraction strategy

Can you also please tell me since when is proposing 2 consecutive blocks considered easy?

I think this comment is under wrong issue and nothing to do with this one.

nevillehuang commented 7 months ago

@0xLogos @0xArz

For #17, you can see from public knowledge that fees are intended to be zero, hence leakage is possible. Additionally, even if fees are implemented, there is no guarantees that it is sufficient to cover large price fluctuations as noted here
Proposing 2 consecutive blocks is not easy, but is a valid external vector to bypass thresholds to alternate between mint/redemption thresholds. It is possible for attacker to have externally owned uAD bought from secondary markets/minted from the protocol. Additionally, the TWAP price is influenced and reflected inaccurately since reserves are skewed similar to in #59, albeit a different cause.

0xArz commented 7 months ago

@nevillehuang The only way to obtain uAD is from the curve pool so you realistically cant have a huge amount like with 3crv. Can you also describe the whole attack path including the "profit" that the attacker gets combining https://github.com/sherlock-audit/2023-12-ubiquity-judging/issues/17 and https://github.com/sherlock-audit/2023-12-ubiquity-judging/issues/56?

nevillehuang commented 7 months ago

@0xArz I believe this is untrue based on your comment here and this statement here by sponsor. I think no further explanations is required from my end if not this discussion will be endless, better leave it to head of judging.

0xArz commented 7 months ago

Fine but they still have to get this uAD from somewhere but nvm. I just want you to realize how hard it is to perform an attack like this and looks like you cant even give me the attack path due to how hard the attack is. If you want to mint and then redeem you would have to manipulate the twap 2 times because as we mentioned you can either mint or redeem. This means that you would have to propose 2 consecutive blocks 2 times, if thats that easy is a 51% attack also a valid external vector to bypass thresholds?

I really dont want to argue but im tired of having to fight back because this issue is getting validated just because you can "manipulate a twap" without actually stating the profit that the attacker gets, the extremely low circumstances and the huge amount of resources that the attacker needs.

Talking about https://github.com/sherlock-audit/2023-12-ubiquity-judging/issues/17 or https://github.com/sherlock-audit/2023-12-ubiquity-judging/issues/56 here not https://github.com/sherlock-audit/2023-12-ubiquity-judging/issues/13 which is valid i believe.

Czar102 commented 7 months ago

The outline of the attack, some details may be wrong since I haven't audited the code myself:

Price of uAD was $0.99.
There was a trade that put the price at $1.01. No more trades are made.
The price of LUSD reported by an oracle is $1.
The real price of LUSD drifted to $0.99 and a few minutes pass.
The attacker deposits 1m LUSD to get 1m uAD.
The attacker triggers an update in the metapool. Roughly at the same time, the Chainlink oracle heartbeat is triggered to change the LUSD price to $0.99.
The TWAP is $1.01 now and the attacker can redeem 1m uAD for roughly 1.01m uAD to make a ~$10k profit.

Will close this escalation today.

0xArz commented 7 months ago

@Czar102 This probably makes sense for this issue https://github.com/sherlock-audit/2023-12-ubiquity-judging/issues/13 even though the likelihood is low but can you please describe the whole attack path in https://github.com/sherlock-audit/2023-12-ubiquity-judging/issues/56 where you have to propose 2 consecutive blocks 2 times? Really curious to see how you would do that!

Czar102 commented 7 months ago

When one of deposits/redeems are available, it's sufficient to propose 2 consecutive blocks only once.

Also, simply providing liquidity after a rapid price change will help changing the operation permitted between mints and redeems.

0xArz commented 7 months ago

In that case the price of uAD has to be 1.01$ so the attacker is able to mint first. If you really think that all of these preconditions and the amount of resources that the attacker needs just to hedge a small amount of collateral is an easy value extraction strategy then i really dont know but looks like there is no point in continuing to explain why this issue is invalid so you can decide whatever you want.

Czar102 commented 7 months ago

Result: Medium Has duplicates

@0xArz There are some preconditions, but the attack is possible and Medium severity is created for issues where loss of funds is limited, potentially due to many constraints.

sherlock-audit / 2023-12-ubiquity-judging

cergyk - LibUbiquityPool::mintDollar/redeemDollar reliance on outdated TWAP oracle may be inefficient for preventing depeg #13

LibUbiquityPool::mintDollar/redeemDollar reliance on outdated TWAP oracle may be inefficient for preventing depeg

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

181 is nice explanation, but i believe it lacks severe impact