xiaoming90 - Price returned by Oracle is not verified

[sherlock-admin]

xiaoming90

high

Price returned by Oracle is not verified

Summary

The price returned by the oracle is not adequately verified, leading to incorrect pricing being accepted.

Vulnerability Detail

As per the example provided by Tellor on how to integrate the Tellor oracle into the system, it has shown the need to check that the price returned by the oracle is not zero.

https://github.com/tellor-io/tellor-caller-liquity/blob/3ed91e4b15f0bac737f2966e36805ade3daf8162/contracts/TellorCaller.sol#L51C34-L51C34

function getTellorCurrentValue(bytes32 _queryId)
	..SNIP..
    // retrieve most recent 20+ minute old value for a queryId. the time buffer allows time for a bad value to be disputed
    (, bytes memory data, uint256 timestamp) = tellor.getDataBefore(_queryId, block.timestamp - 20 minutes);
    uint256 _value = abi.decode(data, (uint256));
    if (timestamp == 0 || _value == 0) return (false, _value, timestamp);

Thus, the value returned from the getDataBefore function should be verified to ensure that the price returned by the oracle is not zero. However, this was not implemented.

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

File: TellorOracle.sol
101:     function getPriceInEth(address tokenToPrice) external returns (uint256) {
102:         TellorInfo memory tellorInfo = _getQueryInfo(tokenToPrice);
103:         uint256 timestamp = block.timestamp;
104:         // Giving time for Tellor network to dispute price
105:         (bytes memory value, uint256 timestampRetrieved) = getDataBefore(tellorInfo.queryId, timestamp - 30 minutes);
106:         uint256 tellorStoredTimeout = uint256(tellorInfo.pricingTimeout);
107:         uint256 tokenPricingTimeout = tellorStoredTimeout == 0 ? DEFAULT_PRICING_TIMEOUT : tellorStoredTimeout;
108: 
109:         // Check that something was returned and freshness of price.
110:         if (timestampRetrieved == 0 || timestamp - timestampRetrieved > tokenPricingTimeout) {
111:             revert InvalidDataReturned();
112:         }
113: 
114:         uint256 price = abi.decode(value, (uint256));
115:         return _denominationPricing(tellorInfo.denomination, price, tokenToPrice);
116:     }

Impact

The protocol relies on the oracle to provide accurate pricing for many critical operations, such as determining the debt values of DV, calculators/stats used during the rebalancing process, NAV/shares of the LMPVault, and determining how much assets the users should receive during withdrawal.

If an incorrect value of zero is returned from Tellor, affected assets within the protocol will be considered worthless.

Code Snippet

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

Tool used

Manual Review

Recommendation

Update the affected function as follows.

