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

cergyk - LibTWAPOracle::update Providing large liquidity will manipulate TWAP, DOSing redeem of uADs #56

Open
Tracked by #866
sherlock-admin2 opened this issue Jan 10, 2024 · 34 comments
Labels
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 Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed

Comments

@sherlock-admin2
Copy link
Contributor

sherlock-admin2 commented Jan 10, 2024

cergyk

high

LibTWAPOracle::update Providing large liquidity will manipulate TWAP, DOSing redeem of uADs

Summary

The twap oracle used by ubiquity does not compute a TWAP (Time weighted average price) but an instant price based on time weighted average of balances, and thus is more vulnerable to manipulation by an actor temporarily allocating a large amount to the metapool.

Vulnerability Detail

We can see the LibTWAPOracle::update() calls on
IMetapool::get_twap_balances(uint256[2] memory _first_balances, uint256[2] memory _last_balances, uint256 _time_elapsed) which gets the time-weighted average of the pool reserves given time_elapsed and cumulative balances.

Then the time-weighted average is deduced by using the average of reserves over the time_elapsed. However we can see that a user with a large amount of capital and controlling 2 consecutive blocks can either add large liquidity in an imbalanced way in block N, and remove in block N+1.
In that case the higher reserves ratio will have a higher weight in the computation and will last for many blocks.

Scenario

1/ In block N-1:

Metapool contains 10 of uAD and 10 of 3CRV, the price is balanced, and withdrawals of uAD are accepted.

2/ In block N:

Alice updates both metapool twap and ubiquityPool twap.

Alice provides 100 uAD and 200 3CRV to the pool, now reserves are 110 uAD and 210 3CRV

3/ In block N+1:

Alice removes the previously added liquidity putting the reserves back to 10 uAD and 10 3CRV, but does not updates UbiquityPool right away.

4/ In block N+40:

Alice updates both Metapool and ubiquity TWAP oracle,

Let's see the value which is computed for the returned TWAP:

uAD cumulative reserve difference = 12*110+12*40*10 = 6120
3CRV cumulative reserve difference = 12*210+12*40*10 = 7320

We can see that the reserves are still very imbalanced even 40 blocks after the 2 blocks manipulation, blocking withdrawals, because uAD price versus 3CRV is too high:
https://github.com/sherlock-audit/2023-12-ubiquity/blob/main/ubiquity-dollar/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol#L418-L421

Impact

A malicious user with access to considerable capital and controlling two consecutive blocks (feasible since the merge), can DOS the withdraw functionality of UbiquityPool for many blocks.

Code Snippet

Tool used

Manual Review

Recommendation

Find a way to compute the twap of the price (as Uniswap v3 does), instead of using twap of balances as a proxy

@github-actions github-actions bot added the Excluded Excluded by the judge without consulting the protocol or the senior label Jan 14, 2024
@github-actions github-actions bot reopened this Jan 16, 2024
@github-actions github-actions bot added High A valid High severity issue Has Duplicates A valid issue with 1+ other issues describing the same vulnerability and removed Excluded Excluded by the judge without consulting the protocol or the senior labels Jan 16, 2024
@gitcoindev
Copy link

@rndquu @pavlovcik @molecula451 what do you think about this one? Technically possible, but would likely require lots of staked ETH. I am wondering if this is just hypothetical and controlling 2 blocks would require funds of e.g. Coinbase size. If this is the case, then perhaps impact is high but probability is very low. There are currently ~900k ETH validators https://beaconcha.in/charts/validators with each staked at least 32 ETH.

@0x4007
Copy link
Contributor

0x4007 commented Jan 18, 2024

I spent some time looking through our old issues and discussions but unfortunately I couldn't find my writeup on this. Basically as I recall, after The Merge there was a research report that proved how a two-consecutive-block TWAP attack could be performed with a realistic amount of funds.

Unfortunately I don't remember the conclusion of how to mitigate.

@rndquu
Copy link

rndquu commented Jan 18, 2024

uAD cumulative reserve difference = 12110+124010 = 6120
3CRV cumulative reserve difference = 12
210+124010 = 7320

TWAP window here is ~8 minutes. If we take a 30 minutes TWAP window (suggested everywhere) we get uAD cumulative reserves = 19320 & 3CRV cumulative reserves = 20520 which is much closer to a balanced pool.

Overall this seems to be a duplicate of #20 since increasing the TWAP window fixes the issue (to some extent).

@rndquu
Copy link

rndquu commented Jan 23, 2024

