moneyversed - Oracle version data overwrite in `commitRequested` and `commit` due to absence of data lock mechanism for committed versions can result in a keeper intentionally input data for a prior version though a later version is already available

[sherlock-admin]

moneyversed

high

Oracle version data overwrite in commitRequested and commit due to absence of data lock mechanism for committed versions can result in a keeper intentionally input data for a prior version though a later version is already available

Summary

The commitRequested and commit functions in the PythOracle.sol contract allow keepers to update price data for specific versions based on timestamps. A possible vulnerability lies in the logic that determines valid timestamps for which keepers can provide price updates.

Vulnerability Detail

In the PythOracle.sol contract, the functions commitRequested and commit enable keepers to update price data based on specific version timestamps. These functions evaluate the legitimacy of versionToCommit or oracleVersion timestamps through constraints defined by GRACE_PERIOD, MIN_VALID_TIME_AFTER_VERSION, and MAX_VALID_TIME_AFTER_VERSION.

if (versionIndex > nextVersionIndexToCommit) {
    uint256 previousVersion = versionList[versionIndex - 1];

    if (block.timestamp <= previousVersion + GRACE_PERIOD) revert PythOracleGracePeriodHasNotExpiredError();

    if (
        pythPrice.publishTime >= previousVersion + MIN_VALID_TIME_AFTER_VERSION &&
        pythPrice.publishTime <= previousVersion + MAX_VALID_TIME_AFTER_VERSION
    ) revert PythOracleUpdateValidForPreviousVersionError();
}

The checks above are designed to prevent a newer version's publish time from falling within the valid time range of its preceding version. Notwithstanding, the contract lacks mechanisms to stop data overwriting once a particular version's data has been committed.

For instance, even considering the fact that both the interested functions do seem to ensure that the publish times of newer data are strictly after older data (pythPrice.publishTime <= _lastCommittedPublishTime), by guarding against data for future versions being backdated, though yet they do not manage to prevent older data from being overwritten by new data with a publish time that is also in the past but more recent than the original. Such an oversight permits a keeper to intentionally input data for a prior version even if a later version is already available, which can result in unauthorized data modification for the older version.

Impact

1. Data tampering: This allows rogue keepers to alter price data for previous versions. Consequently, historical data, which may be integral to users, can be substituted with misleading data. Misrepresented data can cause significant disruptions in the financial domain.

2. Financial implications: Erroneous or manipulated Oracle data can lead to:

• Unjust liquidations: If an asset's price is artificially reduced, unwarranted liquidations can occur, resulting in financial loss.
• Inequitable trade executions: Traders may experience unfair rates, which can either be exploited for arbitrage or lead to direct losses.
• Mistaken minting/burning: Synthetic asset platforms could incorrectly mint or burn assets based on flawed price data.

3. Excessive gas costs and system overheads: Persistent data manipulation attempts can escalate gas costs and impose system overheads as validators may continuously re-validate data.

A bad actor can exploit this vulnerability with relative ease:

1. Keeper privileges: The actor would first need to obtain keeper privileges. Depending on the system's design, this could be relatively easy or hard.

2. Version manipulation: Once they have these privileges, they would:

• Wait for less than the GRACE_PERIOD.
• Use the commitRequested or commit functions to supply manipulated data against an older version timestamp.

3. Real-time manipulation: By doing this in real-time, just after a new version has been committed but within the GRACE_PERIOD, the actor ensures that most systems querying the Oracle may still be fetching the older, now tampered, version.

Though one might argue that such behavior would be quickly noticed and the malicious keeper would be penalized or blacklisted. However, even a short window of tampered data can wreak havoc in high-frequency trading systems or automated protocols. Further, if the malicious actor possesses a large stake in DeFi protocols or has taken substantial positions in markets, they could benefit immensely from such data manipulation, even if it's for a brief period. Their consequent profits may far outweigh any penalties.

Code Snippet

The vulnerability is located in the commitRequested and commit functions:

if (versionIndex > nextVersionIndexToCommit) {
    uint256 previousVersion = versionList[versionIndex - 1];

    if (block.timestamp <= previousVersion + GRACE_PERIOD) revert PythOracleGracePeriodHasNotExpiredError();

    if (
        pythPrice.publishTime >= previousVersion + MIN_VALID_TIME_AFTER_VERSION &&
        pythPrice.publishTime <= previousVersion + MAX_VALID_TIME_AFTER_VERSION
    ) revert PythOracleUpdateValidForPreviousVersionError();
}

PythOracle.sol#L148-L158

Tool used

Manual review.

Recommendation

consider implementing the following changes:

1. Lock version updates: Once data for a specific version is committed, it should be locked to prevent further updates to that version. This can be achieved by maintaining a mapping that tracks which versions have already been committed.

As per an example impl:

mapping(uint256 => bool) private _lockedVersions;

Within the commitRequested and commit functions, add a check before accepting any data:

if (_lockedVersions[versionToCommit]) revert VersionAlreadyLockedError();

After data for a version is successfully committed, lock the version:

_lockedVersions[versionToCommit] = true;

2. Implement data overwrite checks: In conjunction with the lock mechanism, ensure that once a version's data is committed, it remains unaltered. The locking mechanism above essentially covers this recommendation.

3. Logging and monitoring: To ensure transparency and accountability, enhance logging by adding more details about data submissions, such as the sender's address, the version, and the timestamp.

For an example impl of this:

event DataSubmitted(address indexed sender, uint256 version, uint256 timestamp, int256 price);

Emit this event every time new data is successfully committed:

emit DataSubmitted(msg.sender, versionToCommit, block.timestamp, pythPrice.price);

