commitRequested() front-run malicious invalid oralce

Summary

Both commitRequested() and commit() can modify lastCommittedPublishTime, and both check that they cannot pythPrice.publishTime<=lastCommittedPublishTime. This allows a malicious user to front-run commitRequested() to execute commit(), causing commitRequested() to revert, invalid oralce

Vulnerability Detail

Execution of the commitRequested() method restricts the lastCommittedPublishTime from going backward.

    function commitRequested(uint256 versionIndex, bytes calldata updateData)
        public
        payable
        keep(KEEPER_REWARD_PREMIUM, KEEPER_BUFFER, updateData, "")
    {
...

@>      if (pythPrice.publishTime <= lastCommittedPublishTime) revert PythOracleNonIncreasingPublishTimes();
@>        lastCommittedPublishTime = pythPrice.publishTime;
...

commit() has a similar limitation and can set lastCommittedPublishTime.

    function commit(uint256 versionIndex, uint256 oracleVersion, bytes calldata updateData) external payable {
        if (
            versionList.length > versionIndex &&                // must be a requested version
            versionIndex >= nextVersionIndexToCommit &&         // must be the next (or later) requested version
@>          oracleVersion == versionList[versionIndex]          // must be the corresponding timestamp
        ) {
            commitRequested(versionIndex, updateData);
            return;
        }
...
@>      if (pythPrice.publishTime <= lastCommittedPublishTime) revert PythOracleNonIncreasingPublishTimes();
@>      lastCommittedPublishTime = pythPrice.publishTime;
....

This leads to a situation where anyone can front-run commitRequested() and use his updateData to execute commit(). In order to satisfy the commit() constraint, we need to pass a commit() parameter set as follows

versionIndex= nextVersionIndexToCommit
oracleVersion = versionList[versionIndex] - 1 and oralceVersion > _latestVersion
pythPrice.publishTime >= versionList[versionIndex] - 1 + MIN_VALID_TIME_AFTER_VERSION

This way lastCommittedPublishTime will be modified, causing commitRequested() to execute with revert PythOracleNonIncreasingPublishTimes

Example: Given: nextVersionIndexToCommit = 10 versionList[10] = 200
_latestVersion = 100

when:

keeper exexute commitRequested(versionIndex = 10 , VAA{ publishTime = 205})
front-run execute `commit(versionIndex = 10 , oracleVersion = 200-1 , VAA{ publishTime = 205})
- versionIndex= nextVersionIndexToCommit （pass）
- oracleVersion = versionList[versionIndex] - 1 and oralceVersion > _latestVersion (pass)
- pythPrice.publishTime >= versionList[versionIndex] - 1 + MIN_VALID_TIME_AFTER_VERSION (pass)

By the time the keeper submits the next VVA, the price may have passed its expiration date

Impact

If the user can control the oralce invalidation, it can lead to many problems e.g. invalidating oracle to one's own detriment, not having to take losses Maliciously destroying other people's profits, etc.

Code Snippet

https://github.com/sherlock-audit/2023-09-perennial/blob/main/perennial-v2/packages/perennial-oracle/contracts/pyth/PythOracle.sol#L174

https://github.com/sherlock-audit/2023-09-perennial/blob/main/perennial-v2/packages/perennial-oracle/contracts/pyth/PythOracle.sol#L129

Tool used

Manual Review

Recommendation

check pythPrice whether valid for nextVersionIndexToCommit

    function commit(uint256 versionIndex, uint256 oracleVersion, bytes calldata updateData) external payable {
        // Must be before the next requested version to commit, if it exists
        // Otherwise, try to commit it as the next request version to commit
        if (
            versionList.length > versionIndex &&                // must be a requested version
            versionIndex >= nextVersionIndexToCommit &&         // must be the next (or later) requested version
            oracleVersion == versionList[versionIndex]          // must be the corresponding timestamp
        ) {
            commitRequested(versionIndex, updateData);
            return;
        }

        PythStructs.Price memory pythPrice = _validateAndGetPrice(oracleVersion, updateData);

        // Price must be more recent than that of the most recently committed version
        if (pythPrice.publishTime <= lastCommittedPublishTime) revert PythOracleNonIncreasingPublishTimes();
        lastCommittedPublishTime = pythPrice.publishTime;

        // Oracle version must be more recent than that of the most recently committed version
        uint256 minVersion = _latestVersion;
        uint256 maxVersion = versionList.length > versionIndex ? versionList[versionIndex] : current();

        if (versionIndex < nextVersionIndexToCommit) revert PythOracleVersionIndexTooLowError();
        if (versionIndex > nextVersionIndexToCommit && block.timestamp <= versionList[versionIndex - 1] + GRACE_PERIOD)
            revert PythOracleGracePeriodHasNotExpiredError();
        if (oracleVersion <= minVersion || oracleVersion >= maxVersion) revert PythOracleVersionOutsideRangeError();
+       if (nextVersionIndexToCommit < versionList.length) {
+           if (
+               pythPrice.publishTime >= versionList[nextVersionIndexToCommit] + MIN_VALID_TIME_AFTER_VERSION &&
+               pythPrice.publishTime <= versionList[nextVersionIndexToCommit] + MAX_VALID_TIME_AFTER_VERSION
+           ) revert PythOracleUpdateValidForPreviousVersionError();
+       }

        _recordPrice(oracleVersion, pythPrice);
        nextVersionIndexToCommit = versionIndex;
        _latestVersion = oracleVersion;
    }

sherlock-audit / 2023-09-perennial-judging

bin2chen - commitRequested() front-run malicious invalid oralce #27