`ChainLink:price` does not revert for return feedValuefeedValue=0

[hats-bug-reporter[bot]]

Github username: @@Tri-pathi Twitter username: 0xTripathi Submission hash (on-chain): 0xa937e50099dfc66633defee589edefdcaf2c4d73192ac498a5e0cfeab03945f5 Severity: low

Description: Description

ChainLink:price does not revert for return price=0

Attack Scenario\ Describe how the vulnerability can be exploited.

Attachments

1. Proof of Concept (PoC) File

    function price(
        Config memory self,
        uint256 stalenessThreshold,
        OrigamiMath.Rounding roundingMode
    ) internal view returns (uint256) {
        (uint80 roundId, int256 feedValue, , uint256 lastUpdatedAt, uint80 answeredInRound) = self.oracle.latestRoundData();

        // Check for staleness
        if (
            answeredInRound <= roundId && 
            block.timestamp - lastUpdatedAt > stalenessThreshold
        ) {
            revert IOrigamiOracle.StalePrice(address(self.oracle), lastUpdatedAt, feedValue);
        }

        // Check for negative price
        if (feedValue < 0) revert IOrigamiOracle.InvalidPrice(address(self.oracle), feedValue);

        return self.scaleDown 
            ? uint256(feedValue).scaleDown(self.scalar, roundingMode)
            : uint256(feedValue).scaleUp(self.scalar);
    }

https://github.com/TempleDAO/origami-public/blob/main/apps/protocol/contracts/libraries/Chainlink.sol#L25

it reverts for negative price for not for price=0.

At many instances pricefeed returns 0 value which should be handled properly

2. Revised Code File (Optional)

[0xfuje]

OrigamiStableChainlinkOracle.sol - latestPrice() is likely the higher level function used that will revert because of the price bounds + Chainlink oracles can't return a value below their underlying aggregator's minAnswer

[Tri-pathi]

yes, it will revert in higher function but same goes for negative prices.

PS: it's better to remove negative price check as it will revert anyway due to out of range. [may be consider this as gas saving submission ?]

[Tri-pathi]

After talking with sponsors, I came to know that zero could be a valid price and that is why in latestAnswer only denominator is nonzero

which can cause serious issue

convertAmount Converts the baseAsset->quoteAsset or quoteAsset->baseAsset.

which does division by zero.

    function convertAmount(
        address fromAsset,
        uint256 fromAssetAmount,
        PriceType priceType,
        OrigamiMath.Rounding roundingMode 
    ) external override view returns (uint256 toAssetAmount) {
        if (fromAsset == baseAsset) {
            // The numerator needs to round in the same way to be conservative
            uint256 _price = latestPrice(
                priceType, 
                roundingMode
            );

            return fromAssetAmount.mulDiv(
                _price,
                assetScalingFactor,
                roundingMode
            );
        } else if (fromAsset == quoteAsset) {
            // The denominator needs to round in the opposite way to be conservative
            uint256 _price = latestPrice(
                priceType, 
                roundingMode == OrigamiMath.Rounding.ROUND_UP ? OrigamiMath.Rounding.ROUND_DOWN : OrigamiMath.Rounding.ROUND_UP
            );

            return fromAssetAmount.mulDiv(
                assetScalingFactor,
                _price,//@audit could be zero and it will lead to division by zero
                roundingMode
            );
        }

        revert CommonEventsAndErrors.InvalidToken(fromAsset);
    }

https://github.com/TempleDAO/origami-public/blob/main/apps/protocol/contracts/common/oracle/OrigamiOracleBase.sol#L126

Further this function is used extensively in the function which does transfer of tokens.

imo Medium/High now?

[frontier159]

As @0xfuje points out, the valid expected range for the Chainlink price is managed and checked within the OrigamiStableChainlinkOracle.sol#L114 contract

That is the only caller of this (internal) library. We intentionally did not add a check for feedValue==0 within the Chainlink library as this would duplicate the check and waste user gas.

If you think there's more to this, then please provide a test/PoC