Honour - `RedstoneOracle` priceTimestamp can be acurately determined , eliminating the need for a -3 mins worst case scenario

[sherlock-admin3]

Honour

Medium

RedstoneOracle priceTimestamp can be acurately determined , eliminating the need for a -3 mins worst case scenario

Summary

RedstoneOracle sets the priceTimestamp of a price feed update to block.timestamp - 3 mins. Assuming the worst case, priceTimestamp is 3 mins behind the actual timestamp of price update ,this 3 min lag is significant enough to dos the oracle when the price feed is not yet stale.

Especially when it is possible to retrieve the actual timestamp by overriding the validateTimestamp function.

Root Cause

In RedstoneOracle::updatePrice it is assumed that the price timestamp of the price feed cannot be retrieved:

https://github.com/sherlock-audit/2024-08-sentiment-v2/blob/main/protocol-v2/src/oracle/RedstoneOracle.sol#L60

 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];

        // 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;
    }

However this is not correct as we see in the getOracleNumericValuesFromTxMsg function and from the redstone docs:

  function getOracleNumericValuesFromTxMsg(bytes32[] memory dataFeedIds)
    internal
    view
    virtual
    returns (uint256[] memory)
  {
    (uint256[] memory values, uint256 timestamp) = _securelyExtractOracleValuesAndTimestampFromTxMsg(dataFeedIds);
->  validateTimestamp(timestamp);
    return values;
  }

/**
   * @dev This function may be overridden by the child consumer contract.
   * It should validate the timestamp against the current time (block.timestamp)
   * It should revert with a helpful message if the timestamp is not valid
   * @param receivedTimestampMilliseconds Timestamp extracted from calldata
   */
  function validateTimestamp(uint256 receivedTimestampMilliseconds) public view virtual {
    RedstoneDefaultsLib.validateTimestamp(receivedTimestampMilliseconds);
  }

The validateTimestamp function can be overridden. It's possible to retrieve the actual timestamp by overriding the validateTimestamp function and prevent any possible oracle DOS due to the time lag.

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

No response

Impact

Possible Oracle DOS due to lagging priceTimestamp

PoC

No response

Mitigation

Override validateTimestamp to retrieve the actual timestamp, as an example:

function validateTimestamp(uint256 timestamp) public view override {
  priceTimestamp = timestamp;
  super.validateTimestamp(timestamp);

}

[cawfree]

Escalate

Although a timestamp can indeed be parsed from the supplied observation, the watson's comments fail to respect that the three minutes worst-case is enshrined in the Redstone library itself:

// Requesting oracle values (without duplicates)
(uint256[] memory valuesWithoutDuplicates, uint256 timestamp) = _securelyExtractOracleValuesAndTimestampFromTxMsg(dataFeedIdsWithoutDuplicates);
@> validateTimestamp(timestamp);

source

@> uint256 constant DEFAULT_MAX_DATA_TIMESTAMP_DELAY_SECONDS = 3 minutes;
@> uint256 constant DEFAULT_MAX_DATA_TIMESTAMP_AHEAD_SECONDS = 1 minutes;

  error TimestampFromTooLongFuture(uint256 receivedTimestampSeconds, uint256 blockTimestamp);
  error TimestampIsTooOld(uint256 receivedTimestampSeconds, uint256 blockTimestamp);

@> function validateTimestamp(uint256 receivedTimestampMilliseconds) internal view {
    // Getting data timestamp from future seems quite unlikely
    // But we've already spent too much time with different cases
    // Where block.timestamp was less than dataPackage.timestamp.
    // Some blockchains may case this problem as well.
    // That's why we add MAX_BLOCK_TIMESTAMP_DELAY
    // and allow data "from future" but with a small delay
    uint256 receivedTimestampSeconds = receivedTimestampMilliseconds / 1000;

    if (block.timestamp < receivedTimestampSeconds) {
      if ((receivedTimestampSeconds - block.timestamp) > DEFAULT_MAX_DATA_TIMESTAMP_AHEAD_SECONDS) {
        revert TimestampFromTooLongFuture(receivedTimestampSeconds, block.timestamp);
      }
    } else if ((block.timestamp - receivedTimestampSeconds) > DEFAULT_MAX_DATA_TIMESTAMP_DELAY_SECONDS) {
      revert TimestampIsTooOld(receivedTimestampSeconds, block.timestamp);
    }
  }

source

So, although a nice recommendation for parsing the timestamp on the consumer, the 3 minutes delay period itself is already validated and enforced by Redstone itself.

Consequently this issue has no impact, since it is not possible to dos the oracle as suggested.

[sherlock-admin3]

