zarkk01 - RedStone oracle is vulnerable because ```updatePrice``` is not called during the ```getEthValue``` function.

[sherlock-admin2]

zarkk01

Medium

RedStone oracle is vulnerable because updatePrice is not called during the getEthValue function.

Summary

Redstone oracle doesn't work as expected returning outdated or user selected prices leading to every asset using it return wrong ETH values.

Vulnerability Detail

[!NOTE] All off-chain mechanisms of Sentiment protocol in the scope of this audit are stated in this section of README

As we can see in the RedstoneOracle contract the actual ethUsdPrice and assetUsdPrice are state variables which need to be updated every time the getValueInEth function be called so to calculate the real value of the asset in ETH. We can see the implementation here :

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

However, the updatePrice function is not called from anywhere, not even from inside the getValueInEth function which should seem logical.

Impact

Combined with the fact that the updatePrice function can be called by anyone "giving" the price 3 minutes of liveness, the impact/result of this vulnerability is someone to take advantage of a price which is not updated and get a wrong value of the asset in ETH, either lower or higher than the real one. For example, he can borrow with the wrong price and repay with the right price which is a bit higher, so return less amount that he took.

Code Snippet

Here is the updatePrice of Redstone oracle :

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

Link to code

Tool used

Manual Review

Recommendation

Consider calling updatePrice in the getEthValue function :

function getValueInEth(address, uint256 amt) external view returns (uint256) {
+       updatePrice();
        // ...
    }

[z3s]

Low/Info; this kind of functions are called regularly by a bot.

[ZeroTrust01]

Escalate judge：this kind of functions are called regularly by a bot. ——There is no mention of this anywhere, Then, what is the time interval between the calls? This should be a valid issue.

[sherlock-admin3]

Escalate judge：this kind of functions are called regularly by a bot. ——There is no mention of this anywhere, Then, what is the time interval between the calls? This should be a valid 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.

[cvetanovv]

Тhis is a Low severity issue because updatePrice() is external and can be called before the price is taken. This way the price will not be outdated.

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

[ZeroTrust01]

Тhis is a Low severity issue because updatePrice() is external and can be called before the price is taken. This way the price will not be outdated.

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

I cannot agree with this. My finding https://github.com/sherlock-audit/2024-08-sentiment-v2-judging/issues/310 mentions how a malicious user could exploit this to manipulate the price.

A borrower could call updatePrice() when the collateral price is high, but refrain from calling updatePrice() when the price is low, thereby maintaining an artificially inflated collateral value.

This is a medium-level issue.

[cvetanovv]

A borrower could call updatePrice() when the collateral price is high, but refrain from calling updatePrice() when the price is low, thereby maintaining an artificially inflated collateral value.

This is a very rare edge case because the function will be called constantly by bots or other users.

The sponsor also confirmed that there would be bots calling the function.

My decision to reject the escalation remains.

[ZeroTrust01]

The sponsor also confirmed that there would be bots calling the function.

I cannot agree with this. According to Sherlock’s rules： https://docs.sherlock.xyz/audits/judging/judging#ii.-criteria-for-issue-severity

the guidelines in the README, there are no bots calling the function updatePrice().

Q: Are there any off-chain mechanisms or off-chain procedures for the protocol (keeper bots, arbitrage bots, etc.)?

Liquidator bots: maintain protocol solvency through timely liquidation of risky positions Reallocation bots: used to rebalance SuperPool deposits among respective base pools

The sponsor did not publicly disclose this information during the competition, so it cannot be used as a basis for the judge’s decision.

And there’s no need for any bots here; simply calling updatePrice() within the getValueInEth() function would suffice.

[cvetanovv]

I think this issue could be Medium severity.

Indeed, the protocol did not specify in the Readme that they would use such bots, so we can't take that into consideration.

The other reason is that if the price satisfies a user and the real price is not to his advantage, he will not call the function.

I am planning to accept the escalation and make this issue Medium.

[Evert0x]

Result: Medium Has Duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• ZeroTrust01: accepted

[ZeroTrust01]

The issue #310 has not been added label (Medium Reward) @cvetanovv @Evert0x Thanks

[cvetanovv]

@ZeroTrust01 will be added at the end. As long as it is duplicated for a valid issue, then the system will not allow the results to come out before the label is added.