So, as far as I understand, the core issue is that the curve metapool uses accumulated balances instead of accumulated spot prices hence the TWAP is not as accurate as it could be.

I agree that it could be improved but disagree with the "high" severity.

@rndquu rndquu added Sponsor Confirmed The sponsor acknowledged this issue is valid Disagree With Severity The sponsor disputed the severity of this issue Will Fix The sponsor confirmed this issue will be fixed labels Jan 23, 2024
@nevillehuang nevillehuang added Medium A valid Medium severity issue and removed High A valid High severity issue labels Jan 23, 2024
@rndquu rndquu removed the Disagree With Severity The sponsor disputed the severity of this issue label Jan 23, 2024
@sherlock-admin2 sherlock-admin2 changed the title Radiant Charcoal Horse - LibTWAPOracle::update Providing large liquidity will manipulate TWAP, DOSing redeem of uADs cergyk - LibTWAPOracle::update Providing large liquidity will manipulate TWAP, DOSing redeem of uADs Jan 24, 2024
@sherlock-admin2 sherlock-admin2 added the Reward A payout will be made for this issue label Jan 24, 2024
@0xLogos
Copy link

0xLogos commented Jan 26, 2024

Escalate
Should be low. According to sherlock rules here.

If it will result in funds being inaccessible for >=1 year, then it would count as a loss of funds and be eligible for a Medium or High designation.

In this issue attacker would need a large amount of funds (exactly 15 times more than curve pool reserves according to example) and control over 2 consecutive block (feasible, but very difficult) just to dos withdrawals for short time (in example 40 blocks = 12*40/60 = 8 min).

@sherlock-admin2
Copy link
Contributor Author

sherlock-admin2 commented Jan 26, 2024

Escalate
Should be low. According to sherlock rules here.

If it will result in funds being inaccessible for >=1 year, then it would count as a loss of funds and be eligible for a Medium or High designation.

In this issue attacker would need a large amount of funds (exactly 15 times more than curve pool reserves according to example) and control over 2 consecutive block (feasible, but very difficult) just to dos withdrawals for short time (in example 40 blocks = 12*40/60 = 8 min).

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
@nevillehuang
Copy link
Collaborator

@CergyK any comments?

@0xLogos
Copy link

0xLogos commented Jan 27, 2024

I also want to note that the proposed solution is quite difficult to implement correctly and I believe that it is an acceptable risk to leave it as is

@0x3agle
Copy link

0x3agle commented Jan 27, 2024

Is uAD TWAP manipulatable?
short answer - Yes. Long answer - #72 (comment)
Pointing out that #212's main focus is on shorting the collateral at the cost of UbiquityPool. So, it should not be a duplicate of this issue

@0xLogos
Copy link

0xLogos commented Jan 27, 2024

@0x3agle It is manipulatable because of #20
I think firstly this issue should be fixed (not so trivial as recommended in report)
Also collateral tokens are "stealed" during arb

@0x3agle
Copy link

0x3agle commented Jan 27, 2024

@0xLogos, even after addressing issue 20, the system remains susceptible to manipulation.

In a standard arbitrage situation, specific conditions enable arbitrageurs to profit.

However, #212 deviates from this. Here, the attacker can intentionally trigger redemptions to acquire collateral at a lower price by influencing the uAD price. This means the attacker can engineer favorable conditions, rather than relying on chance, effectively manipulating the protocol to their advantage.

@0xLogos
Copy link

0xLogos commented Jan 27, 2024

The attacker opts to swap 3,000 uAD for X amount of 3CRV in the Curve pool, strategically lowering the "spot price" of uAD.

from #72 (comment)

Doesn't look like manipulation

@0x3agle
Copy link

0x3agle commented Jan 27, 2024

The attacker can lower the price to enable redeems as they can now get collateral for lower price. If this isn't manipulation, then what is it?

Edit: improved the wording in the comment.

@0xArz
Copy link

0xArz commented Jan 27, 2024

@0x3agle but why would you even need to manipulate the twap? I think your issue describes pretty much the same thing as #72 and the root cause is the same - collateral depegging. The attack will be possible without the TWAP manipulation(assuming the the price of uAD stays at $1.00)

Total Spent: 97,000 LUSD (for minting uAD) + ~$50 (Slippage for swapping twice [uAD to 3CRV then 3CRV to uAD])
Total Received: 101,000 LUSD
Profit: ~3,800 LUSD

There is no actual profit here, because 97,000 LUSD at $1.03 is worth the same as 101,000 LUSD at $0.99

