ni8mare - `getKeeperFee` uses 2 oracles with the same _STALENESS_PERIOD, when their heartbeats could be different.

[sherlock-admin]

ni8mare

medium

getKeeperFee uses 2 oracles with the same _STALENESS_PERIOD, when their heartbeats could be different.

Summary

getKeeperFee uses 2 oracles with the same _STALENESS_PERIOD, when their heartbeats could be different.

Vulnerability Detail

getKeeprFee uses ethOracle:

        {
            (, int256 ethPrice, , uint256 ethPriceupdatedAt, ) = _ethOracle.latestRoundData();
            if (block.timestamp >= ethPriceupdatedAt + _STALENESS_PERIOD) revert FlatcoinErrors.ETHPriceStale();
            if (ethPrice <= 0) revert FlatcoinErrors.ETHPriceInvalid();
            ethPrice18 = uint256(ethPrice) * 1e10; // from 8 decimals to 18
        }

and it also uses _oracleModule:

        // NOTE: Currently the market asset and collateral asset are the same.
        // If this changes in the future, then the following line should fetch the collateral asset, not market asset.
        (uint256 collateralPrice, uint256 timestamp) = _oracleModule.getPrice();

Note the comment in the above code block. If the market asset and collateral asset are the same, then the same staleness period can be used. However, there is a possibility that it can be changed in the future and the market asset and collateral asset can differ. So, the same _STALENESS_PERIOD or heartbeat should not be used.

Impact

Stale prices might be used because of using the same heartbeat for 2 different oracles.

Code Snippet

https://github.com/sherlock-audit/2023-12-flatmoney/blob/bba4f077a64f43fbd565f8983388d0e985cb85db/flatcoin-v1/src/misc/KeeperFee.sol#L29

https://github.com/sherlock-audit/2023-12-flatmoney/blob/bba4f077a64f43fbd565f8983388d0e985cb85db/flatcoin-v1/src/misc/KeeperFee.sol#L83C1-L88C10

https://github.com/sherlock-audit/2023-12-flatmoney/blob/bba4f077a64f43fbd565f8983388d0e985cb85db/flatcoin-v1/src/misc/KeeperFee.sol#L91

Tool used

Manual Review

Recommendation

Use 2 different heartbeats for different oracles.

Duplicate of #155

[sherlock-admin]

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

takarez commented:

invalid

[sherlock-admin]

The protocol team fixed this issue in PR/commit https://github.com/dhedge/flatcoin-v1/pull/271.

[nevillehuang]

See comments here

[NishithPat]

Escalate

I am responding based on the discussion in the link that @nevillehuang put in his comment.

First of all, in the ubiquity contest, the admin can change the heartbeats for different collaterals. That is not the case here, since the staleness check is a constant variable in the flatmoney codebase.

uint256 private constant _STALENESS_PERIOD = 1 days;

The admin can't do anything to change this after deployment.

Also, I suggest the judges read this report before they comment on this issue - https://solodit.xyz/issues/m-12-chainlinkadaptor-uses-the-same-heartbeat-for-both-feeds-which-is-highly-dangerous-sherlock-none-jojo-exchange-git It explains the problem pretty well and it was reported before in Sherlock as well.

Quoting the judge's comment from nevi's link:

I don't see a vulnerability here. The staleness check is a sanity check since Chainlink's feeds are assumed to display up-to-date prices.

Note the word "assumed".

The judge is making an assumption that Chainlink's feeds will display up-to-date prices. But, that's not always the case. We check the heartbeat for a reason. So, that it doesn't return stale prices.

Coming back to this issue, if the market asset and collateral asset are different, the same _STALENESS_PERIOD cannot be used. If they are different, then a one-day staleness check is not sufficient. The protocol also believes the same. Otherwise, they would not have confirmed this issue. They would not have used the "Will FIx" tag.

If the protocol thinks a one-day staleness check is right for them (when the market asset and collateral asset are different), then this issue can be invalid. But, they didn't think that and hence they corrected it. We can ask the sponsors to comment on it to confirm.

[sherlock-admin2]

Escalate

I am responding based on the discussion in the link that @nevillehuang put in his comment.

