valuevalk - Missing price validations for RedstoneOracle can compromise protocol's integrity

[sherlock-admin3]

valuevalk

High

Missing price validations for RedstoneOracle can compromise protocol's integrity

Summary

In the RedstoneOracle, a malicious actor could submit invalid price, such as 0, when there is an error. Although not impossible, its very unlikely that this issue would occur.

Vulnerability Detail

Redstone oracle uses an "on-demand" model, which allows for lower fees as the price is not constantly pushed. Instead, anyone can submit off-chain signed data that can be validated with the Redstone SDK. Check out the docs here.

It is possible to encounter a situation where the price is 0, although it should be very unlikely. Such scenarios can arise due to: Data Source Issues: If all data sources provide a 0 price (e.g., in case of a market failure or misconfiguration). Incorrect Data Handling: Bugs or issues in the aggregation/sdk logic that might incorrectly return a price of 0.

Impact

The impact would be significant as if someone managed to the price to 0. This could lead to mass liquidations.

Proof of Concept

All of the oracles in the protocol provide the getValueInEth function, and one of its main purposes is to ensure that the LTV ratio is healthy. Having 0 price, could lead to mass liquidations, as every position can suddenly become unhealthy.

PositionManager.liquidate() is validating the position health using an oracle -> liquidate code snippet. and validate code snippet

isPositionHealthy is using _getPositionDebtData and _getPositionAssetData which use getAssetValue and getDebtValueForPool. And the values are in ETH, which depends on the Oracle.

    function getDebtValueForPool(address position, uint256 poolId) public view returns (uint256) {
        address asset = pool.getPoolAssetFor(poolId);
        IOracle oracle = IOracle(riskEngine.getOracleFor(asset));
@>>     return oracle.getValueInEth(asset, pool.getBorrowsOf(poolId, position));
    }

    function getAssetValue(address position, address asset) public view returns (uint256) {
        IOracle oracle = IOracle(riskEngine.getOracleFor(asset));
        uint256 amt = IERC20(asset).balanceOf(position);
@>>     return oracle.getValueInEth(asset, amt);
    }

Tool used

Manual Review

Recommendation

Validate price is not 0, or above max.

    function updatePrice() external {
        // values[0] -> price of ASSET/USD
        // values[1] -> price of ETH/USD
        // values are scaled to 8 decimals
        uint256[] memory values = getOracleNumericValuesFromTxMsg(dataFeedIds);

        assetUsdPrice = values[0];
        ethUsdPrice = values[1];

+        if(assetUsdPrice == 0 || ethUsdPrice == 0 || ethUsdPrice > type(uint208).max || assetUsdPrice > type(uint208).max)  revert();
        // RedstoneDefaultLibs.sol enforces that prices are not older than 3 mins. since it is not
        // possible to retrieve timestamps for individual prices being passed, we consider the worst
        // case and assume both prices are 3 mins old
        priceTimestamp = block.timestamp - THREE_MINUTES;
    }

Duplicate of #401

[MrValioBg]

Additionally, this behavior was confirmed by the redstone.finance team that it's technically possible.

Their response:

In theory (and technically) it can be, but in 99.99% of cases - no. We use median for the price and a few sources (usually), but in some edge cases, it can happen, I think.

[MrValioBg]

This issue should be valid and high, as it can result in mass liquidations if the price is 0. This is possible and confirmed by Redstone's team themselves @z3s.

Euler's implementation factors that and has this check -> reference here

[ZdravkoHr]

Escalate On behalf of @MrValioBg

[sherlock-admin3]

Escalate On behalf of @MrValioBg

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.

[cvetanovv]

This issue is borderline medium/low for me as I lean more towards low. This is a very rare edge case, and as Redstone Oracle confirms, the chance of it happening is 0.01%. But on the other hand, if it happens, it is enough for Medium severity.

@z3s what do you think?

[MrValioBg]

Indeed, @cvetanovv, I agree it's definitely a rare case and has some external constraints. However, if it happens, depending on which asset returns 0, we will either:

• liquidate all positions
• be able to borrow assets for free

Both highly dangerous cases, which will lead certainly lead to more than 1% loss.

Due to the external constrains, as defined per sherlocks rules:

IV. How to identify a high issue: Definite loss of funds without (extensive) limitations of external conditions. The loss of the affected party must exceed 1%.

V. How to identify a medium issue: Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained.

I tend to agree that It may be more appropriate to label it as Medium, instead of HIGH, as you mentioned, @cvetanovv.

It should still be a valid H/M instead of low, as 0.01% is not a very low probability, given this impact and that the prices can be submitted by anyone via the updatePrice() function. Thus, when it happens that such a price is aggregated, someone will most likely take advantage of it, and the impact will be HIGH.

[Kalogerone]

I think you are all taking the 99.99% too literally.

This would mean on average that every 10_000 calls, 1 of them will return a price of 0 and all positions will be liquidatable and borrows will be free. If that was the case, protocols would get ruined because of this problem all the time, especially since this check is something that isn't on their documentation and a lot of protocols haven't implemented it for sure.

The dev because he can't guarantee that the redstone oracle will never ever return 0, said that 99.99% it won't. This percentage is not to be taken as a statistical measurement. He is not even sure if it's possible, he just doesn't want to guarantee it won't happen and end up ruining a protocol because of it:

but in some cases it can happen I think

