sherlock-audit / 2024-04-teller-finance-judging

6 stars 6 forks source link

no - The TWAP price without checking the liquidity and TWAP duration can lead to price manipulation in `LenderCommitmentGroup_Smart` #280

Closed sherlock-admin4 closed 2 months ago

sherlock-admin4 commented 2 months ago

no

medium

The TWAP price without checking the liquidity and TWAP duration can lead to price manipulation in LenderCommitmentGroup_Smart

Summary

The TWAP price without checking the liquidity and TWAP duration can lead to price manipulation in LenderCommitmentGroup_Smart

Vulnerability Detail

LenderCommitmentGroup_Smart uses a single price feed based on Uniswap V3 TWAP oracle for principalToken/collateralToken pool. However, there is no checking for the pool' liquidity or TWAP duration.

   function _getUniswapV3TokenPairPrice(uint32 _twapInterval)
        internal
        view
        returns (uint256)
    {
        // represents the square root of the price of token1 in terms of token0

        uint160 sqrtPriceX96 = getSqrtTwapX96(_twapInterval);

        //this output is the price ratio expanded by 1e18
        return _getPriceFromSqrtX96(sqrtPriceX96);
    }
    function getSqrtTwapX96(uint32 twapInterval)
        public
        view
        returns (uint160 sqrtPriceX96)
    {
        if (twapInterval == 0) {
            // return the current price if twapInterval == 0
            (sqrtPriceX96, , , , , , ) = IUniswapV3Pool(UNISWAP_V3_POOL)
                .slot0();
        } else {
            uint32[] memory secondsAgos = new uint32[](2);
            secondsAgos[0] = twapInterval; // from (before)
            secondsAgos[1] = 0; // to (now)

            (int56[] memory tickCumulatives, ) = IUniswapV3Pool(UNISWAP_V3_POOL)
                .observe(secondsAgos);

            // tick(imprecise as it's an integer) to price
            sqrtPriceX96 = TickMath.getSqrtRatioAtTick(
                int24(
                    (tickCumulatives[1] - tickCumulatives[0]) /
                        int32(twapInterval)
                )
            );
        }
    }

The TWAP duration is lower than typical 1,800 secs (30mins) that is used by Euler[1] and Uniswap[2] in their studies. This puts LenderCommitmentGroup_Smart at a higher risk of price manipulation when liquidity of the principalToken/collateralToken pool is not spread over a wide range. This could occur during protocol launch where the Uniswap pool has limited liquidity.

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[3], 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.

References

[1] https://docs.euler.finance/euler-protocol/eulers-default-parameters#twap-length [2] https://blog.uniswap.org/uniswap-v3-oracles [3] https://cmichel.io/replaying-ethereum-hacks-rari-fuse-vusd-price-manipulation/

Impact

This lacks any checks as to whether there is enough liquidity in the Uniswap pool to guarantee data fidelity and the TWAP duration, meaning there is a higher likelihood of price manipulation.

Code Snippet

https://github.com/sherlock-audit/2024-04-teller-finance/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L538C4-L550C6

https://github.com/sherlock-audit/2024-04-teller-finance/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L570C1-L595C6

Tool used

Manual Review

Recommendation

Set the LenderCommitmentGroup_Smart Uniswap V3 Oracle TWAP duration to be at least 30 mins. Note that it is also important to ensure the principalToken/collateralToken pool liquidity is spread over a wide range to increase the attack cost.

Duplicate of #119