Audinarey - Future upgrades to chainlink API can brick the protocol

[sherlock-admin2]

Audinarey

high

Future upgrades to chainlink API can brick the protocol

Summary

The issue should be under scope as the FAQ for the contest although mentions that chainlink is trusted but broken assumptions and future concerns should be reported.

Chainlink is TRUSTED.

Chainlink itself has warned against the use of latestAnswer() concerns which the protocol does not take into account considering future upgrade decisions that Chainlink could make on its price oracle APIs. Moreover, it reports latestAnswer with varying decimals: 18 decimals for crypto quotes and 8 for FX quotes, leading to inconsistency

Vulnerability Detail

Although the protocol trie to mitigate pricing concern by reverting if

    if (price <= 0) revert InvalidPrice();

Considering that owing to future API improvements and upgrades, Chainlink can decide to remove the latestAnswer() endpoint entirely or worse yet hard code it to an arbitrary value including zero, major functionalities of the Market.sol contract will always revert. Some of these functionalities includes but are not limited to: Auditor::accountLiquidity(...). Auditor::checkLiquidation(...) Auditor::calculateSeize(...). DebtPreviewer::minDeposit(...) DebtPreviewer::previewAssetsOut(...)

Impact

The use of an unsupported and deprecated function can brick the protocol or even lead to exploits considering if the protocol does not make an immediate adjustment to the price feed

The issue should be under scope as the FAQ for the contest although mentions that chainlink is trusted but broken assumptions and future concerns should be reported.

Code Snippet

https://github.com/sherlock-audit/2024-04-interest-rate-model/blob/main/protocol/contracts/Auditor.sol#L329

https://github.com/sherlock-audit/2024-04-interest-rate-model/blob/main/protocol/contracts/Auditor.sol#L283-L284

https://github.com/sherlock-audit/2024-04-interest-rate-model/blob/main/protocol/contracts/Auditor.sol#L212

https://github.com/sherlock-audit/2024-04-interest-rate-model/blob/main/protocol/contracts/Auditor.sol#L127

Tool used

Manual Review

Recommendation

Follow the direction in the chainlink documentation here.

[santipu03]

I believe this issue to be invalid according to the README:

Please list any known issues/acceptable risks that should not result in a valid finding. We trust Chainlink, and no liveness checks are performed while retrieving Oracle data.

Even if Chainlink marked the use of latestAnswer as deprecated, Chainlink is trusted so it's assumed that the endpoint will still work in the future. We have to take into account that there's a good amount of non-upgradeable protocols deployed years ago that still depend on that endpoint so it's highly improbable that it stops working suddenly.

Waiting for the sponsor tags to close the issue.

[midori-fuse]

Escalate

I believe the trust assumption works a little different here. If Chainlink is trusted, then their official documentation is also trusted and should be followed.

This issue then describes the use of an API that has been officially declared as deprecated, and explicitly warned against use in the official documentation. Because the API is declared as deprecated at the point of the audit, this does not fall into future integration issues by the rule. Also because it was clearly stated in the Chainlink docs that this is deprecated, notice in advance has been given, and so Chainlink should not be expected to provide further notices on what was already given prior notice.

Thus it can be said that the trusted docs actually confirms this issue (although I think this is a Medium and not a High).

[sherlock-admin3]

Escalate

I believe the trust assumption works a little different here. If Chainlink is trusted, then their official documentation is also trusted and should be followed.

This issue then describes the use of an API that has been officially declared as deprecated, and explicitly warned against use in the official documentation. Because the API is declared as deprecated at the point of the audit, this does not fall into future integration issues by the rule. Also because it was clearly stated in the Chainlink docs that this is deprecated, notice in advance has been given, and so Chainlink should not be expected to provide further notices on what was already given prior notice.

Thus it can be said that the trusted docs actually confirms this issue (although I think this is a Medium and not a High).

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.

[santipu03]

I understand the point of the escalation and I agree with the arguments given. The validity of this issue depends on how you interpret this section of the README:

Please list any known issues/acceptable risks that should not result in a valid finding. We trust Chainlink, and no liveness checks are performed while retrieving Oracle data.

Because the impact of this issue depends on future upgrades within the Chainlink protocol (making latestAnswer stop working) I thought it'd be considered a known issue but I guess it depends on how you look at it.

[midori-fuse]

Good point indeed, I can see the contradiction.

Given that a trusted Chainlink explicitly mentions that the API is deprecated, however the same README question states that no liveness checks are performed (indicating that liveness is a non-issue), this is equivalent to "we trust that a deprecated API won't be bricked", which doesn't really make sense.

I won't be repealing my escalation, but I also acknowledge that your point of view is valid. The only thing I have to disagree with is that it's a future integration (since the docs is known at the present), however I don't disagree that it can be a known issue depending on viewpoint.

If Sherlock judges do eventually decide it's a known issue, I am ok with it as well.

[Audinarey]

Escalate

No where in the FAQ does it state that it is a known issue by the protocol

Because the impact of this issue depends on future upgrades within the Chainlink protocol

@santipu03 the FAQ states that broken assumption about function behaviour be reported if they could pose a risk to the protocol in future integrations as shown below so this issue is valid

[sherlock-admin3]

Escalate

No where in the FAQ does it state that it is a known issue by the protocol

Because the impact of this issue depends on future upgrades within the Chainlink protocol

@santipu03 the FAQ states that broken assumption about function behaviour be reported if they could pose a risk to the protocol in future integrations as shown below so this issue is valid

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.

[Audinarey]

Escalate

@santipu03 I await your response

[santipu03]

The head of judging already has all the necessary information about this issue and will have the final say.

[cvetanovv]

I disagree with the escalation.

The Readme clearly states that Chainlink is TRUSTED:

Are the admins of the protocols your contracts integrate with (if any) TRUSTED or RESTRICTED? If these integrations are trusted, should auditors also assume they are always responsive...:

Chainlink is TRUSTED.

Note that the paragraph also means that auditors will assume they are always responsive.

It is also stated that everything related to Chainlink is a known issue/acceptable risk.

Please list any known issues/acceptable risks that should not result in a valid finding.

We trust Chainlink, and no liveness checks are performed while retrieving Oracle data.

The rule for future integration issues would be valid if Chainlink was RESTRICTED.

Planning to reject the escalation and leave the issue as is.

[midori-fuse]

I agree with the decision.

While I still don't think that Chainlink being restricted should be a requirement (considering that they themselves warned against using their own deprecated API), I can agree that this issue is known and also accepted by the protocol team based on the provided info.

[Evert0x]

Result: Invalid Has Duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• midori-fuse: rejected