Skip to content
This repository has been archived by the owner on Nov 12, 2023. It is now read-only.

0x52 - chainlinkAdaptor uses the same heartbeat for both feeds which is highly dangerous #449

Open
sherlock-admin opened this issue May 10, 2023 · 7 comments
Labels
Escalation Resolved This issue's escalations have been approved/rejected Has Duplicates A valid issue with 1+ other issues describing the same vulnerability Medium A valid Medium severity issue Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed

Comments

@sherlock-admin
Copy link
Contributor

0x52

medium

chainlinkAdaptor uses the same heartbeat for both feeds which is highly dangerous

Summary

chainlinkAdaptor uses the same heartbeat for both feeds when checking if the data feed is fresh. The issue with this is that the USDC/USD oracle has a 24 hour heartbeat, whereas the average has a heartbeat of 1 hour. Since they use the same heartbeat the heartbeat needs to be slower of the two or else the contract would be nonfunctional most of the time. The issue is that it would allow the consumption of potentially very stale data from the non-USDC feed.

Vulnerability Detail

See summary

Impact

Either near constant downtime or insufficient staleness checks

Code Snippet

https://github.com/sherlock-audit/2023-04-jojo/blob/main/smart-contract-EVM/contracts/adaptor/chainlinkAdaptor.sol#L43-L55

Tool used

Manual Review

Recommendation

Use two separate heartbeat periods

@github-actions github-actions bot added Medium A valid Medium severity issue Has Duplicates A valid issue with 1+ other issues describing the same vulnerability labels May 17, 2023
@JoscelynFarr JoscelynFarr added the Sponsor Disputed The sponsor disputed this issue's validity label May 18, 2023
@JoscelynFarr
Copy link

The contract are trying to get the latest price in here:https://github.com/sherlock-audit/2023-04-jojo/blob/main/smart-contract-EVM/contracts/adaptor/chainlinkAdaptor.sol#LL47C1-L47C1

And the heartbeat is trying to prevent chainlink stop updating. It is the same as chainlink's heartbeat.
https://docs.chain.link/data-feeds/price-feeds/addresses/?network=arbitrum#Arbitrum%20Mainnet

@sherlock-admin sherlock-admin added Non-Reward This issue will not receive a payout and removed Medium A valid Medium severity issue Has Duplicates A valid issue with 1+ other issues describing the same vulnerability labels May 30, 2023
@deadrosesxyz
Copy link

Escalate for 10 USDC
I don’t think the sponsor properly understood the issue. On Arbitrum, as well as pretty much any other network, different token pairs have different heartbeats. If the oracle gets the latest price for two pairs with different heartbeats, using the same heartbeat variable for validation would cause either one of the following:

  1. Oracle will be down (will revert) most of the time.
  2. Oracle will allow for stale prices

When validating prices for two different token pairs, two different heartbeats must be used.

@sherlock-admin
Copy link
Contributor Author

Escalate for 10 USDC
I don’t think the sponsor properly understood the issue. On Arbitrum, as well as pretty much any other network, different token pairs have different heartbeats. If the oracle gets the latest price for two pairs with different heartbeats, using the same heartbeat variable for validation would cause either one of the following:

  1. Oracle will be down (will revert) most of the time.
  2. Oracle will allow for stale prices

When validating prices for two different token pairs, two different heartbeats must be used.

You've created a valid escalation for 10 USDC!

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.

@sherlock-admin sherlock-admin added the Escalated This issue contains a pending escalation label May 30, 2023
@JoscelynFarr
Copy link

@hrishibhat hrishibhat added Medium A valid Medium severity issue Sponsor Confirmed The sponsor acknowledged this issue is valid and removed Sponsor Disputed The sponsor disputed this issue's validity labels Jun 16, 2023
@hrishibhat
Copy link

Result:
Medium
Has duplicates
Given that the code uses the same heartbeat to validate both assets, when both assets can have different heartbeats, considering this issue a valid medium

Sponsor comment:

got it, we will accept this issue

@hrishibhat hrishibhat reopened this Jun 16, 2023
@sherlock-admin sherlock-admin added Reward A payout will be made for this issue Escalation Resolved This issue's escalations have been approved/rejected and removed Escalated This issue contains a pending escalation Non-Reward This issue will not receive a payout labels Jun 16, 2023
@sherlock-admin
Copy link
Contributor Author

Escalations have been resolved successfully!

Escalation status:

@sherlock-admin sherlock-admin added the Has Duplicates A valid issue with 1+ other issues describing the same vulnerability label Jun 19, 2023
@JoscelynFarr JoscelynFarr added the Will Fix The sponsor confirmed this issue will be fixed label Jun 20, 2023
@IAm0x52
Copy link
Collaborator

IAm0x52 commented Jun 22, 2023

Fix looks good. Contract now uses separate heartbeats for asset and USDC

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Escalation Resolved This issue's escalations have been approved/rejected Has Duplicates A valid issue with 1+ other issues describing the same vulnerability Medium A valid Medium severity issue Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed
Projects
None yet
Development

No branches or pull requests

6 participants