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

[sherlock-admin]

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]

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]

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]

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]

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

[rndquu]

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]

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]

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]

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]

@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]

@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.

[CergyK]

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]

@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]

@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]

@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]

@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]

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]

@osmanozdemir1 Your report describes this issue really well although this comment #13 (comment) 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]

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]

• 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 #20

[osmanozdemir1]

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]

@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 #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]

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]

The protocol team fixed this issue in PR/commit ubiquity/ubiquity-dollar#893.

[Czar102]

After extensive discussions with the LSW and the Lead Judge, planning to resolve the escalation as previously intended (#13 (comment)), 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]

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]

@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?

[osmanozdemir1]

@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]

@0xLogos @0xArz

• For cergyk - UbiquityPool::mintDollar/redeemDollar collateral depeg will encourage using UbiquityPool to swap for better collateral #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 cergyk - UbiquityPool::redeemDollar DoS on redeeming if USDT/USDC depegs #59, albeit a different cause.

[0xArz]

@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 #17 and #56?

[nevillehuang]

@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]

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 #17 or #56 here not #13 which is valid i believe.

[Czar102]

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

1. Price of uAD was $0.99.
2. There was a trade that put the price at $1.01. No more trades are made.
3. The price of LUSD reported by an oracle is $1.
4. The real price of LUSD drifted to $0.99 and a few minutes pass.
5. The attacker deposits 1m LUSD to get 1m uAD.
6. 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.
7. 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]

@Czar102 This probably makes sense for this issue #13 even though the likelihood is low but can you please describe the whole attack path in #56 where you have to propose 2 consecutive blocks 2 times? Really curious to see how you would do that!

[Czar102]

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]

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]

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-admin2]

Escalations have been resolved successfully!

Escalation status:

• osmanozdemir1: accepted
• 0xLogos: rejected

[sherlock-admin]

The Lead Senior Watson signed off on the fix.