Solution is susceptible to MEV, harming users.

[code423n4]

Handle

evertkors

Vulnerability details

Impact

Miners can hold back transactions and creating beneficial situation for themselves (based on state of solution).

There is not deadline on executing executeOutstandingNextPriceSettlementsUser(), which means the transaction can be delayed until MEV can be extracted.

Proof of Concept

Don't know the exact math of the solution but a miner can hoard one or more transactions calling executeOutstandingNextPriceSettlementsUser() (or similar function). And potentially include a transaction from the MEV marketplace that makes a lot of profit on behalves of the users trying to execute this transaction.

Recommended Mitigation Steps

I recommend to include a deadline in the executeOutstandingNextPriceSettlementsUser transaction. (just like Uniswap does)

e.g.

executeOutstandingNextPriceSettlementsUser(address user, uint32 marketIndex, uint256 deadline) external override {
    require(deadline <= block.number);
    ....
}

This mitigates the issue by limiting the amount of transactions the miner is able to hoard.

[JasoonS]

Can you put numbers to an attack. I don't see how this attack can be carried out. Delaying a executeOutstandingNextPriceSettlementsUser doesn't change its outcome (nor the outcome of any other users transactions). The exact result of that transaction and all other transactions after updateSystemState is fixed and isn't affected by transaction ordering.

[moose-code]

Correct as Jason states. We use price snapshots indexed according to the exact cycle which with the user created the action. executeOutstandingNextPriceSettlementsUser merely executes these deterministically (since the past price snapshot etc is fixed), regardless of what later tx order, or how long after it is called.

[0xean]

After reviewing this one and think through the implications, a miner would have to hold back a call that _updateSystemStateInternal but by doing so, they would delay all transactions, including any that would potentially be profitable from a front running perspective. I do not see the attack vector here, but I do believe that the require statement could be a beneficial feature to users who would like to only execute their transaction prior to a certain block.

Essentially instead of having their order be a "GTC" type order, they now have an order with an expiration built in, like a "GTD".

Will mark as a 0 - non critical