NoyaValueOracle.getValue returns an incorrect price when a multi-token route is used

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-04-noya/blob/9c79b332eff82011dcfa1e8fd51bad805159d758/contracts/helpers/valueOracle/NoyaValueOracle.sol#L89

Vulnerability details

The NoyaValueOracle.getValue is used to convert the price of an asset to baseToken, using token routing when necessary.

If there is no oracle for two tokens, a route needs to be set up between them. This route can then be used to get a value using NoyaValueOracle.getValue.

However, there's a mistake in the NoyaValueOracle.getValue function that makes it give the wrong price.

Impact

The NoyaValueOracle.getValue will return incorrect prices for tokens when the configured priceRoutes[asset][baseToken].length is more than 1.

Proof of Concept

To set a price path for a token pair, the maintainer should use NoyaValueOracle.updatePriceRoute.

For instance, for the SOL to UNI conversion. 1 SOL is roughly equal to 21 UNI, but there's no direct oracle. So, the maintainer sets a route SOL → BNB → ETH → UNI.

When _getValue tries to convert 100 SOL into UNI, it should first convert SOL → BNB, then BNB → ETH, and lastly ETH → UNI. But, what _getValue really does is to convert SOL → BNB, SOL → ETH, and SOL → UNI.

You can see this error in the _getValue function, where asset is used each time instead of quotingToken.

    function _getValue(address asset, address baseToken, uint256 amount, address[] memory sources)
        internal
        view
        returns (uint256 value)
    {
        uint256 initialValue = amount;
        address quotingToken = asset;
        for (uint256 i = 0; i < sources.length; i++) {
>           initialValue = _getValue(asset, sources[i], initialValue);
            quotingToken = sources[i];
        }
        return _getValue(quotingToken, baseToken, initialValue);
    }

The first problem comes up if the SOL → BNB, SOL → ETH, and SOL → UNI oracles are not set up. This will cause a revert of _getValue.

    function _getValue(address quotingToken, address baseToken, uint256 amount) internal view returns (uint256) {
        INoyaValueOracle oracle = priceSource[quotingToken][baseToken];
        if (address(oracle) == address(0)) {
            oracle = priceSource[baseToken][quotingToken];
        }
        if (address(oracle) == address(0)) {
            oracle = defaultPriceSource[baseToken];
        }
        if (address(oracle) == address(0)) {
            oracle = defaultPriceSource[quotingToken];
        }
        if (address(oracle) == address(0)) {
>           revert NoyaOracle_PriceOracleUnavailable(quotingToken, baseToken);
        }
        return oracle.getValue(quotingToken, baseToken, amount);
    }

Even if we configure these oracles, the calculated value will still be incorrect.

Consider this example - the _getValue is called with the following parameters:

• asset = SOL
• baseToken = UNI
• amount = 100
• sources = [BNB, ETH]

This will convert 100 SOL to UNI, using BNB and ETH as the route.

Iteration	initialValue	quotingToken	asset	sources[i]	initialValue’	quotingToken’
1	100	SOL	SOL	BNB	100SOL ≈ 25BNB	BNB
2	25	BNB	SOL	ETH	25BNB ≈ 1.25ETH	ETH

The result is calculated using quotingToken = ETH, baseToken = UNI, and the initialValue = 1.25, which returns 526.

However, the expected result should be 2094.

Tools Used

Manual Review

Recommended Mitigation Steps

Replace asset with quotingToken:

    function _getValue(address asset, address baseToken, uint256 amount, address[] memory sources)
        internal
        view
        returns (uint256 value)
    {
        uint256 initialValue = amount;
        address quotingToken = asset;
        for (uint256 i = 0; i < sources.length; i++) {
-           initialValue = _getValue(asset, sources[i], initialValue);
+           initialValue = _getValue(quotingToken, sources[i], initialValue);
            quotingToken = sources[i];
        }
        return _getValue(quotingToken, baseToken, initialValue);
    }

Assessed type

Oracle

[c4-pre-sort]

DadeKuma marked the issue as primary issue

[c4-pre-sort]

DadeKuma marked the issue as sufficient quality report

[HadiEsna]

true, needs to be fixed

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[c4-judge]

gzeon-c4 marked the issue as selected for report

[HadiEsna]

fix in 66d4104e83cc1b68ab117c78c4a92eafac86341c