LTDingZhen - Keepers can avoid `updatePythPrice` by pass in an empty `priceUpdateData[]`

[sherlock-admin2]

LTDingZhen

medium

Keepers can avoid updatePythPrice by pass in an empty priceUpdateData[]

Summary

In LimitOrder and DelayedOrder, when keepers tries to execute a user's order via executeLimitOrder or executeOrder, they are forced to update Pyth RETH/USD oracle. However, Keepers can bypass the update to make more money.

Vulnerability Detail

executeLimitOrder and executeOrder are protected by modifier updatePythPrice:

//OracleModifiers.sol
modifier updatePythPrice(
    IFlatcoinVault vault,
    address sender,
    bytes[] calldata priceUpdateData
) {
    IOracleModule(vault.moduleAddress(FlatcoinModuleKeys._ORACLE_MODULE_KEY)).updatePythPrice{value: msg.value}(
        sender,
        priceUpdateData
    );
    _;
}

which tries to update Pyth oracle with priceUpdateData passed in by keeper:

function updatePythPrice(address sender, bytes[] calldata priceUpdateData) external payable nonReentrant {
    // Get fee amount to pay to Pyth
    uint256 fee = offchainOracle.oracleContract.getUpdateFee(priceUpdateData);

    // Update the price data (and pay the fee)
    offchainOracle.oracleContract.updatePriceFeeds{value: fee}(priceUpdateData);

    if (msg.value - fee > 0) {
        // Need to refund caller. Try to return unused value, or revert if failed
        (bool success, ) = sender.call{value: msg.value - fee}("");
        if (success == false) revert FlatcoinErrors.RefundFailed();
    }
}

Since Solidity allows function arguments to have dynamic arrays of values, and these dynamic arrays can be empty. So keepers can pass in an empty priceUpdateData to cheat the modifier.

It is worth noting that Pyth oracle contract does accept empty array input:

//https://github.com/pyth-network/pyth-crosschain/blob/main/target_chains/ethereum/contracts/contracts/pyth/Pyth.sol#L71-L95
function updatePythPrice(address sender, bytes[] calldata priceUpdateData) external payable nonReentrant {
    // Get fee amount to pay to Pyth
    uint256 fee = offchainOracle.oracleContract.getUpdateFee(priceUpdateData);

    // Update the price data (and pay the fee)
    offchainOracle.oracleContract.updatePriceFeeds{value: fee}(priceUpdateData);

    if (msg.value - fee > 0) {
        // Need to refund caller. Try to return unused value, or revert if failed
        (bool success, ) = sender.call{value: msg.value - fee}("");
        if (success == false) revert FlatcoinErrors.RefundFailed();
    }
}

//https://github.com/pyth-network/pyth-crosschain/blob/main/target_chains/ethereum/contracts/contracts/pyth/Pyth.sol#L107-L133
function getUpdateFee(bytes[] calldata updateData) public view override returns (uint feeAmount) {
    uint totalNumUpdates = 0;
    for (uint i = 0; i < updateData.length; i++) {
        if (
            updateData[i].length > 4 &&
            UnsafeCalldataBytesLib.toUint32(updateData[i], 0) ==
            ACCUMULATOR_MAGIC
        ) {
            (
                uint offset,
                UpdateType updateType
            ) = extractUpdateTypeFromAccumulatorHeader(updateData[i]);
            if (updateType != UpdateType.WormholeMerkle) {
                revert PythErrors.InvalidUpdateData();
            }
            totalNumUpdates += parseWormholeMerkleHeaderNumUpdates(
                updateData[i],
                offset
            );
        } else {
            totalNumUpdates += 1;
        }
    }
    return getTotalFee(totalNumUpdates);
}

Since the latest offer between Chainlink and Pyth is selected in OracleModule's getPrice() and the Chainlink's maxAge is greater than its heartbeat, in most cases getPrice() will not revert.

As long as the oracle price is updated after the order executability time, the order would succeed. So keepers would

Impact

Since Keepers are selfish, they will choose to update Pyth oracle as infrequently as possible or wait for others to update the oracle before executing transactions, so the oracle will be updated less frequently than expected, and user's orders would always be executed at a staler price.

Code Snippet

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/abstracts/OracleModifiers.sol

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/OracleModule.sol#L64-L76

Tool used

Manual Review

Recommendation

Keepers should not be allowed to determine priceUpdateData.

Duplicate of #194

[sherlock-admin]

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

takarez commented:

invalid