Escalate

Although a timestamp can indeed be parsed from the supplied observation, the watson's comments fail to respect that the three minutes worst-case is enshrined in the Redstone library itself:

// Requesting oracle values (without duplicates)
(uint256[] memory valuesWithoutDuplicates, uint256 timestamp) = _securelyExtractOracleValuesAndTimestampFromTxMsg(dataFeedIdsWithoutDuplicates);
@> validateTimestamp(timestamp);

source

@> uint256 constant DEFAULT_MAX_DATA_TIMESTAMP_DELAY_SECONDS = 3 minutes;
@> uint256 constant DEFAULT_MAX_DATA_TIMESTAMP_AHEAD_SECONDS = 1 minutes;

  error TimestampFromTooLongFuture(uint256 receivedTimestampSeconds, uint256 blockTimestamp);
  error TimestampIsTooOld(uint256 receivedTimestampSeconds, uint256 blockTimestamp);

@> function validateTimestamp(uint256 receivedTimestampMilliseconds) internal view {
    // Getting data timestamp from future seems quite unlikely
    // But we've already spent too much time with different cases
    // Where block.timestamp was less than dataPackage.timestamp.
    // Some blockchains may case this problem as well.
    // That's why we add MAX_BLOCK_TIMESTAMP_DELAY
    // and allow data "from future" but with a small delay
    uint256 receivedTimestampSeconds = receivedTimestampMilliseconds / 1000;

    if (block.timestamp < receivedTimestampSeconds) {
      if ((receivedTimestampSeconds - block.timestamp) > DEFAULT_MAX_DATA_TIMESTAMP_AHEAD_SECONDS) {
        revert TimestampFromTooLongFuture(receivedTimestampSeconds, block.timestamp);
      }
    } else if ((block.timestamp - receivedTimestampSeconds) > DEFAULT_MAX_DATA_TIMESTAMP_DELAY_SECONDS) {
      revert TimestampIsTooOld(receivedTimestampSeconds, block.timestamp);
    }
  }

source

So, although a nice recommendation for parsing the timestamp on the consumer, the 3 minutes delay period itself is already validated and enforced by Redstone itself.

Consequently this issue has no impact, since it is not possible to dos the oracle as suggested.

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.

[Honour-d-dev]

Please understand the issue before escalating

This report is saying that the protocol always assume timestamp to be - 3 mins (worst case) , while it's possible that actual timestamp has a 3 minutes delay, this cannot be expected to always be the case.

