search

sherlock-audit / 2023-12-flatmoney-judging

11 stars 9 forks source link

HSP - Protocol won't be able to get rETH/USD price from OracleModule #90

Closed sherlock-admin2 closed 7 months ago

sherlock-admin2 commented 8 months ago

HSP

high

Protocol won't be able to get rETH/USD price from OracleModule

Summary

Protocol won't be able to get rETH/USD price from OracleModule because rETH/USD is supported by Chainlink on Base Chain.

Vulnerability Detail

FlatMoney uses rETH as collateral and the price will be retrieved from Pyth Network Oracle, as said in the Audit Page:

Are there any off-chain mechanisms or off-chain procedures for the protocol (keeper bots, input validation expectations, etc)?

The protocol uses Pyth Network collateral (rETH) price feed. This is an offchain price that is pulled by the keeper and pushed onchain at time of any order execution.

While Pyth is used as Offchain Oracle, Chainlink Price Feed is used as Onchain oracle, when get price from OracleModule, protocol will check the diffPercent between the 2 oracles and the transaction will revert if diffPercent is larger than maxDiffPercent:

        uint256 priceDiff = (int256(onchainPrice) - int256(offchainPrice)).abs();
        uint256 diffPercent = (priceDiff * 1e18) / onchainPrice;
        if (diffPercent > maxDiffPercent) revert FlatcoinErrors.PriceMismatch(diffPercent);

Because it only makes sense to compare the price of the same collateral token, as rETH/USD price is retrieved from Pyth Oracle, the Chainlink Price Feed should also return rETH/USD. Unfortunately, rETH/USD is not supported by Chainlink on Base Chain, so there is no way to get rETH/USD price.

Protocol may choose an alternative Chainlink Price Feed, for example, ETH/USD, however, the price difference between ETH and rETH can be significant (at the time of wrting, RETH / ETH is 1.1), results in diffPercent being much larger than a rational maxDiffPercent and transaction will always revert, protocol won't be able to get price from OracleModule and this renders the whole protocol useless.

Impact

Protocol won't be able to get price from OracleModule, and protocol becomes useless.

Code Snippet

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/OracleModule.sol#L106

Tool used

Manual Review

Recommendation

Protocol should combine 2 Chainlink Price Feeds (ETH/USD and rETH/ETH) to get the rETH/USD price data on Base Chain.

sherlock-admin commented 8 months ago

1 comment(s) were left on this issue during the judging contest.

takarez commented:

invalid:

nevillehuang commented 8 months ago

@rashtrakoff I believe this is a valid medium severity issue, there indeed are no rETH/USD chainlink oracles on base chain. Based on code logic during the time of contest, wouldn't this cause a revert?

rashtrakoff commented 8 months ago

@nevillehuang , the protocol couldn't have been deployed if we didn't have rETH/USD oracle. It was an oversight on our behalf to not have properly checked the oracles available. We already have an audited version of contracts to get rETH/USD price based on rETH/ETH exchange rate and ETH/USD price. We internally don't believe this to be an issue but given that the code indeed wouldn't have worked/depolyable, we understand if you decide this as an issue. Cc @itsermin @D-Ig.

sherlock-admin2 commented 8 months ago

Escalate

This issue should be HIGH as it renders the protocol unworkable/undeployable.

You've deleted an escalation for this issue.

securitygrid commented 8 months ago

Escalate This is not valid. This is not a matter of contract.

sherlock-admin2 commented 8 months ago

Escalate This is not valid. This is not a matter of contract.

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.

xiaoming9090 commented 8 months ago

Escalate.

The protocol would not even be deployed without the rETH/USD oracle. Thus, there is no possibility of this issue occurring to begin with. So, a Low is more appropriate for this issue.

sherlock-admin2 commented 8 months ago

Escalate.

The protocol would not even be deployed without the rETH/USD oracle. Thus, there is no possibility of this issue occurring to begin with. So, a Low is more appropriate for this issue.

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.

midori-fuse commented 8 months ago

A criteria for medium risk is that

Breaks core contract functionality, rendering the contract useless or leading to loss of funds.

Because:

It certainly qualifies as a medium risk issue by the rules does it not?

itsermin commented 8 months ago

Just to chime in here. There will be no code change here. The oracle contract will be provided by dHedge protocol: https://github.com/dhedge/V2-Public/blob/master/contracts/priceAggregators/ETHCrossAggregator.sol

This contract can combine the rETH/ETH and ETH/USD oracles.

midori-fuse commented 7 months ago

"Not using the in-scope code, but externally provided code" is itself a code change.

The validity of this issue then comes down to whether the contest documentation mention or imply that the OracleModule contract was the intended oracle.

If a conclusion of an external oracle cannot be reasonably reached using the given contest resources, then this issue should fit into the medium criteria.

NishithPat commented 7 months ago

The protocol would not even be deployed without the rETH/USD oracle. Thus, there is no possibility of this issue occurring to begin with. So, a Low is more appropriate for this issue.

Yes, but Watsons are supposed to audit the code that's given to them. In the current state of the code, this is indeed an issue.

nevillehuang commented 7 months ago

Agree with @midori-fuse points, I believe this should remain as medium severity, given the original contract cannot integrate the intended rETH/USD oracle originally, and required a completely new separate contract as mentioned by the sponsor.