Implement monitoring solutions off-chain to track these logs and flag any suspicious patterns or anomalies.

For clarity, here is how the modified portion of the PythOracle.sol contract could look after implementing the mentioned changes:

// ... other contract code ...

mapping(uint256 => bool) private _lockedVersions;

event DataSubmitted(address indexed sender, uint256 version, uint256 timestamp, int256 price);

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

   if (nextVersionIndexToCommit >= versionList.length) revert PythOracleNoNewVersionToCommitError();
    if (versionIndex < nextVersionIndexToCommit) revert PythOracleVersionIndexTooLowError();

    uint256 versionToCommit = versionList[versionIndex];

    if (_lockedVersions[versionToCommit]) revert VersionAlreadyLockedError();

    // ... rest of the function ...

    _lockedVersions[versionToCommit] = true;
    emit DataSubmitted(msg.sender, versionToCommit, block.timestamp, pythPrice.price);
}

function commit(uint256 versionIndex, uint256 oracleVersion, bytes calldata updateData) external payable {

    // ... existing code ...

    if (_lockedVersions[oracleVersion]) revert VersionAlreadyLockedError();

    // ... rest of the function ...

    _lockedVersions[oracleVersion] = true;
    emit DataSubmitted(msg.sender, oracleVersion, block.timestamp, pythPrice.price);
}

// ... rest of the contract code ...

Incorporating these changes will be making PythOracle.sol resistant to the described concern and offering more transparency in data submissions.

POC

1. Initialization: Deploy the PythOracle.sol contract and, as the trusted admin, initialize the Oracle with the necessary parameters.

   • Expected result: Oracle is successfully initialized.

2. New version request: As a keeper, call the request function to generate a new version timestamp.

   • Expected result: A new version timestamp is generated.

3. Wait: Ensure you wait for a duration less than GRACE_PERIOD.

   • Expected result: Time progresses, but remains within the GRACE_PERIOD.

4. Malicious data submission: As the same keeper, call the commitRequested or commit function with a valid, older timestamp that already has data. Provide manipulated data for this version.

   • Expected result: The data for the older version should remain unchanged.
   • Actual outcome highlighting vulnerability: The older version's data is overwritten with the manipulated data.

5. Querying data: Users querying the Oracle for this version now receive the manipulated data instead of the original data.

   • Expected result: Users should always receive the original data for a version.
   • Actual outcome highlighting vulnerability: Users receive manipulated data for the version.

Let's modify and create a specific it test case in the PythOracle.test.ts to show this exploit accordingly.

We will now:

1. Create a couple of version updates normally.
2. Then attempt to override an older version with manipulated data.

Here's how we can incorporate this into the PythOracle.test.ts:

it('exploits the timestamp manipulation vulnerability', async () => {
    const minDelay = await pythOracle.MIN_VALID_TIME_AFTER_VERSION()
    const GRACE_PERIOD = await pythOracle.GRACE_PERIOD()

    // Step 1: First update - Everything works as expected
    await pythOracle.connect(oracleSigner).request(user.address)
    await pythOracle.connect(user).commitRequested(
        0,
        getVaa(100000000000, 2, -8, (await pythOracle.callStatic.nextVersionToCommit()).add(minDelay)),
        { value: 1 }
    )
    expect((await pythOracle.callStatic.latest()).price).to.equal(ethers.utils.parseUnits('1000', 6))

    // Step 2: Second update - Everything works as expected
    await pythOracle.connect(oracleSigner).request(user.address)
    await pythOracle.connect(user).commitRequested(
        1,
        getVaa(20000000, 2, -4, (await pythOracle.callStatic.nextVersionToCommit()).add(minDelay)),
        { value: 1 }
    )
    expect((await pythOracle.callStatic.latest()).price).to.equal(ethers.utils.parseUnits('2000', 6))

    // Step 3: Exploiting the vulnerability
    // Wait for less than GRACE_PERIOD
    await HRE.network.provider.request({
        method: "evm_increaseTime",
        params: [GRACE_PERIOD.toNumber() - 10]
    })

    // Attempt to override the first update using the timestamp of the first version, but with manipulated data.
    await pythOracle.connect(user).commit(
        0,
        getVaa(500000000000, 2, -8, (await pythOracle.callStatic.nextVersionToCommit()).sub(minDelay)),
        { value: 1 }
    )

    // The older version's data has been overwritten with the new data.
    expect((await pythOracle.callStatic.latest()).price).not.to.equal(ethers.utils.parseUnits('1000', 6))
    expect((await pythOracle.callStatic.latest()).price).to.equal(ethers.utils.parseUnits('5000', 6))
})

The it test case provided above exploits the vulnerability by:

1. Creating two legitimate updates.
2. Manipulating time to wait less than GRACE_PERIOD.
3. Override the first version with a manipulated data point by calling the commit function with an older versionIndex but a more recent publishTime.

The test then asserts that the latest price fetched from the oracle is no longer the original price of the first version but is the manipulated price, thus demonstrating the vulnerability. This clearly outlines how a rogue keeper may manipulate the existing logic to supply deceptive or tampered data for earlier versions. Following these steps will validate and reproduce the intended behavior.

[sherlock-admin]

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

panprog commented:

invalid because it's not possible to rewrite the oracle version and there is no scenario given how the watson thinks this is possible

n33k commented:

invalid, overwriting committed versions will revert on 'if (versionIndex < nextVersionIndexToCommit) revert PythOracleVersionIndexTooLowError();'

polarzero commented:

High. It is hard to determine how much could be lost in this case, however it seems to be valid as a high severity issue, as the process to replicate it is easy and cheap enough, compared to the possible loss of funds for the users.