The execution of limit order can be front-runned

Summary

The execution of a limit order by the keeper can be front-run by the leverage position's owner, allowing manipulation of the values of the limit order.

Vulnerability Detail

The LimitOrder implements the logic of announcing a limit order via the announceLimitOrder function, which later has to be executed through the executeLimitOrder function by the keeper. The issue is that the keeper's call to executeLimitOrder can be front-run by the leverage position's owner to change the priceLowerThreshold and priceUpperThreshold values of the limit order via announceLimitOrder.

Impact

The keeper will execute limit order with the priceLowerThreshold and priceUpperThreshold he did not accept effectively leading to the loss of funds by the keeper.

Code Snippet

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

Tool used

Manual Review

Recommendation

Add a mechanism similar to the slippage protection where executeLimitOrder function accepts parameters of priceLowerThreshold and priceUpperThreshold. In case these values do not match the limit order's values the call should revert.

sherlock-admin commented 8 months ago

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

ubl4nk commented:

invalid -> this has a low impact because this doesn't have any benefit for trader and the trader is just lossing the gas-fee.

takarez commented:

invalid

nevillehuang commented 8 months ago

Invalid, front-running is not possible on base, given they use a private mempool see discussions here and comments here. Additionally, agree with sponsors following comments:

The point of a limit order is to limit the loss incurred or take the profit as wished by the trader. If they try to frontrun this, it's their loss really.