Coinstein - ChainlinkPriceFeeds#getTwoFeedPriceMul should not revert for certain output decimals

[sherlock-admin2]

Coinstein

medium

ChainlinkPriceFeeds#getTwoFeedPriceMul should not revert for certain output decimals

Summary

ChainlinkPriceFeeds#getTwoFeedPriceMul should not revert for certain output decimals

Vulnerability Detail

See ChainlinkPriceFeeds#getTwoFeedPriceDiv

uint256 numeratorPrice = _getFeedPrice(
            params.firstFeed,
            uint256(params.firstUpdateThreshold),
            firstFeedDecimals,
            outputDecimals_
        );
        uint256 denominatorPrice = _getFeedPrice(
            params.secondFeed,
            uint256(params.secondUpdateThreshold),
            secondFeedDecimals,
            outputDecimals_
        );

        // Convert to numerator/denominator price and return
        uint256 priceResult = numeratorPrice.mulDiv(10 ** outputDecimals_, denominatorPrice);

The system converts to outputDecimals before getting the numeratorPrice and denominatorPrice. Because of this early conversion, the priceResult which could be a normal number can become 0 or throw error because of a division by 0 error.

Impact

The system could calculate and return correct price result. However, due to early decimal conversion issues, 0 values or division errors would happen instead.

Code Snippet

https://github.com/sherlock-audit/2023-11-olympus/blob/main/bophades/src/modules/PRICE/submodules/feeds/ChainlinkPriceFeeds.sol#L288-L299

Tool used

Manual Review and test unit

Recommendation

Change ChainlinkPriceFeeds#getTwoFeedPriceDiv

        // Get prices from feeds
        uint256 numeratorPrice = _getFeedPrice(
            params.firstFeed,
            uint256(params.firstUpdateThreshold),
            firstFeedDecimals,
-            outputDecimals_
+.         firstFeedDecimals
        );
        uint256 denominatorPrice = _getFeedPrice(
            params.secondFeed,
            uint256(params.secondUpdateThreshold),
            secondFeedDecimals,
-            outputDecimals_
+.          secondFeedDecimals
        );

+    numeratorPrice = numeratorPrice.mulDiv(outputDecimals_, firstFeedDecimals);
+    denominatorPrice = denominatorPrice.mulDiv(outputDecimals_, secondFeedDecimals);

Added a test unit to ChainlinkPriceFeed.t.sol

    function myTest() public {
        bytes memory params = encodeTwoFeedParams(
            ohmEthPriceFeed,
            UPDATE_THRESHOLD,
            daiEthPriceFeed,
            UPDATE_THRESHOLD
        );
        {
            uint256 priceInt = chainlinkSubmodule.getTwoFeedPriceDiv(
                address(0),
                2,
                params
            );
        }
    }

Error thrown in old code, but 1000 is returned in the new code

[nevillehuang]

Invalid, insufficient proof/example to show how this is possible. Additionally, both numeratorPrice and denominatorPrice is first both scaled to outputDecimals, so returning 0 is unlikely if not not possible.

[juntzhan]

Escalate.

price feed decimals is 18 denominator token price returned by price feed is 1e16 denominator token price returned by price feed is 1e15 outputDecimals is 2 denominatorPrice is 1e15 * 1e2 / 1e18 = 0

[sherlock-admin2]

Escalate.

price feed decimals is 18 denominator token price returned by price feed is 1e16 denominator token price returned by price feed is 1e15 outputDecimals is 2 denominatorPrice is 1e15 * 1e2 / 1e18 = 0

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[nevillehuang]

Based on supported tokens in contest details and this comment in #96 highlighting outputDecimals is not user controlled, outputDecimals will never be 2, so this should remain invalid.

[Czar102]

Planning to reject the escalation, agree with the Lead Judge.

[Czar102]

Result: Invalid Unique

[sherlock-admin]

Escalations have been resolved successfully!

Escalation status:

• juntzhan: rejected