TWAP `duration` for Uniswap Oracle should be at least `30 mins`

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-06-vultisig/blob/cb72b1e9053c02a58d874ff376359a83dc3f0742/hardhat-vultisig/contracts/oracles/uniswap/UniswapV3Oracle.sol#L38-L46

Vulnerability details

Impact

The peek() function in UniswapV3Oracle is designed to returns TWAP price for 1 VULT for the last 30 mins. However, this may not be the case in all instances as the longestPeriod may sometimes be less than PERIOD in which case, the function will use the longestPeriod to get tick and hence the TWAP.

This puts UniswapV3Oracle at a higher risk of price manipulation when liquidity of the pool is not spread over a wide range. This could occur during protocol launch where the Uniswap pool has limited liquidity.

Vulnerability details

Here is the peek():

    /// @notice Returns TWAP price for 1 VULT for the last 30 mins
    function peek(uint256 baseAmount) external view returns (uint256) {
>>      uint32 longestPeriod = OracleLibrary.getOldestObservationSecondsAgo(pool);
>>      uint32 period = PERIOD < longestPeriod ? PERIOD : longestPeriod;
        int24 tick = OracleLibrary.consult(pool, period);
        uint256 quotedWETHAmount = OracleLibrary.getQuoteAtTick(tick, BASE_AMOUNT, baseToken, WETH);
        // Apply 5% slippage
        return (quotedWETHAmount * baseAmount * 95) / 1e20; // 100 / 1e18
    }

As seen, the function uses the smallest between PERIOD and longestPeriod. If:

1. PERIOD < longestPeriod, period == PERIOD
2. PERIOD > longestPeriod, period == longestPeriod

Now, when PERIOD < longestPeriod, the function will use 30 mins as duration since PERIOD is hardcoded as 30 mins.

 /// @notice TWAP period
    uint32 public constant PERIOD = 30 minutes;

However, when PERIOD > longestPeriod, the period will be less than 30 mins (It could even be 10mins) which below the standard and also goes against the intended function if the peek() function which should Return TWAP price for 1 VULT for the last 30 mins.

One may argue that such price manipulation is risky for the attacker, as the attacker has to use their own capital (instead of flash loan) to keep the price manipulated for more than a block, making them vulnerable to arbitrage. But that is not a total deterrence as shown in Rari's Fuse hack, where the attacker risked their capital and waited for multiple blocks. The root cause of that hack was due to price manipulation of the Uniswap V3 TWAP oracle, which had a TWAP duration of 600 secs and the Uniswap pool did not have liquidity over a wide range.

Tools Used

Manual Review

Recommended Mitigation Steps

It would be more appropriate to use at least 30 mins as duration for the TWAP

    function peek(uint256 baseAmount) external view returns (uint256) {
-       uint32 longestPeriod = OracleLibrary.getOldestObservationSecondsAgo(pool);
-       uint32 period = PERIOD < longestPeriod ? PERIOD : longestPeriod;
+       uint32 period = PERIOD;
        int24 tick = OracleLibrary.consult(pool, period);
        uint256 quotedWETHAmount = OracleLibrary.getQuoteAtTick(tick, BASE_AMOUNT, baseToken, WETH);
        // Apply 5% slippage
        return (quotedWETHAmount * baseAmount * 95) / 1e20; // 100 / 1e18
    }

Assessed type

Uniswap

[Tigerfrake]

Hello judge, kindly walk with me here:

My report here tries to handle two cases but both seem to have been overlooked:

1. The peek() function in UniswapV3Oracle is designed to returns TWAP price for 1 VULT for the last 30 mins but this will not be the case everytime.
2. TWAP duration for Uniswap Oracle should be at least 30 mins

Break down:

case1

When PERIOD is greater than longestPeriod, price is returned based on this duration (longestPeriod).

        uint32 longestPeriod = OracleLibrary.getOldestObservationSecondsAgo(pool);
>>      uint32 period = PERIOD < longestPeriod ? PERIOD : longestPeriod;
>>      int24 tick = OracleLibrary.consult(pool, period);

Let's say longestPeriod fetched is 5 minutes. The price returned by peek() function will not be a 30 minutes TWAP but a 5 minutes TWAP. This is not the price required or that the function is designed to return.

case2

According to Uniswap, 30 minutes is the minimum duration utilized since spot price can easily be manipulated

Because spot prices are easy and relatively cheap to manipulate, most protocols use a rolling 30-minute time window for TWAP to calculate the price. See this finding. Here is the reference blog

[alex-ppg]

Hey @Tigerfrake, thank you for the PJQA contribution. I will preface all validation repository finding responses by stating that they are not evaluated by judges directly and are only evaluated by the validators if they are deemed unsatisfactory.

After reviewing the submission, it is indeed correct. Specifically, it will solely arise if the Uniswap V3 VULT/WETH pair has had an insufficient cardinality configured.

In relation to impact, the sole point that integrates with the oracle is the enforcement of a maximum cap per address as seen here. The inline documentation right above it states it is a rough estimate, further evidenced by the 5% reduction imposed by the Oracle itself.

While the finding has been incorrectly invalidated, it is deemed to be of QA (L) severity and thus ineligible for a pie of the HM reward pool. It is unrealistic to expect even a small TWAP to be manipulated through more than 1 block for the purposes of bypassing an investment limit in Vultisig.

This paragraph is included in all of my responses and confirms that no further feedback is expected in this submission as PJQA has concluded.