TradingLibrary.verifyPrice: Negative prices not correctly handled

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/utils/TradingLibrary.sol#L113-L115

Vulnerability details

Impact

Referring to the docs, Chainlink oracles are returning the price as an int256, which means that the answer can be a negative price.

https://docs.chain.link/data-feeds/price-feeds/api-reference

Later, this price is casted as an uint256, which overflows when price < 0.

            int256 assetChainlinkPriceInt = IPrice(_chainlinkFeed).latestAnswer();
            if (assetChainlinkPriceInt != 0) {
                uint256 assetChainlinkPrice = uint256(assetChainlinkPriceInt) * 10**(18 - IPrice(_chainlinkFeed).decimals());

Let's say _chainlinkFeed.latestAnswer() = -1. Then, price will be equal to type(uint256).max (i.e., a huge number) and this wrong price will be used by the system, leading to system stuck.

Proof of Concept

https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/utils/TradingLibrary.sol#L113-L115

Tools Used

None

Recommended Mitigation Steps

Check that the latest answer is > 0 before casting

[GalloDaSballo]

Agree with finding, but not convinced by severity, what are the odds of a negative CL feed? Has this ever happened?

[GalloDaSballo]

Have asked some info to CL and believe this finding will be downgraded to R at most (not even Low)

A CL feed ultimately has min and max boundaries, and Price Feeds will have positive min / max values

Negative prices seem to be supported for Commodities Future if you were wondering

[TriHaz]

Price feed can't be negative. Even if we assumed it can be, code will revert.

[c4-sponsor]

TriHaz marked the issue as sponsor disputed

[GalloDaSballo]

Downgrading to R as I would recommend a check before casting, however no clear risk was shown

R

[c4-judge]

Duplicate of #334

[GalloDaSballo]

Note that a cast to uint256 will not revert on a negative number, you'll get the twos complement of the value
https://docs.soliditylang.org/en/v0.8.17/types.html?highlight=int256#explicit-conversions

[Simon-Busch]

Remove Dup of 334 label and mark as unsatisfactory as requested by @GalloDaSballo