`_getUniswapTwapWei()` will show incorrect price for negative ticks cause it doesn't round up for negative ticks.

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/f742b554e18ae1a07cb8d4617ec8aa50db037c1c/src/price_feed/CoreUniswapFeed.sol#L49-L75

Vulnerability details

Proof of Concept

Take a look at https://github.com/code-423n4/2024-01-salty/blob/f742b554e18ae1a07cb8d4617ec8aa50db037c1c/src/price_feed/CoreUniswapFeed.sol#L49-L75

    // Returns amount of token0 * (10**18) given token1
    function _getUniswapTwapWei( IUniswapV3Pool pool, uint256 twapInterval ) public view returns (uint256)
        {
        uint32[] memory secondsAgo = new uint32[](2);
        secondsAgo[0] = uint32(twapInterval); // from (before)
        secondsAgo[1] = 0; // to (now)

        // Get the historical tick data using the observe() function
         (int56[] memory tickCumulatives, ) = pool.observe(secondsAgo);
        //@audit
        int24 tick = int24((tickCumulatives[1] - tickCumulatives[0]) / int56(uint56(twapInterval)));
        uint160 sqrtPriceX96 = TickMath.getSqrtRatioAtTick( tick );
        uint256 p = FullMath.mulDiv(sqrtPriceX96, sqrtPriceX96, FixedPoint96.Q96 );

        uint8 decimals0 = ( ERC20( pool.token0() ) ).decimals();
        uint8 decimals1 = ( ERC20( pool.token1() ) ).decimals();

        if ( decimals1 > decimals0 )
            return FullMath.mulDiv( 10 ** ( 18 + decimals1 - decimals0 ), FixedPoint96.Q96, p );

        if ( decimals0 > decimals1 )
            return ( FixedPoint96.Q96 * ( 10 ** 18 ) ) / ( p * ( 10 ** ( decimals0 - decimals1 ) ) );

        return ( FixedPoint96.Q96 * ( 10 ** 18 ) ) / p;
        }

This function is used to get twap price tick using uniswap oracle. it uses pool.observe() to get tickCumulatives array which is then used to calculate int24 tick.

The problem is that in case if int24(tickCumulatives[1] - tickCumulatives[0]) is negative, then the tick should be rounded down as it's done in the uniswap library.

As result, in case if int24(tickCumulatives[1] - tickCumulatives[0]) is negative and (tickCumulatives[1] - tickCumulatives[0]) % secondsAgo != 0, then returned tick will be bigger then it should be, which opens possibility for some price manipulations and arbitrage opportunities.

Impact

In case if int24(tickCumulatives[1] - tickCumulatives[0])is negative and ((tickCumulatives[1] - tickCumulatives[0]) % secondsAgo != 0, then returned tick will be bigger than it should be which places protocol wanting prices to be right not be able to achieve this goal, note that where as protocol still relies on multiple sources of price, they still come down and end on weighing the differences between the prices and reverting if a certain limit is passed, effectively causing the pricing logic to be unavailable and also reverting on important functions like CollateralAndLiquidity::liquidate() cause a call to underlyingTokenValueInUSD() is made which would not be available.

Recommended Mitigation Steps

Add this line. if (tickCumulatives[1] - tickCumulatives[0] < 0 && (tickCumulatives[1] - tickCumulatives[0]) % secondsAgo != 0) timeWeightedTick --;

Assessed type

Math

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

othernet-global (sponsor) confirmed

[othernet-global]

Now rounds down for negative ticks as suggested.

https://github.com/othernet-global/salty-io/commit/4625393e9bd010778003a1424201513885068800

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as selected for report

[othernet-global]

The stablecoin framework: /stablecoin, /price_feed, WBTC/WETH collateral, PriceAggregator, price feeds and USDS have been removed: https://github.com/othernet-global/salty-io/commit/88b7fd1f3f5e037a155424a85275efd79f3e9bf9