Price Denominator logic return wrong results if underlying assets have decimals other than 8

[code423n4]

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/oracles/PegOracle.sol#L67-L82

Vulnerability details

Impact

The project implements a price oracle in order to get the relative price between the pegged asset and the price of the original asset (example: stETH to ETH). If the ratio (the pegged asset divided by the original asset) is 1 the Token is pegged, otherwise is depegged.

Bellow is a code snippet from the PegOracle.sol function.

 if (price1 > price2) {
            nowPrice = (price2 * 10000) / price1;
        } else {
            nowPrice = (price1 * 10000) / price2;
        }

        int256 decimals10 = int256(10**(18 - priceFeed1.decimals()));
        nowPrice = nowPrice * decimals10;

        return (
            roundID1,
            nowPrice / 1000000,
            startedAt1,
            timeStamp1,
            answeredInRound1
        );
    }

To fetch the ratio at any time, the PegOracle.sol performs some calculations; first the relative price is multiplied by 1e4 and then it returns the above calculation divided by 1e6.

The Controller.sol file makes an external call to the PegOracle.sol oracle to get the relative price. After, the value returned, it is multiplied by 10**(18-(priceFeed.decimals()) and the result represents the relative price between the two assets.

The result is converted to 18 decimal points in order to be compared with the Strike Price passed by the admin on VaultFactory.sol.

While the decimal calculation logic appear to works when decimals == 8, it seems that it won't work when the decimals are different than 8. The impact is relative big, since the whole project depends on the ratio of the price oracles of the two assets.

Proof of Concept

Lets say that we have two assets, A and B, having 6 decimals. Assuming that the oracle of two assets report prices of 400 * 10 ** 6 = 400000000 and 399 * 10 ** 6 = 399000000 , their relative price transformed to 18 decimals is :
uint256(scalePriceTo18(priceA, 6) * 1e18 / scalePriceTo18(priceB, 6)) = 1002506265664160401

where scalePriceTo18 :

function scalePriceTo18(int256 _price, uint8 _priceDecimals)
        internal
        pure
        returns (int256)
    {
        if (_priceDecimals < 18) {
            return _price * int256(10 ** uint256(18 - _priceDecimals));
        } else if (_priceDecimals > 18) {
            return _price * int256(10 ** uint256(_priceDecimals - 18));
        }
        return _price;
    }

By using the formula provided in Controller.soland PegOracle.sol the result is 10025000000000000000000 which has 4 more decimals than the correct result.

The decimal calculation performed by the y2k codebase is :
x / y * (10 ^ 4 * 10 ^ 12 * 10 ^12) / (10 ^ 6) = (x * 10 ^ 22 / y)

Tools Used

Manual Code Review

Recommended Mitigation Steps

Since the 2 assets are required to having the same amount of decimals a formula that transforms the relative price to 1e18 could be:
x * 1e18 / y .

An example that Chainlink implements, that includes a scalePrice function, in order to find a different price denominator could be found here

[HickupHH3]

dup of #195