dimulski - The MixOracle::getPrice() function will revert for some pairs, and return completely incorrect price in the rest of the cases

[sherlock-admin2]

dimulski

High

The MixOracle::getPrice() function will revert for some pairs, and return completely incorrect price in the rest of the cases

Summary

The MixOracle.sol contract tries to combine prices from Uniswap V2 pairs with prices from the Pyth Network. However for certain pairs the getThePrice() function will revert due to truncation when division is utilized in solidity.

    function getThePrice(address tokenAddress) public returns (int) {
        // get tarotOracle address
        address _priceFeed = AttachedTarotOracle[tokenAddress];
        require(_priceFeed != address(0), "Price feed not set");
        require(!isPaused, "Contract is paused");
        ITarotOracle priceFeed = ITarotOracle(_priceFeed);

        address uniswapPair = AttachedUniswapPair[tokenAddress];
        require(isFeedAvailable[uniswapPair], "Price feed not available");
        // get twap price from token1 in token0
        (uint224 twapPrice112x112, ) = priceFeed.getResult(uniswapPair);
        address attached = AttachedPricedToken[tokenAddress];

        // Get the price from the pyth contract, no older than 20 minutes
        // get usd price of token0
        int attachedTokenPrice = IPyth(debitaPythOracle).getThePrice(attached);
        uint decimalsToken1 = ERC20(attached).decimals();
        uint decimalsToken0 = ERC20(tokenAddress).decimals();

        // calculate the amount of attached token that is needed to get 1 token1
        int amountOfAttached = int(
            (((2 ** 112)) * (10 ** decimalsToken1)) / twapPrice112x112
        );

        // calculate the price of 1 token1 in usd based on the attached token
        uint price = (uint(amountOfAttached) * uint(attachedTokenPrice)) /
            (10 ** decimalsToken1);

        require(price > 0, "Invalid price");
        return int(uint(price));
    }

The priceFeed.getResult() returns the TWAP for reserve1/resrve0, however later in amoutOfAttached some very strange calculation is performed. Lets consider that reserve1 is WETH which will also be the tokenAddress, and USDC is the attached token and reserve0 respectively. Now if the comment above the calculation of amountOfAttached is correct, that means the purpose of this calculation is to get how much WETH(token1) we can buy with one USDC(token0). To get that we have to divide the twapPrice112x112 by 2**112, not the other way around. And if we want to account for decimals we have to first multiply the twapPrice112x112 by the decimals of the attached token, USDC in this case which has 6 decimals. It is hard to get into the developers head and understand what he wanted to accomplish with this function. But let's consider an example where this function will revert:

• reserve1 is WETH assume the price is 3_000$, reserve0 is USDC. In the pair we have 1e18 WETH and 3_000e6 USDC.
• priceFeed.getResult(uniswapPair) will return (1e18 * (2**112)) / 3_000e6 = 1772303994379887830538409413707126101333333333
• when this number is utilized for the calculation of amountOfAttached the result will be 0, because the divisor will be bigger than the 2112 * 106 multiplication result. From there on the price will also be 0 and then the function will revert.

If the UniV2 pair has 2 tokens with 18 decimals, then the function won't revert but the price returned from it will be completely incorrect. Which will be even worse for the borrowers or lenders that utilize this price feed.

Root Cause

It is pretty hard to grasp the logic behind the implementation of the getThePrice() function. However it seems that the problem is in the way amountOfAttached is calculated

        // calculate the amount of attached token that is needed to get 1 token1s
        int amountOfAttached = int(
            (((2 ** 112)) * (10 ** decimalsToken1)) / twapPrice112x112
        );

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

No response

Impact

The getThePrice() function doesn't work correctly, it will either return a wrong price or revert. If a wrong price is returned it will be catastrophic for the protocol.

PoC

No response

Mitigation

No response