sherlock-admin commented 8 months ago

osmanozdemir1

high

The TWAP logic is incorrect and even the updated prices are not actually up to date

Summary

Protocol uses Curve TWAP's to determine UbiquityDollarToken price. However, the update function in the oracle contract does not return the current value, but returns the last updated value. The balances and the time between the last update in the metapool and "now" is missed due to not invoking an update in the underlying Curve metapool.

Vulnerability Detail

This protocol plans to use the Curve protocol's TWAP functionality to determine the UbiquityDollarToken's price and will deploy Dollar-3CRVLP Metapool for this purpose.

Let's check LibTWAPOracle::update() function:

    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
                );

            // skipped for brevity   

            // we update the priceCumulative
            ts.priceCumulativeLast = priceCumulative;
            ts.pricesBlockTimestampLast = blockTimestamp;
        }
    }

It gets the latest cumulative prices with currentCumulativePrices() function and calculates balances by calling the metapool.

Here is the currentCumulativePrices() function:

    function currentCumulativePrices()
        internal
        view
        returns (uint256[2] memory priceCumulative, uint256 blockTimestamp)
    {
        address metapool = twapOracleStorage().pool;
        priceCumulative = IMetaPool(metapool).get_price_cumulative_last(); //@audit this function returns latest "updated" price, not the actual latest price. It doesn't update the metapool. 
        blockTimestamp = IMetaPool(metapool).block_timestamp_last();
    }

Unfortunately, this function does not return the current cumulative prices. It does not invoke the metapool to update prices, it only returns the latest updated prices.

I'll explain it a little further, but we have to check the underlying metapool for that.

Protocol's previous metapool adress is: https://etherscan.io/address/0x20955CB69Ae1515962177D164dfC9522feef567E#code
The implementation contract is: https://etherscan.io/address/0x5f890841f657d90e081babdb532a05996af79fe6#code

The underlying metapool has an internal _update() function which updates the balances and the price_cumulative_last.

// Curve metapool contract
@internal
def _update():
    """
    Commits pre-change balances for the previous block
    Can be used to compare against current values for flash loan checks
    """
    elapsed_time: uint256 = block.timestamp - self.block_timestamp_last
    if elapsed_time > 0:
        for i in range(N_COINS):
            _balance: uint256 = self.balances[i]
            self.price_cumulative_last[i] += _balance * elapsed_time
            self.previous_balances[i] = _balance
        self.block_timestamp_last = block.timestamp

These values are crucial for TWAP functionality.

Here is the most important part: The curve metapool implementation calls _update() function in the beginning of major functions, not after. This way all exchanges/swaps/deposits etc happens right after the update. Prices are not updated up until to the next action in the metapool.

For example (add liquidity function in the metapool):

// Curve metapool
@external
@nonreentrant('lock')
def add_liquidity(
    _amounts: uint256[N_COINS],
    _min_mint_amount: uint256,
    _receiver: address = msg.sender
) -> uint256:
    """
    @notice Deposit coins into the pool
    @param _amounts List of amounts of coins to deposit
    @param _min_mint_amount Minimum amount of LP tokens to mint from the deposit
    @param _receiver Address that owns the minted LP tokens
    @return Amount of LP tokens received by depositing
    """
--> self._update() //@audit first thing that happens it the update.

    // rest of the code.

So, LibTWAPOracle contract's both update() and the currentCumulativePrices() functions does not include the effects of the latest transaction in the underlying metapool. It is not updated to

Balance changes created by the last transaction in the metapool and the elapsed time from the last transaction to the current timestamp have not been updated.

/*   Timeline  

   -------------|-------------------------|--------------------|
                |                         |                    |
                |                         |                    |
               T1                         T2                   T3
          Stored prices            Last "_update"          The time when 
       in diamond contract        in the underlying     LibTWAPOracle.update()
                                       metapool             is called 

                |-------------------------|--------------------|
                   These part will be  
                      included to            !***  MISSED ***!
                    TWAP calculation
*

I wanted to visualise it. The time and balances between T2 and T3 is never included into the calculation.

The amount of miscalculation depends on the time gap between T2 and T3. If the underlying metapool is not extremely active, there will be severe miscalculations. For example, there are days/months between transactions in the protocol's previous metapool's transaction activity.

For these TWAP prices to be accurate, the protocol must invoke the _update() function in the underlying metapool just before returning the TWAP prices.

These TWAP prices are used during minting and redeeming UbiquityDollarToken and there are price threshold for these. This issue will cause incorrect amount of tokens being minted or redeemed. Also it will cause minting/redeeming tokens even though the actual price is not met the threshold.

Example scenario:

Let's assume Dollar token price is 1.01 at the moment and it is correct.
Alice performed an exchange in the underlying Dollar - 3CRVLP metapool. Note: _update was performed right in the beginning this exchange and the price is 1.01
There were no exchanges in the underlying metapool for 2 hours.
Bob wanted to mint DollarToken with mintDollar function.
This function called LibTWAPOracle.update() function.
It doesn't invoke the _update in the metapool and the returned cumulative prices are stale.
getDollarPriceUsd returns 1.01 as the current price.
Token is minted.
However, in reality, Alice's transaction 2 hours ago decreased the price to 1.005. But this price update is not performed yet. It will only be updated if there is another exchange in the underlying metapool.

Note: By the way, I think the root cause of this issue in the audit list page is also the problem I explained above. I didn't deep dive into that but it is a fork test using the Curve-LUSD metapool, and that metapool has days between exchange actions (sometimes up to 2 weeks). Therefore get_price_cumulative_last is not up to date.

Impact

TWAP prices are incorrect.
DollarToken's may be minted or redeemed outside of the price threshold.
Incorrect amounts of tokens will be minted.

Code Snippet

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

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

Tool used

Manual Review, Etherscan
This tweet thread by Daniel Von Fange is also helpful even though it is not the same thing.

Recommendation

For TWAP prices to be accurate and up to date, the protocol must invoke the _update() function in the underlying metapool contract. However, that function is an internal function and there is no elegant way to invoke it. It is only called in the beginning of important functions. This protocol may try to call one of these exchange functions with 0 amounts but I'm not sure whether is possible or right way to do. But it is clear that using the Curve metapools for TWAP comes with issues and not updating it is worse.

Duplicate of #13

sherlock-admin2 commented 7 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