PriceOracle.getPrice doesn't check for stale price

Summary

PriceOracle.getPrice doesn't check for stale price. As result protocol can make decisions based on not up to date prices, which can cause loses.

Vulnerability Detail

PriceOracle.getPrice function is going to provide asset price using chain link price feeds. https://github.com/sherlock-audit/2023-05-ironbank/blob/main/ib-v2/src/protocol/oracle/PriceOracle.sol#L66-L72

    function getPriceFromChainlink(address base, address quote) internal view returns (uint256) {
        (, int256 price,,,) = registry.latestRoundData(base, quote);
        require(price > 0, "invalid price");

        // Extend the decimals to 1e18.
        return uint256(price) * 10 ** (18 - uint256(registry.decimals(base, quote)));
    }

This function doesn't check that prices are up to date. Because of that it's possible that price is not outdated which can cause financial loses for protocol.

Impact

Protocol can face bad debt.

Code Snippet

Provided above

Tool used

Manual Review

Recommendation

You need to check that price is not outdated by checking round timestamp.

ibsunhub commented 1 year ago

Same with the answer to #25. It's not practical to setup different heartbeat for individual markets. And we have a backend to monitor the price deviation.

0xffff11 commented 1 year ago

Due to the off-chain system by Iron, issue can be a low. (Does not really affect the current state of the contracts) @ibsunhub Is it the right resolution, or thinking more about invalid?

ib-tycho commented 1 year ago

We still think this is invalid, thanks

0xffff11 commented 1 year ago

Because Iron's off-chain safeguard, invalid

bzpassersby commented 1 year ago

Escalate for 10 USDC I think this is wrongly classified as invalid. (1) It's impossible for Watsons to know that the protocol has off-chain safeguards because the protocol explicitly said there are no off-chain mechanisms in the contest info. It's unfair for Watsons who might be misled by this answer.

Q: Are there any off-chain mechanisms or off-chain procedures for the protocol (keeper bots, input validation expectations, etc)?
nope

(2)It's right and should be encouraged for Watsons to point-out insufficient on-chain checks. The current code ignores any data freshness-related variables when consuming chainlink data, which is clearly not the best practice.

And it's understandable that the protocol chose to implement such checks off-chain. But since Watsons wouldn't have known about this and that the code itself clearly has flaws. This should be at least low/informational. It's unfair for Watsons to be punished because of this.

sherlock-admin commented 1 year ago

Escalate for 10 USDC I think this is wrongly classified as invalid. (1) It's impossible for Watsons to know that the protocol has off-chain safeguards because the protocol explicitly said there are no off-chain mechanisms in the contest info. It's unfair for Watsons who might be misled by this answer.
Q: Are there any off-chain mechanisms or off-chain procedures for the protocol (keeper bots, input validation expectations, etc)?
nope
(2)It's right and should be encouraged for Watsons to point-out insufficient on-chain checks. The current code ignores any data freshness-related variables when consuming chainlink data, which is clearly not the best practice.

And it's understandable that the protocol chose to implement such checks off-chain. But since Watsons wouldn't have known about this and that the code itself clearly has flaws. This should be at least low/informational. It's unfair for Watsons to be punished because of this.

You've created a valid escalation for 10 USDC!

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.