nobody2018 - In executeOrder, OracleModule.getPrice(maxAge) may revert because maxAge is too small

[sherlock-admin2]

nobody2018

medium

In executeOrder, OracleModule.getPrice(maxAge) may revert because maxAge is too small

Summary

[executeOrder](https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/DelayedOrder.sol#L378-L381) will call different functions to process the order according to the type of order, and these functions will call OracleModule.getPrice(maxAge) to get the price. maxAge is equal to the current block.timestamp - order.executableAtTime. If maxAge is too small (for example, 0-3), then OracleModule.getPrice(maxAge) may revert [here](https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/OracleModule.sol#L132) even though the price is fresh.

Vulnerability Detail

Below we use the LeverageAdjust type Order to describe this issue.

minExecutabilityAge is 5s  
maxExecutabilityAge is 60s

1. Alice deposits margin on her position A via DelayedOrder.announceLeverageAdjust. Because if the price of the collateral continues to dump, position A will be liquidated. A pending LeverageAdjust order is created. Assume block.timestamp = 1707000000, so order.executableAtTime = block.timestamp + minExecutabilityAge = 1707000005. The expiration time of this order is order.executableAtTime + maxExecutabilityAge = 1707000065.
2. The order has keeperFee paid to the keeper, so the keepers will compete with each other as long as there is an order that can be executed. One keeper monitored the FlatcoinEvents.OrderAnnounced event, it requested the API to obtain priceUpdateData. Assume that the fresh price's publishTime is 1707000003.
3. After minExecutabilityAge seconds (block.timestamp=1707000005), the keeper executes the order via DelayedOrder.executeOrder. The priceUpdateData argument is obtained in step 2. Eventually tx will revert.

Let’s analyze the reasons for revert. The call stack of [DelayedOrder.executeOrder](https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/DelayedOrder.sol#L378-L381) is as follows:

DelayedOrder.executeOrder
  //This modifer will write priceUpdatedata to pyth oracle. Note: priceUpdatedata is from step 2
  updatePythPrice(vault, msg.sender, priceUpdateData)
    vault.settleFundingFees()
    _executeLeverageAdjust(account)
      //here checking whether order can be executed
      _prepareExecutionOrder(account, order.executableAtTime)
      LeverageModule.executeAdjust
L152    uint32 maxAge = _getMaxAge(_order.executableAtTime);
L159    (uint256 adjustPrice, ) = IOracleModule(vault.moduleAddress(FlatcoinModuleKeys._ORACLE_MODULE_KEY)).getPrice({
            maxAge: maxAge
        });
        ......
      ......
    ......
  ......

L152, maxAge = block.timestamp - _order.executableAtTime = 1707000005 - 1707000005 = 0

L159, the maxAge argument passed into OracleModule.getPrice is 0.

File: flatcoin-v1\src\OracleModule.sol
094:     function getPrice(uint32 maxAge) public view returns (uint256 price, uint256 timestamp) {
095:->       (price, timestamp) = _getPrice(maxAge);
096:     }

106:     function _getPrice(uint32 maxAge) internal view returns (uint256 price, uint256 timestamp) {
107:->       (uint256 onchainPrice, uint256 onchainTime) = _getOnchainPrice(); // will revert if invalid
108:->       (uint256 offchainPrice, uint256 offchainTime, bool offchainInvalid) = _getOffchainPrice();
109:         bool offchain;
110: 
111:         uint256 priceDiff = (int256(onchainPrice) - int256(offchainPrice)).abs();
112:         uint256 diffPercent = (priceDiff * 1e18) / onchainPrice;
113:         if (diffPercent > maxDiffPercent) revert FlatcoinErrors.PriceMismatch(diffPercent);
114: 
115:         if (offchainInvalid == false) {
116:             // return the freshest price
117:             if (offchainTime >= onchainTime) {
118:                 price = offchainPrice;
119:                 timestamp = offchainTime;
120:                 offchain = true;
121:             } else {
122:                 price = onchainPrice;
123:                 timestamp = onchainTime;
124:             }
125:         } else {
126:             price = onchainPrice;
127:             timestamp = onchainTime;
128:         }
129: 
130:         // Check that the timestamp is within the required age
131:->       if (maxAge < type(uint32).max && timestamp + maxAge < block.timestamp) {
132:             revert FlatcoinErrors.PriceStale(
133:                 offchain ? FlatcoinErrors.PriceSource.OffChain : FlatcoinErrors.PriceSource.OnChain
134:             );
135:         }
136:     }

L107, onchainTime = updatedAt returned by oracle.latestRoundData() from chainlink. updatedAt depends on the symbol's heartBeat. The heartbeats of almost chainlink price feeds are based on hours (such as 24 hours, 1 hour, etc.). Therefore, onchainTime is always lagging time, and the probability of being equal to block.timestamp is low. 

L108, offchainTime = publishTime of priceUpdateData = 1707000003

L117-124, timestamp = max(onchainTime, offchainTime), in most cases offchainTime is larger.

L131, in this case maxAge=0,

maxAge < type(uint32).max && timestamp + maxAge < block.timestamp =>
0 < type(uint32).max && 1707000003 + 0 < 1707000005 =>
0 < type(uint32).max && 1707000003 < 1707000005

So if statement is met, tx revert.

Impact

• Pending orders cannot be executed in time. For time-sensitive protocol, this is unacceptable.
• The keeper obviously submitted the fresh price, but the tx failed with FlatcoinErrors.PriceStale.

Code Snippet

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/LeverageModule.sol#L94-L96

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/LeverageModule.sol#L159-L161

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/LeverageModule.sol#L270-L272

Tool used

Manual Review

Recommendation

The cases in the report need to be considered.

File: flatcoin-v1\src\LeverageModule.sol
449:     function _getMaxAge(uint64 _executableAtTime) internal view returns (uint32 _maxAge) {
450:-        return (block.timestamp - _executableAtTime).toUint32();
450:+        return (block.timestamp - _executableAtTime + vault.minExecutabilityAge()).toUint32();
451:     }

[nevillehuang]

Request PoC to facilitate discussion between sponsor and watson

Sponsor comments:

From practice, it's not true. Pyth Network price feeds update once per second

[sherlock-admin2]

PoC requested from @securitygrid

Requests remaining: 11

[sherlock-admin]

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

takarez commented:

valid: this semm valid to me; medium(10)

[securitygrid]

copy the following POC to test/unit/Common/CancelOrder.t.sol:

function test_170() public {
        setWethPrice(2000e8);
        skip(120);

        // First deposit mint doesn't use offchain oracle price
        announceAndExecuteDeposit({
            traderAccount: alice,
            keeperAccount: keeper,
            depositAmount: 100e18,
            oraclePrice: 2000e8,
            keeperFeeAmount: 0
        });

        announceOpenLeverage({traderAccount: alice, margin: 100e18, additionalSize: 100e18, keeperFeeAmount: 0});
        //keeper got offchain price before order.executableAtTime
        skip(vaultProxy.minExecutabilityAge() - 1);
        bytes[] memory priceUpdateData = getPriceUpdateData(2000e8);
        //In order to compete for tradeFee, the keeper must execute orders as quickly as possible.
        skip(vaultProxy.minExecutabilityAge());
        vm.prank(keeper);
        delayedOrderProxy.executeOrder{value: 1}(alice, priceUpdateData);
    }
    function test_170_for_sponor() public {
        //From sponor's comment: From practice, it's not true. Pyth Network price feeds update once per second
        setWethPrice(2000e8);
        skip(120);

        // First deposit mint doesn't use offchain oracle price
        announceAndExecuteDeposit({
            traderAccount: alice,
            keeperAccount: keeper,
            depositAmount: 100e18,
            oraclePrice: 2000e8,
            keeperFeeAmount: 0
        });

        announceOpenLeverage({traderAccount: alice, margin: 100e18, additionalSize: 100e18, keeperFeeAmount: 0});
        //keeper got offchain price before order.executableAtTime
        skip(vaultProxy.minExecutabilityAge() - 1);
        bytes[] memory priceUpdateDataOld = getPriceUpdateData(2000e8);
        //In order to compete for tradeFee, the keeper must execute orders as quickly as possible.
        skip(vaultProxy.minExecutabilityAge());
        //In same block, other keeper or other project updates price. Therefore, such a situation is ok.
        bytes[] memory priceUpdateDataNew = getPriceUpdateData(2000e8);
        mockPyth.updatePriceFeeds{value: 1}(priceUpdateDataNew);
        //keeper executes order.
        vm.prank(keeper);
        delayedOrderProxy.executeOrder{value: 1}(alice, priceUpdateDataOld);
    }
/**output
Running 2 tests for test/unit/Common/CancelOrder.t.sol:CancelDepositTest
[FAIL. Reason: PriceStale(1)] test_170() (gas: 1329703)
[PASS] test_170_for_sponor() (gas: 1347073)
Test result: FAILED. 1 passed; 1 failed; 0 skipped; finished in 29.11ms
**/

[D-Ig]

hey, as far as I understood this whole report is based on the scenario that keeper monitors OrderAnnounced events and then immediately after queries pyth API to get the price update message and then waits minExecutabilityAge before execution.

this is the wrong flow, as our keeper monitors events and does not query anything before minExecutabilityAge. pyth API is fetched after minExecutabilityAge and then call to execute order is made.

[securitygrid]

@D-Ig

A pending order can be executed at [t0,t1]. The average block time of base is 2 seconds. A well-developed keeper can make tx to be mined at t0. The purpose is to get keeperFee(First come, first served). Therefore, the price must be obtained before t0.

this is the wrong flow, as our keeper monitors events and does not query anything before minExecutabilityAge. pyth API is fetched after minExecutabilityAge and then call to execute order is made.

If the keeper can only come from the protocol, then I will not submit this report. The protocol hopes that that the more keepers the better. Please reconsider this issue, thank you.

[nevillehuang]

Hi @rashtrakoff @D-Ig any further comments on the above highlighted comment?

[D-Ig]

Hi @rashtrakoff @D-Ig any further comments on the above highlighted comment?

I don't know what else to add. for me it's a chicken and egg problem

[rashtrakoff]

Let's take a look at the statement which might create an issue:

`timestamp + maxAge < block.timestamp`

Since maxAge = block.timestamp - order.executableAtTime

This can also be re-written as:

`timestamp < order.executableAtTime`

Basically what I mean is that the price fetched from pyth API should be newer than executable at time. I believe this is what we intended anyway so I wouldn't consider this as an issue.

[securitygrid]

A pending order can be executed at [t0,t1].

The problem described in this report is that the order can never be executed at t0. Because it takes time for a tx to be created, sent to the node and finally mined. The price timestamp is obtained when tx is created. Therefore it is smaller than t0.

[0xLogos]

Escalate

Invalid

Keepers from protocol team will work.

After minExecutabilityAge seconds, the keeper executes the order via DelayedOrder.executeOrder. The priceUpdateData argument is obtained in step 2. Eventually tx will revert.

The fact that other keepers won't work is their desing problem.

[sherlock-admin2]

Escalate

Invalid

Keepers from protocol team will work.

After minExecutabilityAge seconds, the keeper executes the order via DelayedOrder.executeOrder. The priceUpdateData argument is obtained in step 2. Eventually tx will revert.

The fact that other keepers won't work is their desing problem.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[securitygrid]

Disagree with this escalation. The purpose of the protocol's own keeper is only to ensure that the system can operate. The purpose of the third-party keepers is only to make a profit, that is, to compete for keeperFee (first come, first served). I have fully described my views in the report/coded POC/previous comments. In the current implementation, an order cannot be executed at t0, even though the keeper takes a fresh price. No more comments, leave it to Sherlock to judge, thank you

[Evert0x]

I think this reports highlights a potential mismatch in incentives for off chain components. Seems like the protocol can function in a healthy way without implementing the proposed change. That makes it a design choice.

Planning to accept escalation and invalidate.

[nevillehuang]

Fair enough @Evert0x can be low severity given sponsor comments here as well

[Czar102]

Result: Low Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• 0xLogos: accepted