TWAP oracle is manipulable as the only time-window requirement is `blockTimestamp

Summary

TWAP oracle is more manipulable because its time-window can be as short as one block time.

Vulnerability Detail

TWAP oracle are used to make it exponentially expensive to manipulate the price. This is done by not getting the spot price of a pool, but rather the time weighted averaged price over a period of time. The longer the period, the harder it is to manipulate the price (but the laggier is the price representation)

The update function in LibTWAPOracle can be called once every block as the only requirement is the time difference between to consecutive calls is >0. Not only it is called everytime mintDollar or redeemDollar is called (reducing the time-window to the time between two calls of these function), as it is also a public function in the TWAPOracleDollar3poolFacet, so anyone can control the time-window. This means the price is calculated between block N and block N-1, which really reduce the impact and goal of a TWAP here.

Impact

The TWAP advantage is not used correctly, increasing the feasibility of an attack based on Dollar price manipulation. The LibUbiqityPool.getDollarPriceUsd() make use of these prices here and here, which is used as a protection mechanism for the Dollar peg. We can also easily imagine in the future that this TWAP oracle can be used for other functions, which will then be vulnerable to a price manipulation.

Code Snippet

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

File: src\dollar\libraries\LibTWAPOracle.sol
69:     function update() internal {
70:         TWAPOracleStorage storage ts = twapOracleStorage(); 
71:         (
72:             uint256[2] memory priceCumulative,
73:             uint256 blockTimestamp
74:         ) = currentCumulativePrices();
75: @>      if (blockTimestamp - ts.pricesBlockTimestampLast > 0) { 
76:             // get the balances between now and the last price cumulative snapshot
77:             uint256[2] memory twapBalances = IMetaPool(ts.pool)         
78:                 .get_twap_balances(             
79:                     ts.priceCumulativeLast,
80:                     priceCumulative,
81: @>                  blockTimestamp - ts.pricesBlockTimestampLast    
82:                 );

Tool used

Manual Review

Recommendation

Use a configurable time period, and either update the TWAP price once very period, or use a sliding window. Uniswap propose examples here

Duplicate of #20

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