First of all, in the ubiquity contest, the admin can change the heartbeats for different collaterals. That is not the case here, since the staleness check is a constant variable in the flatmoney codebase.

uint256 private constant _STALENESS_PERIOD = 1 days;

The admin can't do anything to change this after deployment.

Also, I suggest the judges read this report before they comment on this issue - https://solodit.xyz/issues/m-12-chainlinkadaptor-uses-the-same-heartbeat-for-both-feeds-which-is-highly-dangerous-sherlock-none-jojo-exchange-git It explains the problem pretty well and it was reported before in Sherlock as well.

Quoting the judge's comment from nevi's link:

I don't see a vulnerability here. The staleness check is a sanity check since Chainlink's feeds are assumed to display up-to-date prices.

Note the word "assumed".

The judge is making an assumption that Chainlink's feeds will display up-to-date prices. But, that's not always the case. We check the heartbeat for a reason. So, that it doesn't return stale prices.

Coming back to this issue, if the market asset and collateral asset are different, the same _STALENESS_PERIOD cannot be used. If they are different, then a one-day staleness check is not sufficient. The protocol also believes the same. Otherwise, they would not have confirmed this issue. They would not have used the "Will FIx" tag.

If the protocol thinks a one-day staleness check is right for them (when the market asset and collateral asset are different), then this issue can be invalid. But, they didn't think that and hence they corrected it. We can ask the sponsors to comment on it to confirm.

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.

[NishithPat]

A slight correction to the way I am using the terms market asset and collateral asset. All I am trying to say is that the _STALENESS_PERIOD should be different for _ethOracle and _oracleModule. And I am just seeking confirmation from the protocol whether they believe a one-day _STALENESS_PERIOD is good enough for them. If one-day _STALENESS_PERIOD is what they want, then this issue can be invalid. But, I don't think that should be the case, because the protocol will get stale prices.

[Czar102]

The submission is speculative – "heartbeats could be different" – there is no information whether there actually is an issue.

Also, failing to execute a sanity check is not a valid issue. To my knowledge, there is no oracle with the staleness period longer than 1 day.

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

[NishithPat]

@Czar102

This issue is not speculative. The _ethOracle here has a staleness period of 1 day. This should not be the case as the ETH/USD oracle on base has a heartbeat of 1200 sec. Please check the link - https://docs.chain.link/data-feeds/price-feeds/addresses?network=base&page=1&search=eth%2F

Using a staleness period of one day in such a case is wrong.

Please read these issues:

1. https://solodit.xyz/issues/m-4-outdated-variable-is-not-effective-to-check-price-feed-timeliness-sherlock-none-arrakis-git
2. https://solodit.xyz/issues/m-12-chainlinkadaptor-uses-the-same-heartbeat-for-both-feeds-which-is-highly-dangerous-sherlock-none-jojo-exchange-git

Both of them judged valid on Sherlock for the right reasons.

[NishithPat]

Also, just because I used the words "could be", it does not take away from what I am trying to imply/point out. I have also substantiated the problems with the issue in my escalation and further comments.

I have just noticed the main issue linked with this issue has also raised an escalation and that issue also talks about the staleness period of 1 day being incorrect. You may also want to look at it.

[Czar102]

Hey, I think this is of Low severity. Lack of a sanity check for a trusted party is not a severe error.

[NishithPat]

Low? But, these issues have always been ranked as medium. Be it Sherlock, C4 or anywhere else. Also, czar, it's not "a lack of sanity check", but it's incorrect use of that sanity check in this case.

Also, would it mean that all chainlink issues which do not check for a heartbeat are no longer a medium?

Have the docs been updated to suggest the same?

Honestly, I am a bit surprised.

[Czar102]

it's not "a lack of sanity check", but it's incorrect use of that sanity check in this case

That's right, this sanity check effectively doesn't work for one of the oracles. The team intended it to work, but provided that Chainlink is trusted to provide up-to-date data, this check is not needed anyway.

The docs have been updated recently to make the rules clearer. Feel free to check the changelog.

[nevillehuang]

Agree with @Czar102 should remain as invalid.

[Czar102]

Result: Low Duplicate of #155

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• NishithPat: rejected