Also i believe the redeemPriceThreshold is $1.01 not $0.99, it wouldnt make sense to not allow users to redeem when the price is $1.00 right? Would be great if sponsor could confirm this

I think what you are saying is that you can manipulate the price so that redeems are opened but i believe that the redeemPriceThreshold is $1.01 so you dont have to manipulate anything but just wait when the collateral drops

@0xLogos
Copy link

0xLogos commented Jan 27, 2024

There is no actual profit here, because 97,000 LUSD at $1.03 is worth the same as 101,000 LUSD at $0.99

Good point

@0x3agle
Copy link

0x3agle commented Jan 27, 2024

@0xArz

This is called as hedging.
There is no profit but if the attacker held those 97,000 LUSD at $1.03 and when it drops to $0.99, the attacker loses 4%.

By applying this strategy, the attacker offsets the losses at the cost of protocol. Therefore, the attacker does not lose any value.

Regarding redeem threshold: you are saying that to enable redeem, the uAD should go up instead of down.
Think about it: if uAD goes up to $1.01, there is more demand. We need the uAD to move down to $1.
You can't control demand, you can control the supply.

What will you do to level the uAD back? - Mint.
Increase the supply to match the demand. So, we want to enable mints when uAD goes up.

When uAD goes down to $0.99, the supply is more demand is less. Therefore we need to burn the uAD to match supply and demand. Hence redeem will be enabled if uAD goes below $0.99

@0xArz
Copy link

0xArz commented Jan 27, 2024

@0x3agle

require(
            getDollarPriceUsd() >= poolStorage.mintPriceThreshold,
            "Dollar price too low"
        );

So you are saying that the mintPriceThreshold is $1.01? How will then users be able to mint uAD when the curve pool was just created and the price is $1.00?

$0.99 for minting and $1.01 for redeeming makes more sense.

Same impact like in #72 btw

@0x3agle
Copy link

0x3agle commented Jan 27, 2024

When the pool is first deployed, users can neither mint or redeem.

The demand for uAD arises. Users start buying it using 3CRV from curve pool.

So, eventually the twap will show an increased price. Let’s say $1.02
Now, the mints on the pool will be unlocked. So users can technically mint more uAD for exact amount from Ubiquity Pool instead of 3CRV. This will lead to arbitrage, balancing the curve pool. So, now the pool will reflect the stable price of $1 for uAD.

How will redeems be unlocked?
In case a lot if uAD is minted, users will swap it for 3CRV in the curve pool. Increasing the supply of uAD in curve pool. Therefore reducing the price of uAD, let's say $0.98

Now redeems will be enabled (mints are disabled) which will give you more collateral for same uAD. Again arbitrage, leading to curve pool balance. Stabilising price of uAD back to $1- disabling any mints or redeems

@0xArz
Copy link

0xArz commented Jan 27, 2024

hey @pavlovcik @gitcoindev @rndquu what are the correct thresholds that will be used? In the tests the mintPriceThreshold is set to $1.01 and the redeemPriceThreshold is set to $0.99 which means that when the price is $1.00 users will not be able to mint or redeem uAD. This doesnt really makes sense as users are supposed to be able to obtain uAD on a 1:1 ratio or is the UbiquityPool just going to be used to rebalance the price? Thank you in advance!

@0x4007
Copy link
Contributor

0x4007 commented Jan 29, 2024

Also i believe the redeemPriceThreshold is $1.01 not $0.99, it wouldnt make sense to not allow users to redeem when the price is $1.00 right? Would be great if sponsor could confirm this

Minting above $1.00 and redemptions below $1.00. The threshold should be adjustable so for now redeemPriceThreshold should be $0.99. That way a user can purchase $0.99 "dollars" from the market and then redeem them for $1.00 worth of collateral. This stabilizes the price floor.

which means that when the price is $1.00 users will not be able to mint or redeem uAD. This doesnt really makes sense as users are supposed to be able to obtain uAD on a 1:1 ratio or is the UbiquityPool just going to be used to rebalance the price? Thank you in advance!

That's a good question. We have an application to manage and incentivize distributed teams which offers incentives to settle payments in our Ubiquity Dollars. Assuming that there is some demand generated for our Ubiquity Dollars with our DevPool system, we should easily be able to move the price above the threshold.

If there is no demand, I think it makes sense for the protocol to not respond with incentives to change the supply.

@CergyK
Copy link
Collaborator

CergyK commented Jan 29, 2024

@CergyK any comments?

It seems the escalation is based on the fact that a relatively short (?) duration was used in the example.
This duration can be arbitrarily extended by using more capital.
Overall this should not be happening in a TWAP, and for example in the TWAP as implemented in Uniswap, it is not possible to manipulate more efficiently by providing more liquidity during 2 blocks.

@rndquu
Copy link

rndquu commented Jan 29, 2024

hey @pavlovcik @gitcoindev @rndquu what are the correct thresholds that will be used? In the tests the mintPriceThreshold is set to $1.01 and the redeemPriceThreshold is set to $0.99 which means that when the price is $1.00 users will not be able to mint or redeem uAD. This doesnt really makes sense as users are supposed to be able to obtain uAD on a 1:1 ratio or is the UbiquityPool just going to be used to rebalance the price? Thank you in advance!

Users may also get Dollar(uAD) tokens on secondary markets

@0xLogos
Copy link

0xLogos commented Jan 29, 2024

It seems the escalation is based on the fact that a relatively short (?) duration was used in the example. This duration can be arbitrarily extended by using more capital. Overall this should not be happening in a TWAP, and for example in the TWAP as implemented in Uniswap, it is not possible to manipulate more efficiently by providing more liquidity during 2 blocks.

@pavlovcik @rndquu First of all we need to know max aceptable by protocol team time that pool can be DOSed taking into account that admin can in any time prevent DOS by adjusting threshholds. Based on example you need 100K pool liquidity * 15 = 1.5M$ AND control over 2 block to DOS for 8 min (correct me if i am wrong)

@rndquu
Copy link

rndquu commented Jan 29, 2024

It seems the escalation is based on the fact that a relatively short (?) duration was used in the example. This duration can be arbitrarily extended by using more capital. Overall this should not be happening in a TWAP, and for example in the TWAP as implemented in Uniswap, it is not possible to manipulate more efficiently by providing more liquidity during 2 blocks.

@pavlovcik @rndquu First of all we need to know max aceptable by protocol team time that pool can be DOSed taking into account that admin can in any time prevent DOS by adjusting threshholds. Based on example you need 100K pool liquidity * 15 = 1.5M$ AND control over 2 block to DOS for 8 min (correct me if i am wrong)

I don't think that any time >0 seconds is an acceptable DOS duration for any protocol :)

@0xLogos
Copy link

0xLogos commented Jan 30, 2024

I don't think that any time >0 seconds is an acceptable DOS duration for any protocol :)

Sorry but this is not true. There's no perfect systems, you have to come up with suitable threat model.

Also I believe that in order to understand the severity of the issue you need to somehow evaluate the difference between calculating price based on time average balances and using time average price like uni v3 because this is proposed solution. (If you have control over 2 block you can do similar thing with prices swapping back and forth in 2 blocks)

My point is example in report is very vague and there's need at least for more realistic PoC and ideally comparison with proposed solution.

@Czar102
Copy link

Czar102 commented Feb 15, 2024

I believe oracle manipulation can have a more severe impact than a DoS for a few minutes, see #13 (comment).

Planning to reject the escalation and leave the issue as is.

@0xLogos
Copy link

0xLogos commented Feb 16, 2024

#13 (comment) is inaccurate

@CergyK
Copy link
Collaborator

CergyK commented Feb 16, 2024

Since we are discussing this issue, it seems #184 #197 #212 are invalid/unrelated

@Czar102
Copy link

Czar102 commented Feb 17, 2024

Thank you @0xLogos for noting a mistake in my previous statement.
@CergyK there seems to be no loss of funds impact here, I think it's just a DoS for a few minutes (TWAP length) and that's the risk of using TWAP. No serious loss is inflicted on anyone.

We are also discussing the severity of TWAP manipulation on #59 right now, it may be relevant.

@sherlock-admin
Copy link
Contributor

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

@Czar102
Copy link

Czar102 commented Feb 19, 2024

After extensive discussions with the LSW and the Lead Judge, planning to leave this issue a Medium severity one, as it 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 reject the escalation and leave the issue as is.

@Czar102
Copy link

Czar102 commented Feb 19, 2024

Result:
Medium
Has duplicates

@sherlock-admin2 sherlock-admin2 removed the Escalated This issue contains a pending escalation label Feb 19, 2024
@sherlock-admin2
Copy link
Contributor Author

Escalations have been resolved successfully!

Escalation status:

@sherlock-admin sherlock-admin added the Escalation Resolved This issue's escalations have been approved/rejected label Feb 19, 2024
@sherlock-admin
Copy link
Contributor

The Lead Senior Watson signed off on the fix.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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 Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed
Projects
None yet
Development

No branches or pull requests