Malicious keepers can manipulate the price when executing an order
Malicious keepers can manipulate the price when executing an order by selecting a price in favor of either the LPs or long traders, leading to a loss of assets to the victim's party.\\nWhen the keeper executes an order, it was understood from the protocol team that the protocol expects that the keeper must also update the Pyth price to the latest one available off-chain. In addition, the contest page mentioned that "an offchain price that is pulled by the keeper and pushed onchain at time of any order execution".\\nThis requirement must be enforced to ensure that the latest price is always used.\\n```\\nFile: DelayedOrder.sol\\n    function executeOrder(\\n        address account,\\n        bytes[] calldata priceUpdateData\\n    )\\n        external\\n        payable\\n        nonReentrant\\n        whenNotPaused\\n        updatePythPrice(vault, msg.sender, priceUpdateData)\\n        orderInvariantChecks(vault)\\n    {\\n        // Settle funding fees before executing any order.\\n        // This is to avoid error related to max caps or max skew reached when the market has been skewed to one side for a long time.\\n        // This is more important in case the we allow for limit orders in the future.\\n        vault.settleFundingFees();\\n..SNIP..\\n    }\\n```\\n\\nHowever, this requirement can be bypassed by malicious keepers. A keeper could skip or avoid the updating of the Pyth price by passing in an empty `priceUpdateData` array, which will pass the empty array to the `OracleModule.updatePythPrice` function.\\n```\\nFile: OracleModifiers.sol\\n    /// @dev Important to use this modifier in functions which require the Pyth network price to be updated.\\n    ///      Otherwise, the invariant checks or any other logic which depends on the Pyth network price may not be correct.\\n    modifier updatePythPrice(\\n        IFlatcoinVault vault,\\n        address sender,\\n        bytes[] calldata priceUpdateData\\n    ) {\\n        IOracleModule(vault.moduleAddress(FlatcoinModuleKeys._ORACLE_MODULE_KEY)).updatePythPrice{value: msg.value}(\\n            sender,\\n            priceUpdateData\\n        );\\n        _;\\n    }\\n```\\n\\nWhen the Pyth's `Pyth.updatePriceFeeds` function is executed, the `updateData` parameter will be set to an empty array.\\n```\\nFile: OracleModule.sol\\n    function updatePythPrice(address sender, bytes[] calldata priceUpdateData) external payable nonReentrant {\\n        // Get fee amount to pay to Pyth\\n        uint256 fee = offchainOracle.oracleContract.getUpdateFee(priceUpdateData);\\n\\n        // Update the price data (and pay the fee)\\n        offchainOracle.oracleContract.updatePriceFeeds{value: fee}(priceUpdateData);\\n\\n        if (msg.value - fee > 0) {\\n            // Need to refund caller. Try to return unused value, or revert if failed\\n            (bool success, ) = sender.call{value: msg.value - fee}("");\\n            if (success == false) revert FlatcoinErrors.RefundFailed();\\n        }\\n    }\\n```\\n\\nInspecting the source code of Pyth's on-chain contract, the `Pyth.updatePriceFeeds` function will not perform any update since the `updateData.length` will be zero in this instance.\\n```\\nfunction updatePriceFeeds(\\n    bytes[] calldata updateData\\n) public payable override {\\n    uint totalNumUpdates = 0;\\n    for (uint i = 0; i < updateData.length; ) {\\n        if (\\n            updateData[i].length > 4 &&\\n            UnsafeCalldataBytesLib.toUint32(updateData[i], 0) ==\\n            ACCUMULATOR_MAGIC\\n        ) {\\n            totalNumUpdates += updatePriceInfosFromAccumulatorUpdate(\\n                updateData[i]\\n            );\\n        } else {\\n            updatePriceBatchFromVm(updateData[i]);\\n            totalNumUpdates += 1;\\n        }\\n\\n        unchecked {\\n            i++;\\n        }\\n    }\\n    uint requiredFee = getTotalFee(totalNumUpdates);\\n    if (msg.value < requiredFee) revert PythErrors.InsufficientFee();\\n}\\n```\\n\\nThe keeper is permissionless, thus anyone can be a keeper and execute order on the protocol. If this requirement is not enforced, keepers who might also be LPs (or collude with LPs) can choose whether to update the Pyth price to the latest price or not, depending on whether the updated price is in favor of the LPs. For instance, if the existing on-chain price ($1000 per ETH) is higher than the latest off-chain price ($950 per ETH), malicious keepers will use the higher price of $1000 to open the trader's long position so that its position's entry price will be set to a higher price of $1000. When the latest price of $950 gets updated, the longer position will immediately incur a loss of $50. Since this is a zero-sum game, long traders' loss is LPs' gain.\\nNote that per the current design, when the open long position order is executed at $T2$, any price data with a timestamp between $T1$ and $T2$ is considered valid and can be used within the `executeOpen` function to execute an open order. Thus, when the malicious keeper uses an up-to-date price stored in Pyth's on-chain contract, it will not revert as long as its timestamp is on or after $T1$.\\n\\nAlternatively, it is also possible for the opposite scenario to happen where the keepers favor the long traders and choose to use a lower older price on-chain to execute the order instead of using the latest higher price. As such, the long trader's position will be immediately profitable after the price update. In this case, the LPs are on the losing end.\\nSidenote: The oracle's `maxDiffPercent` check will not guard against this attack effectively. For instance, in the above example, if the Chainlink price is $975 and the `maxDiffPercent` is 5%, the Pyth price of $950 or $1000 still falls within the acceptable range. If the `maxDiffPercent` is reduced to a smaller margin, it will potentially lead to a more serious issue where all the transactions get reverted when fetching the price, breaking the entire protocol.
Ensure that the keepers must update the Pyth price when executing an order. Perform additional checks against the `priceUpdateData` submitted by the keepers to ensure that it is not empty and `priceId` within the `PriceInfo` matches the price ID of the collateral (rETH), so as to prevent malicious keeper from bypassing the price update by passing in an empty array or price update data that is not mapped to the collateral (rETH).
Loss of assets as shown in the scenario above.
```\\nFile: DelayedOrder.sol\\n    function executeOrder(\\n        address account,\\n        bytes[] calldata priceUpdateData\\n    )\\n        external\\n        payable\\n        nonReentrant\\n        whenNotPaused\\n        updatePythPrice(vault, msg.sender, priceUpdateData)\\n        orderInvariantChecks(vault)\\n    {\\n        // Settle funding fees before executing any order.\\n        // This is to avoid error related to max caps or max skew reached when the market has been skewed to one side for a long time.\\n        // This is more important in case the we allow for limit orders in the future.\\n        vault.settleFundingFees();\\n..SNIP..\\n    }\\n```\\n