Access to Chainlink price feeds can be blocked to DOS important trading function calls

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/utils/TradingLibrary.sol#L91-L122

Vulnerability details

Impact

Calling important trading functions like Trading.initiateMarketOrder, Trading.initiateCloseOrder, Trading.addToPosition, Trading.removeMargin, Trading.executeLimitOrder, and Trading.liquidatePosition eventually calls the following TradingLibrary.verifyPrice function, which verifies if the off-chain price is within the plus or minus 2% of the price returned by the Chainlink price feed given that the Chainlink verification is on and the corresponding Chainlink price feed exists.

According to https://blog.openzeppelin.com/secure-smart-contract-guidelines-the-dangers-of-price-oracles/, it is possible that Chainlink’s "multisigs can immediately block access to price feeds at will". When this occurs, the TradingLibrary.verifyPrice function call's execution of IPrice(_chainlinkFeed).latestAnswer() will revert, which will cause these important trading function calls to revert. As a result, these important trading function calls are DOS'ed, and the protocol's usability becomes much limited.

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/utils/TradingLibrary.sol#L91-L122

    function verifyPrice(
        uint256 _validSignatureTimer,
        uint256 _asset,
        bool _chainlinkEnabled,
        address _chainlinkFeed,
        PriceData calldata _priceData,
        bytes calldata _signature,
        mapping(address => bool) storage _isNode
    )
        external view
    {
        ...
        if (_chainlinkEnabled && _chainlinkFeed != address(0)) {
            int256 assetChainlinkPriceInt = IPrice(_chainlinkFeed).latestAnswer();
            if (assetChainlinkPriceInt != 0) {
                uint256 assetChainlinkPrice = uint256(assetChainlinkPriceInt) * 10**(18 - IPrice(_chainlinkFeed).decimals());
                require(
                    _priceData.price < assetChainlinkPrice+assetChainlinkPrice*2/100 &&
                    _priceData.price > assetChainlinkPrice-assetChainlinkPrice*2/100, "!chainlinkPrice"
                );
            }
        }
    }

Proof of Concept

The following steps can occur for a scenario involving a DOS'ed Trading.initiateCloseOrder function call.

1. Alice calls the Trading.initiateMarketOrder function to initiate a market order.
2. Without any prior notice, Chainlink's multisigs blocks access to the price feed for the corresponding position asset.
3. Alice calls the Trading.initiateCloseOrder function to initiate closing position for the corresponding market order.
4. Because of step 2, Alice's TradingLibrary.verifyPrice and Trading.initiateCloseOrder function calls revert. She is unable to withdraw the corresponding deposited margin.

Tools Used

VSCode

Recommended Mitigation Steps

In the TradingLibrary.verifyPrice function, IPrice(_chainlinkFeed).latestAnswer() can be refactored to try IPrice(_chainlinkFeed).latestAnswer() returns (int256 answer) { ... } catch Error(string memory) { ... }. The logics for verifying the off-chain price by using answer returned by the Chainlink price feed can then be placed in the try block.

[GalloDaSballo]

Checking for reference the ETH/USD Feed

A 4/9 Multi can set the AccessController which can ultimately deny service.

In case of a service denial, all tx will revert.

That is a fact

This has never happened to my knowledge, and the try/catch approach doesn't really offer a complete solution

Will flag, not sure about severity

[TriHaz]

it is possible that Chainlink’s "multisigs can immediately block access to price feeds at will".

I don't think that ever happened, maybe still valid but I don't think it should be med risk.

[c4-sponsor]

TriHaz marked the issue as sponsor confirmed

[c4-sponsor]

TriHaz marked the issue as disagree with severity

[GalloDaSballo]

I think this finding to have validity, but:

• Oracle can be changed by Governance
• We have never seen a CL feed be deprecated "randomly"

I believe the most appropriate severity to be Low

[GalloDaSballo]

L

[c4-judge]

Duplicate of https://github.com/code-423n4/2022-12-tigris-findings/issues/658

[rbserver]

Hi @GalloDaSballo ,

I raised the same concern in a recent contest (https://github.com/code-423n4/2022-10-inverse-findings/issues/586), which was considered as a median risk after the judge and sponsor of that contest reached an agreement.

I think this risk has some economic impact on the user. Since there is no guarantee that this issue will never occur in the future, if this issue occurs, the time of being unable to access the oracle can be difficult to estimate. This down time until the governance notices the issue and react, which may include processes like voting that can be time-consuming, can be long. If a user, such as Alice mentioned in the Proof of Concept section above, needs to withdraw her deposited margin for a financial emergency, she is unable to do so in a timely manner when this issue occurs. This can result in an immediate and unexpected economic hardship for this user.

In a business point of view, if this protocol does not decide to address this issue while other protocols do, when this issue happens, the user experience of this protocol will be negatively impacted, which damages its competitiveness.

Because of the reasons above, I would like to ask for a possibility for re-evaluating this finding's risk. Thanks for your work and time!

[GalloDaSballo]

Hi @GalloDaSballo ,

I raised the same concern in a recent contest (code-423n4/2022-10-inverse-findings#586), which was considered as a median risk after the judge and sponsor of that contest reached an agreement.

I think this risk has some economic impact on the user. Since there is no guarantee that this issue will never occur in the future, if this issue occurs, the time of being unable to access the oracle can be difficult to estimate. This down time until the governance notices the issue and react, which may include processes like voting that can be time-consuming, can be long. If a user, such as Alice mentioned in the Proof of Concept section above, needs to withdraw her deposited margin for a financial emergency, she is unable to do so in a timely manner when this issue occurs. This can result in an immediate and unexpected economic hardship for this user.

In a business point of view, if this protocol does not decide to address this issue while other protocols do, when this issue happens, the user experience of this protocol will be negatively impacted, which damages its competitiveness.

Because of the reasons above, I would like to ask for a possibility for re-evaluating this finding's risk. Thanks for your work and time!

Have replied in the Dicussion, thank you for flagging.

Ultimately both me and 0xean agree that the key distinction is the Immutability of the setting, which makes the finding for the INV contest more severe, while in this case it can be addressed within minutes because of the more trusted setup used (and disclosed in the Admin Privilege Report)