OracleVersion will not be invalid

Summary

KeeperOracle._commitRequested() has been modified so that if timeout takes latestVersion price, will not be 0. This will cause OracleVersion.valid to always be true

Vulnerability Detail

The code for KeeperOracle._commitRequested() after this version change is as follows:

    function _commitRequested(OracleVersion memory version) private returns (bool) {
        if (block.timestamp <= (next() + timeout)) {
            if (!version.valid) revert KeeperOracleInvalidPriceError();
            _prices[version.timestamp] = version.price;
        } else {
@>          _prices[version.timestamp] = _prices[_global.latestVersion];
        }
        _global.latestIndex++;
        return true;
    }

The above logic has been modified to take the last price after the timeout, _prices[version.timestamp] = _prices[_global.latestVersion]; No more price=0. But determining whether an oracle is valid is still determining whether it's 0 or not.

    function at(uint256 timestamp) public view returns (OracleVersion memory oracleVersion) {
        (oracleVersion.timestamp, oracleVersion.price) = (timestamp, _prices[timestamp]);
@>      oracleVersion.valid = !oracleVersion.price.isZero();
    }

This way the timeout oralce is modified to be valid and the oracle becomes all valid. This breaks the logic that relied on the oralce being invalid.

Impact

OracleVersion will no longer be invalid, and market logic that relies on oralce to be invalid will all be incorrect

Code Snippet

https://github.com/sherlock-audit/2024-02-perennial-v2-3/blob/main/perennial-v2/packages/perennial-oracle/contracts/keeper/KeeperOracle.sol#L158

Tool used

Manual Review

Recommendation

Suggestion, record timeout But note that there may be a problem if you use historical invalid prices, the earlier invalid price was 0, not previous price.

contract KeeperOracle is IKeeperOracle, Instance {
...
    mapping(uint256 => mapping(IMarket => EnumerableSet.AddressSet)) private _localCallbacks;
+   mapping(uint256 => bool) private _isTimeout;
    /// @notice Constructs the contract
    /// @param timeout_ The timeout for a version to be committed
    constructor(uint256 timeout_)  {
        timeout = timeout_;
    }

    function _commitRequested(OracleVersion memory version) private returns (bool) {
        if (block.timestamp <= (next() + timeout)) {
            if (!version.valid) revert KeeperOracleInvalidPriceError();
            _prices[version.timestamp] = version.price;
        } else {
            _prices[version.timestamp] = _prices[_global.latestVersion];
+           _isTimeout[version.timestamp]= true;
        }
        _global.latestIndex++;
        return true;
    }

    function at(uint256 timestamp) public view returns (OracleVersion memory oracleVersion) {
        (oracleVersion.timestamp, oracleVersion.price) = (timestamp, _prices[timestamp]);
-       oracleVersion.valid = !oracleVersion.price.isZero();
+       oracleVersion.valid = !oracleVersion.price.isZero() && !_isTimeout[timestamp];
    }

Duplicate of #6

sherlock-admin3 commented 5 months ago

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

panprog commented:

valid medium, dup of #6

sherlock-admin4 commented 4 months ago

The protocol team fixed this issue in the following PRs/commits: https://github.com/equilibria-xyz/perennial-v2/pull/308