Obsidian - The RedstoneCoreOracle has a constant stale price threshold, this is dangerous to use with tokens that have a smaller threshold as the oracle will report stale prices as valid

[sherlock-admin3]

Obsidian

Medium

The RedstoneCoreOracle has a constant stale price threshold, this is dangerous to use with tokens that have a smaller threshold as the oracle will report stale prices as valid

Summary

Different tokens have different STALE_PRICE_THRESHOLD. The protocol uses a constant STALE_PRICE_THRESHOLD = 3600 for all tokens in the RedstoneCoreOracle.

The issue arises when the token actually has a STALE_PRICE_THRESHOLD < 3600, then the oracle will report the stale price as valid.

Here are some tokens whose redstone priceFeed has a STALE_PRICE_THRESHOLD < 3600 (1 hour)

1. TRX/USD 10 minutes
2. BNB/USD 1 minute

Root Cause

using a constant STALE_PRICE_THRESHOLD = 3600, rather than setting one for each token

Internal pre-conditions

No response

External pre-conditions

Token has a threshold < 3600

Attack Path

No response

Impact

The protocol will report stale prices as valid, this results in collateral being valued using stale prices.

It will lead to unfair liqudiations due to stale price valuation of collateral AND/OR a position not being liquidated due to stale price valuation of collateral

It will also lead to borrowing a wrong amount due to stale price valuation of collateral

PoC

No response

Mitigation

Set a unique STALE_PRICE_THRESHOLD for each token, similar to the chainlink oracle

[sherlock-admin2]

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

z3s commented:

Each asset has their own instance of a RedstoneOracle, so this param can be changed

[0xspearmint1]

escalate

The lead judge states that since each asset has it's own instance of the oracle the param can be changed

1. Firstly, the STALE_PRICE_THRESHOLD is a constant variable that is already set, there is no evidence that the team intended to change the currently set constant variable.

2. Secondly, since each oracles instance uses 2 price feeds to determine the USD price of the asset (Asset/ETH and ETH/USD), as long as the asset has a different threshold to ETH the described issue in the report will occur.

[sherlock-admin3]

escalate

The lead judge states that since each asset has it's own instance of the oracle the param can be changed

1. Firstly, the STALE_PRICE_THRESHOLD is a constant variable that is already set, there is no evidence that the team intended to change the currently set constant variable.

2. Secondly, since each oracles instance uses 2 price feeds to determine the USD price of the asset (Asset/ETH and ETH/USD), as long as the asset has a different threshold to ETH the described issue in the report will occur.

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.

[ruvaag]

I think this should be a low because the intended behavior in the described case would be to use the worst stale price threshold which should mitigate this

[0xspearmint1]

What do you mean by the worst stale price? @ruvaag

If you mean the smaller one, this will cause a serious DOS issue for the token with a larger threshold (liquidations will revert).

If you mean the larger one, this will allow borrowing the other token at a stale price.

The only mitigation is to have a seperate threshold for each token.

[cvetanovv]

I agree that the constant STALE_PRICE_THRESHOLD is not good to be hardcoded to 1 hour because each token pair has a different stale period when it needs to be updated.

Because of this, I agree that this issue is more of a Medium because the price may be outdated.

I plan to accept the escalation and make this issue a Medium severity.

[0xspearmint1]

Hi @cvetanovv #346 is not a duplicate of this issue, it is actually invalid.

This issue is about using a constant STALE_PRICE_THRESHOLD

#346 describes a totally different attack vector which claims that the threshold is too short (threshold is set by the admin).

[HHK-ETH]

Agree, 346 is similar but incomplete. It only talks about the max duration threshold and not the constant itself. As it was correctly pointed out it could also be an issue to use a duration too small.

It should be removed from duplicates 👍

[cvetanovv]

@0xspearmint1 @HHK-ETH Thanks for noting that #346 is not a duplicate of this issue. And it indeed uses a different attack vector.

My decision is to accept the escalation and make this issue and its duplicates Medium severity without #346.

[WangSecurity]

Result: Medium Has duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• 0xspearmint1: accepted