Evert0x commented 7 months ago

Planning to accept escalation and invalidate issue

There are no funds at risk since the protocol wouldn't be able to get deployed

midori-fuse commented 7 months ago

@Evert0x Can you please provide explanation on why it doesn't fit in the second criteria of medium severity? The criteria certainly mentions rendering the contract useless as a condition.

Furthermore, if the reasoning is that protocol cannot be deployed, then all findings are invalid since the protocol won't be deployed anyway isn't it?

0xjuaan commented 7 months ago

I believe that the root cause for this bug would be an incorrect deployment script. However the deployment script is out of scope.

midori-fuse commented 7 months ago

Understandable. I do not have more context to add then.

RealLTDingZhen commented 7 months ago

I have a few things to add:

Sherlock used to always think that the inability to deploy/use the current version of the contract's code on a specific chain was a valid problem:

pengun - CREATE3 is not available in the zkSync Era.

HHK - computePoolAddress() will not work on ZkSync Era

I know that 'Historical decisions are no longer considered sources of truth.' , but I believe the comments made by Evert and Czar was enough to justify the question.

And, this issue cannot be solved by updating deployment script.

0xjuaan commented 7 months ago

And, this issue cannot be solved by updating deployment script.

It actually can be fixed in the deployment script, by just setting the oracle address to the address of this contract that was made by the protocol team, and already audited (according to them).

0xhsp commented 7 months ago

And, this issue cannot be solved by updating deployment script.

It actually can be fixed in the deployment script, by just setting the oracle address to the address of this contract that was made by the protocol team, and already audited (according to them).

Disagree. There is no evidence provided to Watsons in the public domain during the audit contest period (22 Jan to 4 Feb) that states that the protocol team will set the oracle address to the address of this contract that was made by the protocol team.

0xjuaan commented 7 months ago

@0xhsp yes, that is correct. but the fix to this issue raised would involve a change in the deployment script, which is out of scope.

0xhsp commented 7 months ago

@0xjuaan Since the contract you mentioned is not known during the contest period, the code change should be considered as the only way to mitigate the issue. Otherwise all the valid issue can be invalidated as the affected contracts can be replaced by a new contract.

0xjuaan commented 7 months ago

that's a good point. we will see what judges say

nevillehuang commented 7 months ago

I too have no additional comments to add and stand by my previous comment here, that this should maintain as medium severity given the whole oracle contract code logic was changed to integrated into the deployment script.

Czar102 commented 7 months ago

I'd like to draw a clear line between different types of vulnerabilities.

There are bugs that cause the codebase not to work at all (like mentioned https://github.com/sherlock-audit/2023-10-real-wagmi-judging/issues/104) and these are valid Medium severity issues.

There are also bugs that prevent the contracts from being properly deployed/initialized, and these are considered of Low/Informational severity:

  1. Front-running initializers: Front-running initializers where there is no irreversible damage or loss of funds & the protocol could just redeploy and initialize again is not a valid issue.

I must claim that one needs to slightly extrapolate this rule, and this will be corrected in the rules to be more generalized.

Anyway, for this issue I think there are grounds for invalidation anyway – Chainlink can be asked to introduce an additional feed. They can provide it for a small fee.

Maintaining Sherlock's stance to accept an escalation and invalidate the issue.

RealLTDingZhen commented 7 months ago

Fair enough, I would respect Sherlock's decision.

But I can't understand how Chainlink can be asked to introduce an additional feed. could be the reason to invalidate a issue. Such statement could invalidate most Chainlink-related issues which were regarded M/H😂

gstoyanovbg commented 7 months ago

@Evert0x @Czar102 Reading the comments, I am quite confused by Sherlock's decision. As far as I understand, the main argument for invalidating this report is that the protocol wouldn't be able to get deployed. I disagree with that. The protocol can be deployed with 2 incompatible feeds, as described in the report. This will result in non-functioning core features, which, as mentioned above, fully meets the criteria for a Medium severity issue described by Sherlock. I want to emphasize again that an auditor cannot know what the sponsors think and whether they are aware that their code will not work with the available feeds.

@Czar102 , with all due respect, it takes a great deal of imagination to fit the current report into the definition from point 7. Before submitting this report, I carefully examined the validity criteria several times and found no grounds to invalidate it. It seems that I am not alone, as many other auditors, including the lead judge, have made same conclusion. A new auditor who is not familiar with the way the Sherlock team reasons cannot, based solely on the validity criteria, conclude that the report is not valid. This would not be a problem if there were no penalties for invalid reports, but there are. And it turns out that auditors would be penalized with worsened ratios due to flaws in the validity criteria, which is unfair in my opinion.

If this trend continues, auditors will stop submitting such reports, and at some point it will turn out that some protocol passed the Sherlock audit without problems and at the same time is totally broken.

Czar102 commented 7 months ago

@RealLTDingZhen we are focusing on vulnerabilities here, not on technical difficulties of deployment.

@gstoyanovbg deploying with 2 incompatible feeds would be a deployment mistake, and we don't consider that scenario.

Czar102 commented 7 months ago

Result: Low Has duplicates

Rejecting the second escalation since the first escalation already proposed the correct outcome.

sherlock-admin2 commented 7 months ago

Escalations have been resolved successfully!

Escalation status: