0xadrii - TWAP oracle might return a stale price

[sherlock-admin2]

0xadrii

medium

TWAP oracle might return a stale price

Summary

Ubiquity's methodology to query the metapool's TWAP oracle makes the code susceptible of fetching a stale TWAP price.

Vulnerability Detail

Ubiquity uses a uAD-3CRV Curve metapool in order to fetch the current uAD dollar price. If uAD’s dollar price is above $1, uAD minting will be enabled. Inversely, if uAD’s price is below $1, uAD burning will be enabled.

The price from the Curve metapool is fetched by using the metapool’s get_twap_balances. The process is as follows:

1. Every time a request for a price is needed (either in the mintDollar() or redeemDollar() functions) the getDollarPriceUsd() function will be called, which will internally interact with Ubiquity’s LibTWAPOracle’s getTwapPrice() function:

   // LibUbiquityPool.sol
   
   function getDollarPriceUsd()
           internal
           view
           returns (uint256 dollarPriceUsd)
       {
           // get Dollar price from Curve Metapool (18 decimals)
           uint256 dollarPriceUsdD18 = LibTWAPOracle.getTwapPrice();
           
   				...
       }

   // LibTWAPOracle.sol
   function getTwapPrice() internal view returns (uint256) {
           return
               LibTWAPOracle.consult(
                   LibAppStorage.appStorage().dollarTokenAddress
               );
       }
   

2. Internally, getTwapOracle() will call the consult() function:

   function consult(address token) internal view returns (uint256 amountOut) {
           TWAPOracleStorage memory ts = twapOracleStorage();
   
           if (token == LibAppStorage.appStorage().dollarTokenAddress) {
               // price to exchange 1 Ubiquity Dollar to 3CRV based on TWAP
               amountOut = ts.price0Average;
           } else {
               require(token == ts.token1, "TWAPOracle: INVALID_TOKEN");
               // price to exchange 1 3CRV to Ubiquity Dollar based on TWAP
               amountOut = ts.price1Average;
           }
       }

consult() will be the function that will actually return the USD price. If the price to exchange 1 Ubiquity Dollar to 3CRV is requested, the storage variable ts.price0Average will be returned. On the other hand, if the price to exchange 1 3CRV to Ubiquity Dollar is requested, the storage variable ts.price1Average is returned.

In order to update such storage variables that contain the latest price conversions, LibTWAPOracle’s update() function must be triggered (this function is always called before actually calling getDollarPriceUsd() when minting/redeeming). This is the function that actually interacts with the metapool’s get_twap_balances:

function update() internal {
        TWAPOracleStorage storage ts = twapOracleStorage(); 
        (
            uint256[2] memory priceCumulative, 
            uint256 blockTimestamp
        ) = currentCumulativePrices(); 
        if (blockTimestamp - ts.pricesBlockTimestampLast > 0) {  
            // get the balances between now and the last price cumulative snapshot
            uint256[2] memory twapBalances = IMetaPool(ts.pool)
                .get_twap_balances( 
                    ts.priceCumulativeLast,
                    priceCumulative,
                    blockTimestamp - ts.pricesBlockTimestampLast
                );

            // price to exchange amountIn Ubiquity Dollar to 3CRV based on TWAP
            ts.price0Average = IMetaPool(ts.pool).get_dy( 
                0,
                1,
                1 ether,
                twapBalances
            );

            // price to exchange amountIn 3CRV to Ubiquity Dollar based on TWAP
            ts.price1Average = IMetaPool(ts.pool).get_dy(
                1,
                0,
                1 ether,
                twapBalances
            );
            // we update the priceCumulative
            ts.priceCumulativeLast = priceCumulative;
            ts.pricesBlockTimestampLast = blockTimestamp;
        }
    }

The following steps are performed inside update() in order to fetch the TWAP price:

1. currentCumulativePrices() is called, which will internally fetch the last price cumulative values from the curve metapool, as well as the latest block.timestamp where such cumulatives where updated:

   // LibTWAPOracle.sol
   
   function currentCumulativePrices()
           internal
           view
           returns (uint256[2] memory priceCumulative, uint256 blockTimestamp) 
       { 
           address metapool = twapOracleStorage().pool;
           priceCumulative = IMetaPool(metapool).get_price_cumulative_last();
           blockTimestamp = IMetaPool(metapool).block_timestamp_last();
       }