function getPriceInEth(address tokenToPrice) external returns (uint256) {
    TellorInfo memory tellorInfo = _getQueryInfo(tokenToPrice);
    uint256 timestamp = block.timestamp;
    // Giving time for Tellor network to dispute price
    (bytes memory value, uint256 timestampRetrieved) = getDataBefore(tellorInfo.queryId, timestamp - 30 minutes);
    uint256 tellorStoredTimeout = uint256(tellorInfo.pricingTimeout);
    uint256 tokenPricingTimeout = tellorStoredTimeout == 0 ? DEFAULT_PRICING_TIMEOUT : tellorStoredTimeout;

    // Check that something was returned and freshness of price.
-   if (timestampRetrieved == 0 || timestamp - timestampRetrieved > tokenPricingTimeout) {
+	if (timestampRetrieved == 0 || value == 0 || timestamp - timestampRetrieved > tokenPricingTimeout) {
        revert InvalidDataReturned();
    }

    uint256 price = abi.decode(value, (uint256));
    return _denominationPricing(tellorInfo.denomination, price, tokenToPrice);
}

[sherlock-admin2]

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

Trumpero commented:

low, similar to chainlink round completeness

[xiaoming9090]

Escalate

The reason why chainlink round completeness is considered invalid/low in Sherlock is that the OCR does not rely on rounds for reporting anymore. Not validating the price returned from the oracle is zero is a much more serious issue and is different from the round completeness issue related to lagged/outdated price.

If an incorrect value of zero is returned from Tellor, affected assets within the protocol will be considered worthless. Thus, it is important that this check must be done. In addition, per the example provided by Tellor on how to integrate the Tellor oracle into the system, it has shown the need to check that the price returned by the oracle is not zero.

Thus, this is a valid High issue.

[sherlock-admin2]

Escalate

The reason why chainlink round completeness is considered invalid/low in Sherlock is that the OCR does not rely on rounds for reporting anymore. Not validating the price returned from the oracle is zero is a much more serious issue and is different from the round completeness issue related to lagged/outdated price.

If an incorrect value of zero is returned from Tellor, affected assets within the protocol will be considered worthless. Thus, it is important that this check must be done. In addition, per the example provided by Tellor on how to integrate the Tellor oracle into the system, it has shown the need to check that the price returned by the oracle is not zero.

Thus, this is a valid High 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.

[Trumpero]

Hello @xiaoming9090, according to the information I found in the Tellor documentation under the section "Solidity Integration," the provided example in the documentation differs from the example you provided, and it lacks a check for the 0-value.

Could you please provide more details about the source of your example?

[xiaoming9090]

Hey @Trumpero, the repo can be found in the Tellor's User Checklist, which is a guide that the protocol team need to go through before using the Tellor oracle.

One point to make is that Chainlink or Tellor would not provide an example that checks for zero-value in all their examples. The reason is that not all use cases require zero-value checks. Suppose a simple protocol performs a non-price-sensitive operation, such as fetching a price to emit an event for the protocol team's internal reference. In that case, there is no need to check for zero value.

However, for Tokemak and many other protocols involving financial transactions, it is critical that the price of assets cannot be zero due to errors from Oracle. Thus, a zero-values check is consistently implemented on such kind of protocols. Therefore, we need to determine if a zero-values check is required on a case-by-case basis. In this case, Tokemak falls under the latter group.

[Trumpero]

Tks @xiaoming9090, understand it now. Seem a valid issue for me @codenutt.
The severity for me should be medium, since it assumes the tellor oracle returns a 0 price value.

[Evert0x]

Planning to accept escalation and make issue medium

[codenutt]

@Trumpero Our goal with the check is to verify that a price was actually found. Based on their contract checking for timestamp == 0 is sufficient as it returns both 0 and 0 in this state: https://github.com/tellor-io/tellorFlex/blob/bdefcab6d90d4e86c34253fdc9e1ec778f370c3c/contracts/TellorFlex.sol#L450

[Trumpero]

Based on the sponsor's comment, I think this issue is low.

[Evert0x]

Planning to reject escalation and keep issue state as is @xiaoming9090

[xiaoming9090]

The sponsor's comment simply explains the purpose/intention of the if-condition is to check that a price is returned from Tellor Oracle. However, this does not necessarily mean that it is fine that the returned price is zero OR there is no risk if the return price is zero.

The protocol uses two (2) oracles (Chainlink and Tellor). In their Chainlink oracle's implementation, the protocol has explicitly checked that the price returned is more than zero. Otherwise, the oracle will revert. Thus, it is obvious that zero price is not accepted to the protocol based on the codebase.

https://github.com/sherlock-audit/2023-06-tokemak/blob/5d8e902ce33981a6506b1b5fb979a084602c6c9a/v2-core-audit-2023-07-14/src/oracles/providers/ChainlinkOracle.sol#L113

if (
	roundId == 0 || price <= 0 || updatedAt == 0 || updatedAt > timestamp
		|| updatedAt < timestamp - tokenPricingTimeout
) revert InvalidDataReturned();

However, this was not consistently implemented in their Tellor's oracle, which is highlighted in this report.

In addition, as pointed out in my earlier escalation. for Tokemak and many other protocols involving financial transactions, it is critical that the price of assets cannot be zero. Thus, a zero-values check is consistently implemented on such kind of protocols. Some examples are as follows:

• AAVE - Source Code and Documentation
• Compound - Source Code

[Evert0x]

Thanks @xiaoming9090

The core issue is that 0 value isn't handled well. There is no counter argument to this in the recent comments.

Planning to accept escalation and make issue medium

[Evert0x]

@Trumpero let me know if you agree with this.

[Trumpero]

I agree that this issue should be a medium within the scope of Tokemak contracts.

[Evert0x]

Result:
Medium
Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• xiaoming9090: accepted