I believe this edge case is even rarer than the 0.01% you are talking about or even impossible. The check would be nice to have nonetheless but the issue itself seems like a low because of its likelihood.

[MrValioBg]

We could only speculate what the dev meant exactly, both taking the "99.99%" too literally and also speculating that "He is not even sure if it's possible, he just doesn't want to guarantee it won't happen" and "believe this edge case is even rarer than the 0.01% you are talking about or even impossible" are very subjective and a matter of interpretation and bias.

However, it is a matter of fact that it's confirmed that "In theory (and technically) it can be.". It's a matter of fact that the Redstone Team confirmed they plan to add this to the docs, as suggested in the chat. It's a matter of fact that Euler has added this check reference here.
And it's a matter of fact that the impact would be high, even though the likelihood is low.

And as I quoted the validity criteria for Medium and High issues from Sherlock's rules, given the current arguments and information, a Medium issue would be the best match as it perfectly describes that "certain external conditions or specific states" are needed. Those "conditions" are confirmed to be possible, hence this should be a valid issue.

[Kalogerone]

However, it is a matter of fact that it's confirmed that "In theory (and technically) it can be.".

That's not confirmed anywhere. The redstone dev literally said that he THINKS in some edge cases it can happen. He didn't give a definite answer that, yes this is possible but rare.

That's just my opinion though, it's up to the judge to decide.

[MrValioBg]

However, it is a matter of fact that it's confirmed that "In theory (and technically) it can be.".

That's not confirmed anywhere. The redstone dev literally said that he THINKS in some edge cases it can happen. He didn't give a definite answer that, yes this is possible but rare.

That's just my opinion though, it's up to the judge to decide.

Just to clarify, what I said is that he confirmed that "In theory (and technically) it can be," as this is exactly what he said.

Nit-picking words from the conversation is always subjective. For me, it's clear that he said it's possible theoretically and technically and that they agreed this should be added to the docs.

As suggested, let's wait for Judge's take on this.

[cvetanovv]

This issue is still borderline Low/Medium for me. While the probability of returning a price of 0 is very low, the potential impact is high if it occurs. It would trigger liquidations or free borrowing, leading to significant financial losses for the protocol.

One of the most important reasons why I think it might be valid is that Euler has also implemented such a check: https://github.com/euler-xyz/euler-price-oracle/blob/4c798098e6212e4cbf9387526e4f918d270191f0/src/adapter/redstone/RedstoneCoreOracle.sol#L99

The Redstone team is also not sure what the chance of this happening is, but they confirm it is possible to return zero.

So I think this issue could be Medium. Very low likelihood and high impact.

I am planning to accept the escalation and make this issue(#591) and #401 Medium severity.

[Kalogerone]

Excuse me but the burden of proof is on the finding writer to prove that this is possible. It's not our job to prove that this is not possible.

One of the most important reasons why I think it might be valid is that Euler has also implemented such a check

For all we know, this could be a Low or even an Info finding from one of all the 5-6 audits they have gone through. For sure, it is a nice check to have just in case but that doesn't make the finding Medium/High.

The Redstone team is also not sure what the chance of this happening is, but they confirm it is possible to return zero.

Again, they never validated this. "In some edge cases it can happen I think" is far from validating something. Also, I don't think that a private thread with a Redstone team member is a valid source of truth here. We don't know the affiliations of that Redstone team member.

Thankfully, the blockchain is public and millions of transactions have happened through that oracle. The writers of this finding can freely search any blockchain transaction they want and find a case where it returned 0. This would 100% prove that this finding is valid and not a speculation.

I'm not saying that that's what happened, but anyone could contact team members of various projects and agree to push an invalid finding as valid for all we know. This is not a valid source of truth and the finding could be proven with real world data. Again the burden of proof is on the writers of the finding.

[cvetanovv]

@MrValioBg I have to agree with what @Kalogerone wrote - https://github.com/sherlock-audit/2024-08-sentiment-v2-judging/issues/591#issuecomment-2381427230

To make sure this can really happen and not validate something that is not valid, can you show a transaction where this has happened?

[MrValioBg]

I'm not saying that that's what happened, but anyone could contact team members of various projects and agree to push an invalid finding as valid for all we know.

The SC was from a ticket from their official discord server, not a private thread, all of their team members were in it. Its a silly assumption that all of them would want to split a hundred bucks with me. However, I can agree that a valid source of truth should be something all of us can have access to, such as warning in the docs or proof on the blockchain.

Even though they confirmed that they will add this somewhere, its been a month now and I couldn't find anything related to that, not an open issue or a PR, so its either not that critical or they forgot.

Anyway, I took a little bit of time before responding here, to think it through and gather more information, as our goal is for this to be judged fairly. Given the circumstances and the limitations for public proof and the lack of action from the redstone team, at the current state it might be more appropriate and I am okay for this to be considered low severity.

cc: @cvetanovv @Kalogerone

[Kalogerone]

Yes ser I didn’t believe you did something sketchy intentionally, was just exaggerating to show how a discord message shouldn’t be a valid source of truth. Thank you for understanding though, very mature of you and rare to see in escalations.

[cvetanovv]

I see that we have come to the conclusion that this issue is Low severity, and therefore, I will reject the escalation.

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

[WangSecurity]

Result: Invalid Duplicate of #401

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• ZdravkoHr: rejected