The actual timestamp might be equal to current block.timestamp or even ahead as redstone prices are not on-chain(they're brought on-chain on request, you can read their docs), hence why they have a DEFAULT_MAX_DATA_TIMESTAMP_AHEAD_SECONDS.

In these cases the priceTimestamp will lag the actual timestamp of the feed by <= 4 minutes (4 mins is the valid time window i.e. 3min delay + 1 min ahead), this will cause the staleness check to revert when the feed is not actually stale.

If it were not possible to retrieve the actual feed timestamp as the code comments suggests, then this is an acceptable compromise, however , it's very possible to do so. As shown here

[ruvaag]

isn't this a duplicate of #23

[Honour-d-dev]

Nope, this is saying we can retrieve the actual timestamp of the feed by overriding the validateTimestamp() function.

like so:

function validateTimestamp(uint256 timestamp) public view override {
  priceTimestamp = timestamp/1000; //timestamp is in milliseconds
  super.validateTimestamp(timestamp);

}

Current implementation leads to cases where the recorded timestamp is behind the actual timestamp, which means some staleness checks can revert when prices aren't yet stale

[ruvaag]

i get that it's a different fix. but this is not a different issue, it's a different fix for the same issue.

could you expand on the attack path, impact and PoC --- what is the DoS vector and how is it executed? what is the root issue being solved through timestamp retrieval? why is it an issue apart from the reasons mentioned in 23 (and it's dupes). if it's a design optimization with no impact-likelihood it should be a valid low instead.

[cvetanovv]

I agree with the sponsor comment. This issue is a duplicate of #23.

I am planning to reject the escalation, but I duplicate this issue with #23.

[MrValioBg]

@cvetanovv It does not sound like this is the same issue as #23, as it does not clearly mention the root cause and tries to describe an attack path which seems invalid.

23 and its duplicates talk about bringing back older prices, which can result in arbitrage and using price discrepancies for liquidating positions.

This issue's title is implying a suggestion/optimization "RedstoneOracle priceTimestamp can be acurately determined , eliminating the need for a -3 mins worst case scenario".

The description is focused on a a possible DoS vulnerability, which does not seem valid, as @cawfree explained here

I think that the report is missing the part that the priceTimestamp has nothing to do with the timestamp passed through the calldata that is validated in the Redstone SDK's getOracleNumericValuesFromTxMsg() function.

From what @Honour-d-dev has mentioned: This statement is true:

This report is saying that the protocol always assume timestamp to be - 3 mins (worst case) , while it's possible that actual timestamp has a 3 minutes delay, this cannot be expected to always be the case.

However this isn't:

In these cases the priceTimestamp will lag the actual timestamp of the feed by <= 4 minutes (4 mins is the valid time window i.e. 3min delay + 1 min ahead), this will cause the staleness check to revert when the feed is not actually stale.

As the priceTimestamp is not the one trying to pass the staleness check in getOracleNumericValuesFromTxMsg() function, so DoS can't happen.

In Summary: As it does not describe a valid attack path, regarding IX Duplication rules

2. Identify at least a Medium impact
   3. Identify a valid attack path or vulnerability path

It should not be a duplicate, but rather invalid.

I've tried to be thorough and objective. If someone wants to elaborate on my statements, please do so.

cc: @cawfree @ruvaag @Honour-d-dev

[MrValioBg]

In addition to my last comment I want to also elaborate on this statement by @Honour-d-dev with a very clear example:

Current implementation leads to cases where the recorded timestamp is behind the actual timestamp, which means some staleness checks can revert when prices aren't yet stale

priceTimestamp is set to block.timestamp - 3 Minutes.

The passed actualTimestamp is retrieved from decoding the calldata of the call to the updatePrice() function.

• The actualTimestamp has to pass the validation checks for staleness, not the priceTimestamp, thus no DoS is possible. ( Though even if the priceTimestamp was passed, it should pass too, as its still in the valid range )
• The actualTimestamp is retrieved off-chain and submitted on-chain. If the currentTime - 3min < actualTimestamp < currentTime + 1min. The staleness check will pass, but this has nothing to do with the logic of Sentiment's protocol. This is how the Redstone oracle works.

I am very confident about this, however if @Honour-d-dev still thinks this is wrong, I propose that a PoC could be made for this.

[Honour-d-dev]

@MrValioBg While i'd agree that i don't also see the similarities between this report and #23 as mentioned by @ruvaag & @cvetanovv

The following section of your comment is wrong

From what @Honour-d-dev has mentioned: This statement is true:

This report is saying that the protocol always assume timestamp to be - 3 mins (worst case) , while it's possible that actual timestamp has a 3 minutes delay, this cannot be expected to always be the case.

However this isn't:

In these cases the priceTimestamp will lag the actual timestamp of the feed by <= 4 minutes (4 mins is the valid time window i.e. 3min delay + 1 min ahead), this will cause the staleness check to revert when the feed is not actually stale.

As the priceTimestamp is not the one trying to pass the staleness check in getOracleNumericValuesFromTxMsg() function, so DoS can't happen.

This report does not claim that there's a staleness check in getOracleNumericValuesFromTxMsg() function. The staleness check can be clearly seen in the getValueInEth() function which is used extensively across the protocol

    function getValueInEth(address, uint256 amt) external view returns (uint256) {
     @> if (priceTimestamp < block.timestamp - STALE_PRICE_THRESHOLD) revert RedstoneCoreOracle_StalePrice(ASSET);

        // scale amt to 18 decimals
        if (ASSET_DECIMALS <= 18) amt = amt * 10 ** (18 - ASSET_DECIMALS);
        else amt = amt / 10 ** (ASSET_DECIMALS - 18);

        // [ROUND] price is rounded down
        return amt.mulDiv(assetUsdPrice, ethUsdPrice);
    }

By always assuming the worst case timestamp (-3) for the price feed instead of just retrieving the actual timestamp, the recorded timestamp priceTimestamp will lag behind the actual timestamp by upto 4 mins in worst case( as the valid time window is -3 to +1). This means the staleness check in getValueInEth() can revert with stale price error when the price feed is not yet stale but has about <= 4 mins left.

I believe the issue here is very straight forward and i'm willing to explain further if there're any confusions still

[Honour-d-dev]

In addition to my last comment I want to also elaborate on this statement by @Honour-d-dev with a very clear example:

Current implementation leads to cases where the recorded timestamp is behind the actual timestamp, which means some staleness checks can revert when prices aren't yet stale

priceTimestamp is set to block.timestamp - 3 Minutes.

The passed actualTimestamp is retrieved from decoding the calldata of the call to the updatePrice() function.

• The actualTimestamp has to pass the validation checks for staleness, not the priceTimestamp, thus no DoS is possible. ( Though even if the priceTimestamp was passed, it should pass too, as its still in the valid range )
• The actualTimestamp is retrieved off-chain and submitted on-chain. If the currentTime - 3min < actualTimestamp < currentTime + 1min. The staleness check will pass, but this has nothing to do with the logic of Sentiment's protocol. This is how the Redstone oracle works.

I am very confident about this, however if @Honour-d-dev still thinks this is wrong, I propose that a PoC could be made for this.

The staleness check i'm referring to is the one in getValueInEth() it has nothing to do with how redstone oracle validates timestamp

[MrValioBg]

Ooh, okay, I might have misunderstood you before you, as you did not mention the getValueInEth(), prior to your last messagess. If I got it right you are referring to this scenario:

actualPrice is +1min in the future, the priceTimestamp which is saved will be -3min, thus this will make the 1 hour stale check fail a little earlier compared to the actualPrice?

 @> if (priceTimestamp < block.timestamp - STALE_PRICE_THRESHOLD) revert RedstoneCoreOracle_StalePrice(ASSET);

If thats the case I have some thoughts on that:

• Wasn't this the intended design? Considering the priceTimestamp stale, after 1 Hour, no matter if compared to the actualTimestamp its a little less?
• If we invalidate my question about the "intended design". Is this a valid DoS and medium severity, rather than low/info? - The 4 min difference on 60 minutes stale check is not a lot, and actually its in favour of the protocol. Meaning, that in the worst case we will make a price stale 3-4 minutes earlier, so after 56-57 mins, since it was submitted.

Maybe it could have a Medium impact is if there were off-chain feeds which gave you prices in an interval longer than 56-57minutes, which does not seem to be the case. Sentiment uses the Pull model There isn't such a huge delay for aggregated prices from off-chain nodes, as it can be seen here https://app.redstone.finance/app/tokens/ and https://app.redstone.finance/app/data-services/redstone-primary-prod/

In the pull model, there isn't a heartbeat of hours, as we submit the data to the blockchain ourselves, off-chain aggregatioins should happen at low intervals to ensure the oracle's integrity and after checking myself at least 50 feeds & all the data source nodes, all of them are aggregating prices within seconds to minutes, not hours.

The https://www.npmjs.com/package/@redstone-finance/sdk is recommending to use redstone-primary-prod Which is set to 10s interval, see here

So it seems like there won't be an impact from this issue.

cc: @cvetanovv @Honour-d-dev

[Honour-d-dev]

Again, how redstone choose to validate timestamps or heartbeat isn't relevant here. The protocol has chosen to use 1-hour as the max staleness of all redstone feeds - we can say it's a design decision.

The issue is that said design decision (i.e. to use 1 hour for staleness) isn't upheld correctly, which can cause price feeds not yet stale (i.e. below 1 hour in age) to revert on the staleness check.

@MrValioBg

[MrValioBg]

@Honour-d-dev I mentioned the redstone validation and heartbeat, because I was trying to provide a possible scenario where I believe your issue would've had an impact and would've been valid, if it wasn't for the actual intervals being that low.

From my experience and knowledge, in Sherlock, if such a "design decision" is not mentioned in the README that it should 100% hold (such as with the ERC strict compliances), it will not be treated as valid if it does not have a >= Medium impact.

I believe we should also see what the judge @cvetanovv thinks.

[Honour-d-dev]

I don't think compliance with staleness checks or lack of it, needs to be added to README, this type of bugs are common, all be it not this exact case

i will wait to see what Hoj thinks though 👍

[cvetanovv]

@Honour-d-dev @MrValioBg In the end, I didn't understand what the impact is? If we have a DoS here for a few blocks, then we have no loss of funds or something else. Only uncomfortable user experience. It will be called again later, or another oracle will be used.

[MrValioBg]

@cvetanovv We cannot possibly have a DoS, neither "uncomfortable user experience" as described here in this comment There is no actual impact, because all the nodes of Redstone aggregate price within seconds/minutes, so there won't be a case where we can only get a price once in an hour.

@Honour-d-dev claims that the issue is "compliance with staleness checks or lack of it", however as I stated here if the issue does not have an actual impact and is just about "compliance" it should not be valid, unless the "compliance" is required and stated explicitly in the readme.

[cvetanovv]

I agree with @MrValioBg last comment - https://github.com/sherlock-audit/2024-08-sentiment-v2-judging/issues/477#issuecomment-2393536650 Due to the lack of impact, I will accept the escalation and invalidate the issue.

Planning to accept the escalation and invalidate the issue.

[WangSecurity]

Result: Invalid Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• cawfree: accepted