The issue titled "Oracle can return different prices in same transaction" is not remediated.

Summary

The issue titled "Oracle can return different prices in same transaction" (Issue 216) has not been remediated.

Vulnerability Detail

The issue titled "Oracle can return different prices in same transaction" (Issue 216) has not been remediated. In the current implementation, it is still possible to submit and read two different prices in the same transaction to carry out the attack described in the report.

https://github.com/sherlock-audit/2024-03-flat-money-fix-review-contest/blob/main/flatcoin-v1/src/abstracts/OracleModifiers.sol#L12

File: OracleModifiers.sol
10:     /// @dev Important to use this modifier in functions which require the Pyth network price to be updated.
11:     ///      Otherwise, the invariant checks or any other logic which depends on the Pyth network price may not be correct.
12:     modifier updatePythPrice(
13:         IFlatcoinVault vault,
14:         address sender,
15:         bytes[] calldata priceUpdateData
16:     ) {
17:         IOracleModule(vault.moduleAddress(FlatcoinModuleKeys._ORACLE_MODULE_KEY)).updatePythPrice{value: msg.value}(
18:             sender,
19:             priceUpdateData
20:         );
21:         _;
22:     }

https://github.com/sherlock-audit/2024-03-flat-money-fix-review-contest/blob/main/flatcoin-v1/src/OracleModule.sol#L58

File: OracleModule.sol
58:     function updatePythPrice(address sender, bytes[] calldata priceUpdateData) external payable nonReentrant {
59:         // Get fee amount to pay to Pyth
60:         uint256 fee = offchainOracle.oracleContract.getUpdateFee(priceUpdateData);
61: 
62:         // Update the price data (and pay the fee)
63:         offchainOracle.oracleContract.updatePriceFeeds{value: fee}(priceUpdateData);
64: 
65:         if (msg.value - fee > 0) {
66:             // Need to refund caller. Try to return unused value, or revert if failed
67:             (bool success, ) = sender.call{value: msg.value - fee}("");
68:             if (success == false) revert FlatcoinErrors.RefundFailed();
69:         }
70:     }

Impact

Medium as per the risk rating of the original report.

Code Snippet

https://github.com/sherlock-audit/2024-03-flat-money-fix-review-contest/blob/main/flatcoin-v1/src/abstracts/OracleModifiers.sol#L12

https://github.com/sherlock-audit/2024-03-flat-money-fix-review-contest/blob/main/flatcoin-v1/src/OracleModule.sol#L58

Tool used

Manual Review

Recommendation

To remediate the issue, one or more of the following possible solutions could be adopted to reduce the risk:

1) The attack mentioned in the original report requires executing an order with the first price and another order with the second price, ideally all within the same block. Otherwise, the chance of a profitable attack will be low. Consider allowing each account to execute only one order within a single block. 2) If the price has been updated at the start of a block, store this price and use this price consistently throughout all orders executed within the same block. If there is a subsequent price update within the same block, they will be ignored. The protocol must ensure that the "freshness" of the price is acceptable.

One might think that the current locking mechanism is similar to the above-mentioned first solution. However, if we look at the original report, one possible attack path is to use one leveraged order + one limit order. The issue is that the current locking mechanism only ensures that an account cannot have more than one announced leveraged order. Thus, an account is allowed to have one leveraged order + one limit order required to carry out the attacks mentioned in the report. The correct approach is to ensure that an account can only be one announced order, regardless of whether it is a leveraged or limit order at any point in time.

Duplicate of #27

sherlock-audit / 2024-03-flat-money-fix-review-contest-judging

xiaoming90 - The issue titled "Oracle can return different prices in same transaction" is not remediated. #18