2. After obtaining the cumulative prices and the timestamp (stored in priceCumulative and blockTimestamp, such values will be used to fetch the metapool’s TWAP using get_twap_balances), which will be stored in the twapBalances array. Curve’s TWAP only performs a time-weighted computation using the _last_balances and _first_balances passed as parameter, and considering the _time_elapsed between those two values:

   // Curve's uAD/3CRV metapool
   @view
   @external
   def get_twap_balances(_first_balances: uint256[N_COINS], _last_balances: uint256[N_COINS], _time_elapsed: uint256) -> uint256[N_COINS]:
       balances: uint256[N_COINS] = empty(uint256[N_COINS])
       for x in range(N_COINS):
           balances[x] = (_last_balances[x] - _first_balances[x]) / _time_elapsed
       return balances

3. Finally, the ts.price0Average and ts.price1Average are updated using the get_dy function and passing the freshly-obtained twapBalances.

Having understood Ubiquity’s approach to fetch the metapool’s TWAP price, it is now time to explore how a TWAP actually works. This will unveil the vulnerability that lies in Ubiquity’s code.

A TWAP (or Time-Weighted Average Price) is similar to a weighted average, where the TWAP weights price by how long the price stays at a certain level.

If the price for an asset is $5 for 2 hours and $10 for 2 more hours, the TWAP price will be $ ((52+102) / 4 = $7.5).

On the other hand, if the price for an asset is $5 for 23 hours and $10 for 1 hour, the TWAP price will be $ ((5 * 23 + 10 * 1) / 24 ≈ $5.208).

Thus, the formula for the TWAP is given by:

$\frac{P_1 T_1+P_2 T_2+\ldots+P_n T_n}{\sum_{i=1}^n T_i}$

where P is the price at a certain time and T is a certain duration (NOT timestamp).

In order to be able to fetch the TWAP prices, Curve’s metapool (like many other TWAP implementations, such as UniswapV2) does not store the price at every block.timestamp , but it rather uses price accumulators (priceCumulative fetched in Ubiquity’s code by using the currentCumulativePrices() function). This accumulators will record and aggregate the new pool’s balance ratios every time a change occurs in the Curve metapool since the pool’s inception. Checking the curve metapool code, we can see that such values are updated each time the metapool’s _update() function is called (which happens in the add_liquidity() , exchange(), exchange_underlying(), remove_liquidity(), remove_liquidity_imbalance() and remove_liquidity_one_coin() (note that unlike Uniswap V2’s snapshot() function, there is NOT a specific function here that allows the cumulative prices to be snapshotted in the metapool)).

Because Ubiquity does not want to fetch the aggregated prices since the metapool’s inception, it stores the latest update timestamp in the ts.pricesBlockTimestampLast variable, and then compares it with the newest timestamp when the price was updated in the Curve metapool (this is the timestamp stored in the temporal blockTimestamp variable).

The problem with Ubiquity’s approach fetching the TWAP: there is no validation to ensure that the metapool’s latest stored price cumulatives are actually recent and not stale. The price computed with the current approach is susceptible of being extremely deviated from the actual price that should be obtained using the TWAP. The following Proof of Concept illustrates the potential issues that might arise given the current implementation.

Proof of Concept

Imagine the following scenario, where a sequence of different prices takes place:

As mentioned before, Curve’s metapool latest price is recorded through the metapool’s _update() function (which is only called in certain situations, which happens mainly when liquidity changes in the metapool take place, BUT NOT WHEN THE CUMULATIVE PRICES ARE FETCHED), which will make the blockTimestamp returned by currentCumulativePrices() be T5, and the priceCumulative showing a price of $0.70. Let’s also say that the last timestamp and cumulative prices stored by Ubiquity was at T1.

Because Ubiquity passes blockTimestamp - ts.pricesBlockTimestampLast as the time to be considered for the TWAP computation when calling get_twap_balances(), the price will be computed in the following way, yielding an extremely stale price:

(4 * $1 + 1 * $0.70) / 5 = $0.94

However, the correct price considering the TWAP data should actually be computed in the following way:

(4 * $1 + 6 * $0.70) / 10 = $0.82

This incurs in a difference of $0.12, which is an extremely high difference considering that we are dealing with a stablecoin price.

Impact

Medium, it is probable that prices will be outdated at a certain time, leading to improper prices returned by the TWAP. Although the redeeming mechanism will be active at any time when uAD’s price is below $1, incorrectly pricing uAD might be of critical impact for the proper functioning of the stabilization mechanisms.

Code Snippet

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

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

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

Tool used

Manual Review

Recommendation

It is recommended to add a staleness check when the cumulative prices are fetched, in order to verify that the pool’s latest timestamp update was performed not too long ago. In the situation where the pool’s balances are stale, Ubiquity’s team must force an update in the metapool by interacting with it so that the protocol can keep running.

Duplicate of #13

[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