The TWAPOracleFacet TWAP timeframe depends on the distance from the last update() call, which compromises the price accuracy and enables price manipulation..

Summary

The TWAPOracleFacet TWAP timeframe depends on the distance from the last update() call which can be either too short or too long for an acceptable TWAP price. This also opens the door to price manipulation given that the update() function is accessible to all.

Vulnerability Detail

The update logic of TWAPOracleFacet is to check if the current latest observation (cumulativePrice) is later than the most recent observation on the last update() call, and if so, to calculate the TWAP between the previous last observation and the current one:

(
            uint256[2] memory priceCumulative,
            uint256 blockTimestamp
        ) = currentCumulativePrices();
        if (blockTimestamp - ts.pricesBlockTimestampLast > 0) {
           uint256[2] memory twapBalances = IMetaPool(ts.pool)
                .get_twap_balances(
                    ts.priceCumulativeLast,
                    priceCumulative,
                    blockTimestamp - ts.pricesBlockTimestampLast
                );
            ...
            ...    
           //update prices and last observation/last timestamp
        }

This logic causes to twap timeframe (and consequently its accuracy and reliability) to be dependant on the time of the previous update() calls (which can be called by anyone). This means the timeframe might be either too long (if update was not called for a very long period of time) or too short (if update is called too frequently). This also enables anyone to manipulate the twap. For example: If the price is constant for a long period of time around 1.02, then drops to 0.5 at block X, someone can call update() at block X immediately after the price change, then perform a very small trade on block X+1 - to trigger a new observation of roughly the same price and for a low cost - and then call update again immediately after in block X+1. This will cause the TWAPOracle to report 0.5 as the TWAP price, one block after the change.

Impact

TWAP price might be significantly inaccurate (due to an excessively short/long timeframe), either unintentionally due to the timimg of update() calls or as a deliberate manipulation as shown above. This can disrupt core functionality such as the enabling/disabling of Mint or Redeem operations.

Code Snippet

TWAPOracleDollar3poolFacet update():
https://github.com/sherlock-audit/2023-12-ubiquity/blob/d9c39e8dfd5601e7e8db2e4b3390e7d8dff42a8e/ubiquity-dollar/packages/contracts/src/dollar/facets/TWAPOracleDollar3poolFacet.sol#L32

LibTWAPOracle update():
https://github.com/sherlock-audit/2023-12-ubiquity/blob/d9c39e8dfd5601e7e8db2e4b3390e7d8dff42a8e/ubiquity-dollar/packages/contracts/src/dollar/libraries/LibTWAPOracle.sol#L68

MintDollar dependency on TWAP:
https://github.com/sherlock-audit/2023-12-ubiquity/blob/d9c39e8dfd5601e7e8db2e4b3390e7d8dff42a8e/ubiquity-dollar/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol#L347

RedeemDollar dependency on TWAP:
https://github.com/sherlock-audit/2023-12-ubiquity/blob/d9c39e8dfd5601e7e8db2e4b3390e7d8dff42a8e/ubiquity-dollar/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol#L419

Tool used

Manual Review

Recommendation

One option might be saving an array of observations (say the last 7-8) and have update() calculate the TWAP from the current observation to the one closest to be a pre-set time gap from now (i.e. 8 hours). Probably should couple that with some offchain service that calls update() at the selected frequency to make sure there are recent enough observations logged in the TWAPOracle.

Duplicate of #20

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