Time weighted average price is wrongly implemented

Summary

Time weighted average price is wrongly implemented and can easily be manipulated.

Vulnerability Detail

For twap to be resistant to price manipulation the observation window should be large enough because it calculates averages price within this window. To manipulate such oracle you need to mantain manipulated spot price significant part of the observation window, which is considered very expensive if observation window and liquidity in pool is enough due to arbitrageurs constantly trying to bring price back for profit.

But in LibTWAPOracle implementation observation window it could be as little as 1 second. (block time to be precise, on mainnet ~12 sec)

// @issue this time window could be not enough to prevent manipulation
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,
            // @issue using too short observation window
            blockTimestamp - ts.pricesBlockTimestampLast
        );

    ...
}

Impact

Price can be easely manipulated. This could dos mint/redeem because of mintPriceThreshold/redeemPriceThreshold checks and harm other parts of protocol that uses twap oracle.

Code Snippet

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

Tool used

Manual Review

Recommendation

Curve pool do not store old observations for cummalative balances, only current values. So i think you have to store them in your contract and then use for twap calculation at least X seconds old observaation along with current from pool (something similar to uniswap twap implementation with observations array)

Duplicate of #20

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