c4-bot-7 commented 9 months ago

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/price_feed/CoreUniswapFeed.sol#L112

Vulnerability details

Impact

The internal oracle price system uses three different price feeds and aggregates them into one by taking the average price of the two closest prices.

The three different price feeds are chainlink, uniswap TWAP, and the internal balances and these are used to calculate the price of ETH and BTC in USD. Inside the uniswap TWAP calculation to get the BTC price a wrong calculation leads to a very low BTC price. Here we can see this function:

function getTwapWBTC( uint256 twapInterval ) public virtual view returns (uint256)
    {
    uint256 uniswapWBTC_WETH = getUniswapTwapWei( UNISWAP_V3_WBTC_WETH, twapInterval );
    uint256 uniswapWETH_USDC = getUniswapTwapWei( UNISWAP_V3_WETH_USDC, twapInterval );

    // Return zero if either is invalid
    if ((uniswapWBTC_WETH == 0) || (uniswapWETH_USDC == 0 ))
        return 0;

    if ( wbtc_wethFlipped )
        uniswapWBTC_WETH = 10**36 / uniswapWBTC_WETH;

    if ( ! weth_usdcFlipped )
        uniswapWETH_USDC = 10**36 / uniswapWETH_USDC;

    return ( uniswapWETH_USDC * 10**18) / uniswapWBTC_WETH;
    }

As we can see it uses two TWAP prices from uniswap. The WETH / USDC and the WBTC / WETH pool. The reason for that is that the TVL in these pools is bigger than the TVL in the WBTC / USDC pool. The last calculation is the problem. It divides the WETH in USDC price by the WBTC in WETH price while it should be multiplied instead.

Here we can see a calculation that showcases the problem:

Example values:
1 ETH = 2000 USDC
1 BTC = 50000 USDC
Therefore 1 BTC = 25 ETH

Current calculation = ( uniswapWETH_USDC * 10**18) / uniswapWBTC_WETH = WETH_USDC / WBTC_WETH = 2000 / 25 = 80

Correct calculation = WETH_USDC * WBTC_WETH = 2000 * 25 = 50000

Therefore manipulating the oracle price becomes way easier as the price aggregator will always use the average of the chainlink price and the internal balance because the uniswap TWAP price of BTC will always be very off. Therefore the attacker just needs to manipulate the reserves of the salty pool to manipulate the whole oracle price and this should be possible with a flash loan attack.

Also, the price aggregator reverts if two different price feeds return 0, but with this bug, it will revert if only one of the price feeds returns 0 for BTC as it will take the average of the very small BTC price and the 0 price instead of the two not 0 prices. This will always revert because of this check (_absoluteDifference(priceA, priceB) * 100000) / averagePrice > maximumPriceFeedPercentDifferenceTimes1000 ). Therefore if any price feed returns 0 a DoS of the BTC oracle system occurs.

Proof of Concept

The following POC can be implemented in the CoreUniswapFeed.t.sol test file:

function testPOC() public {
    address _wbtc = address(0x2260FAC5E5542a773Aa44fBCfeDf7C193bc2C599);
    address _weth = address(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2);
    address _usdc = address(0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48);

    testUniswapFeed = new TestUniswapFeed( IERC20(_wbtc), IERC20(_weth), IERC20(_usdc), UNISWAP_V3_BTC_ETH, UNISWAP_V3_USDC_ETH );

    // Check if WBTC/WETH order is correctly determined
    bool expectedWbtcWethFlipped = address(_weth) < address(_wbtc);
    assertEq(testUniswapFeed.wbtc_wethFlipped(), expectedWbtcWethFlipped, "WBTC/WETH order incorrectly determined in constructor");

    // Check if WETH/USDC order is correctly determined
    bool expectedWethUsdcFlipped = address(_usdc) < address(_weth);
    assertEq(testUniswapFeed.weth_usdcFlipped(), expectedWethUsdcFlipped, "WETH/USDC order incorrectly determined in constructor");

    // Set forced TWAPs
    uint256 forcedTWAP_WBTC_WETH = 2 ether; // BTC should be worth 2 ETH
    uint256 forcedTWAP_WETH_USDC = 3 ether;
    testUniswapFeed.setTwapWBTC_WETH(forcedTWAP_WBTC_WETH);
    testUniswapFeed.setTwapWETH_USDC(forcedTWAP_WETH_USDC);

    // Get TWAPs
    uint256 twapWBTC = testUniswapFeed.getTwapWBTC(1);
    uint256 twapWETH = testUniswapFeed.getTwapWETH(1);

    // the opposite was calculated, BTC is worth way less than ETH
    assertGt(twapWETH, twapWBTC * 2 - 1);
}

Tools Used

Manual Review

Recommended Mitigation Steps

Change the last calculation inside the getTwapWBTC function to multiply instead of divide.

Assessed type

Math

c4-judge commented 8 months ago

Picodes marked the issue as primary issue

c4-sponsor commented 8 months ago

othernet-global (sponsor) disputed

othernet-global commented 8 months ago

The calculations work as expected.

Note the line:

if ( ! weth_usdcFlipped ) uniswapWETH_USDC = 10**36 / uniswapWETH_USDC;

...which essentially results in us using USDS/WETH as the value.

So then the final calculation: ( uniswapWETH_USDC * 10**18) / uniswapWBTC_WETH;

is really ( USDC/WETH) / (WBTC/WETH).

Note that the foundry test is incorrect because the forced values need to be flipped if the pairs are flipped. uint256 forcedTWAP_WBTC_WETH = 2 ether; // BTC should be worth 2 ETH uint256 forcedTWAP_WETH_USDC = 3 ether;