Vagner - getPriceInEth in TellorOracle.sol doesn't uses the best practices recommended by Tellor which can cause wrong pricing

[sherlock-admin2]

Vagner

medium

getPriceInEth in TellorOracle.sol doesn't uses the best practices recommended by Tellor which can cause wrong pricing

Summary

The function getPriceInEth it is used to call Tellor oracle to check the prices of different assets but the implementation doesn't respect the best practices recommended by Tellor which can cause wrong pricing.

Vulnerability Detail

Tellor team has a Development Checklist
https://docs.tellor.io/tellor/getting-data/user-checklists#development-checklist
that is important to use when you are using Tellor protocol to protect yourself against stale prices and also to limit dispute attacks. The prices in Tellor protocol can be disputed and changed in case of malicious acting and because of that dispute attacks can happen which would make the prices unstable. getPriceInEth calls getDataBefore
https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/oracles/providers/TellorOracle.sol#L105
and uses the timestamp returned to check if the prices are stale or not using DEFAULT_PRICING_TIMEOUT or tellorStoredTimeout
https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/oracles/providers/TellorOracle.sol#L107-L112
The value of the DEFAULT_PRICING_TIMEOUT is 2 hours and the frame of time where a dispute could happen is around 12 hours as per Tellor documentation, which could make getPriceInEth revert multiple times because of possible dispute attacks that could happen. The way the Tellor protocol recommends to limit the possibility of dispute attacks is by caching the most recent values and timestamps as can seen here in their example
https://github.com/tellor-io/tellor-caller-liquity/blob/3ed91e4b15f0bac737f2966e36805ade3daf8162/contracts/TellorCaller.sol#L9-L11
Basically every time data is returned it is checked against the last stored values and in case of a dispute attack it will return the last stored undisputed and accurate value as can be seen here
https://github.com/tellor-io/tellor-caller-liquity/blob/3ed91e4b15f0bac737f2966e36805ade3daf8162/contracts/TellorCaller.sol#L51-L58
It is also important to note that the Tellor protocol checks for data returned to not be 0
https://github.com/tellor-io/tellor-caller-liquity/blob/3ed91e4b15f0bac737f2966e36805ade3daf8162/contracts/TellorCaller.sol#L51
which is not done in the getPriceInEth, so in the case of a 0 value it will be decoded
https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/oracles/providers/TellorOracle.sol#L114
and then passed into _denominationPricing
https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/oracles/providers/TellorOracle.sol#L115
which would return a 0 value that could hurt the protocol.

Impact

Medium of because of wrong pricing or multiple revert in case of dispute attacks

Code Snippet

https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/oracles/providers/TellorOracle.sol#L101-L116

Tool used

Manual Review

Recommendation

I recommend to use the Tellor example in
https://github.com/tellor-io/tellor-caller-liquity/blob/main/contracts/TellorCaller.sol
and implement mappings to cache the values all the time, similar to how Tellor does it to limit as much as possible dispute attacks and also check for 0 value.

[sherlock-admin2]

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

Trumpero commented:

low, oracle completeness is considered low similar to chainlink round completeness

thekmj commented:

great analysis

[VagnerAndrei26]

Escalate
I don't believe this should be taken as a low because of several factors :

• firstly, sherlock rules states that Chainlink round completeness check are invalid since the new OCR doesn't rely on rounds for reporting the price, it doesn't state that every round completeness is invalid, because the mechanics may differ from oracle to oracle and in some situations it is important to check for everything
  
• secondly, because of how Tellor oracle work, anyone can dispute prices and act maliciously by paying the right amount of fees in TRB, that's why the Tellor talks about "dispute attacks" and specify why it is important to protect yourself against them, they acknowledge the risk where bad actors could manipulate the prices for their benefits and if a protocol implementing Tellor oracle does not protect themselves against this, it could hurt it.
• thirdly, it is important when implementing an external protocol, especially an important one like an oracle, to follow their best practices and recommendations since their methods are battle-tested and any mistakes could lead to loss of funds

In the end, I believe that this should not be treated as a simple Chainlink round completeness check , since the mechanics are different and the issue is more complex than that, I believe it should be treated highly since an attack path that could lead to loss of funds by manipulating the price exists, which Tokemak protocol doesn't protect against, as recommender by Tellor.

[sherlock-admin2]

Escalate
I don't believe this should be taken as a low because of several factors :

• firstly, sherlock rules states that Chainlink round completeness check are invalid since the new OCR doesn't rely on rounds for reporting the price, it doesn't state that every round completeness is invalid, because the mechanics may differ from oracle to oracle and in some situations it is important to check for everything
  
• secondly, because of how Tellor oracle work, anyone can dispute prices and act maliciously by paying the right amount of fees in TRB, that's why the Tellor talks about "dispute attacks" and specify why it is important to protect yourself against them, they acknowledge the risk where bad actors could manipulate the prices for their benefits and if a protocol implementing Tellor oracle does not protect themselves against this, it could hurt it.
• thirdly, it is important when implementing an external protocol, especially an important one like an oracle, to follow their best practices and recommendations since their methods are battle-tested and any mistakes could lead to loss of funds

In the end, I believe that this should not be treated as a simple Chainlink round completeness check , since the mechanics are different and the issue is more complex than that, I believe it should be treated highly since an attack path that could lead to loss of funds by manipulating the price exists, which Tokemak protocol doesn't protect against, as recommender by Tellor.

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]

I agree with the escalator that this should be a valid issue. The root cause is that oracle did not cache the most recent values and timestamps per Tellor's recommendation, leading to dispute attacks. This is similar to the issue I mentioned in my report (#612).

This issue (#351) and Issue #612 should be an issue of its own, and should not be grouped with the rest. Refer to my escalation (#612 (comment)) for more details.

[Evert0x]

Planning to accept escalation and make issue medium

[Evert0x]

Result:
Medium
Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• VagnerAndrei26: accepted