search

sherlock-audit / 2023-12-flatmoney-judging

11 stars 9 forks source link

the-first-elder - Users can continually update their orders to get a free look into prices in future blocks #84

Closed sherlock-admin2 closed 8 months ago

sherlock-admin2 commented 8 months ago

the-first-elder

high

Users can continually update their orders to get a free look into prices in future blocks

Summary

Users can continually update their orders to get a free look into prices in future blocks

Vulnerability Detail

Order execution relies on signed archived prices from off-chain oracles, where each price is stored along with the block range it applies to, and limit orders are only allowed to execute with oracle prices where the block is greater than the block in which the order was last updated. Since prices are required to be future prices, there is a time gap between when the last signed price was archived, and the new price for the next block is stored in the archive, and the order keeper is able to fetch it and submit an execution for it in the next block.

Impact

If a user has a pending exit order that was submitted a block N, and the user sees that the price at block N+1 will be more favorable, they can update their exit order, changing the amount by +/- 1 wei, and have the order execution delayed until the next block, at which point they can decided again whether the price and or impact is favorable, and whether to exit.

An example to this review can be found here

Code Snippet

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/LimitOrder.sol#L58

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/LimitOrder.sol#L119

Tool used

Manual Review

Recommendation

Require a delay between when the limit order was last announced and when it was executed

sherlock-admin commented 8 months ago

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

takarez commented:

invalid

nevillehuang commented 8 months ago

Invalid, seems like a front-running attack vector is required to see future prices, and front-running is a non-issue in base chain based on comments in #49. Additionally invalid based on sponsor comments:

"There's a delay between the price of limit order announce and execution https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/LimitOrder.sol#L146"