search

sherlock-audit / 2023-12-flatmoney-judging

11 stars 9 forks source link

Bjorn_Bug - OracleModule#updatePythPrice() is inefficient and can lead to Unnecessary losses of funds for keepers #152

Closed sherlock-admin2 closed 6 months ago

sherlock-admin2 commented 7 months ago

Bjorn_Bug

high

OracleModule#updatePythPrice() is inefficient and can lead to Unnecessary losses of funds for keepers

Summary

OracleModule#updatePythPrice() is inefficient because it uses the updatePriceFeeds function which initiates fee payments for the keeper every time it's called in DelayedOrder#executeOrder regardless of whether the new data is the most recent or not.

Vulnerability Detail

The Pyth network interface Natspec describes the updatePriceFeeds function as follows:

The call will succeed even if the update is not the most recent.

This means each time the keeper calls DelayedOrder#executeOrder, they pay fees to updatePriceFeeds , even if the updated price is not the most recent. Alternatively, the Pyth network offers a more efficient function named updatePriceFeedsIfNecessary. This function is a wrapper around updatePriceFeeds and only allows updates when necessary, rejecting any fast updates if a price change isn't needed. A price update is considered necessary if the on-chain publishTime is older than the given publishTime, and it reverts if an update is not required, preventing unnecessary fee payments by the keeper.

According to Pyth protocol's data feeds for RETH/ETH at the time of writing this report, the price updates occur every 4 to 6 seconds, meaning there's a low probability of significant price changes for such pairs within a short time.

Consider this scenario with updatePriceFeeds for RETH/ETH (Pyth Network Price Feeds):

  1. The keeper calls DelayedOrder#executeOrder to execute any valid pending orders, short or long.
  2. updatePythPrice is invoked first as a modifier, internally calling offchainOracle.oracleContract.updatePriceFeeds to update the PythPrice before executing the orders.
  3. The keeper will still pay fees, even if the updated price is not the most recent
  4. This can lead to significant fund losses for keepers on a large scale due to the current implementation of updatePythPrice.

Impact

The keeper will loose ETH each time updatePythPrice() is called as a modifier when DelayedOrder#executeOrder() is used to execute pending orders. If keepers notice these losses, they might stop executing pending orders, and the system won't work as it should.

Code Snippet

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/OracleModule.sol#L69 https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/abstracts/OracleModifiers.sol#L9-L22 https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/DelayedOrder.sol#L386

Tool used

Manual Review

Recommendation

Consider replacing updatePriceFeeds with updatePriceFeedsIfNecessary in updatePythPrice function to ensure that the prices are updated and fees are paid only when necessary.

Check the link bellow for more details about both functions: https://github.com/pyth-network/pyth-crosschain/blob/main/target_chains/ethereum/sdk/solidity/IPyth.sol#L103-L107

sherlock-admin commented 6 months ago

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

takarez commented:

invalid

nevillehuang commented 6 months ago

Invalid, design improvement suggestion, where worse case scenario is simply paying more to ensure off-chain prices are always up to date which is not an invalid design choice.