sherlock-audit / 2023-12-flatmoney-judging

9 stars 8 forks source link

dimulski - Users that create a limit order can get their long positions closed with a price less than the stop-loss price they specified. #223

Closed sherlock-admin closed 5 months ago

sherlock-admin commented 5 months ago

dimulski

medium

Users that create a limit order can get their long positions closed with a price less than the stop-loss price they specified.

Summary

The FlatMoney protocol allows users to create limit orders where they can specify a stop-loss price as well as profit-take price by calling the announceLimitOrder() function. The problem arises in the _closePosition() function

if (price <= _limitOrder.priceLowerThreshold) {
            minFillPrice = 0; // can execute below lower limit price threshold
}

If the current price is below the priceLowerThreshold specified by the user when he was creating the limit order the new minFillPrice will be set to 0. When executeClose() function is then called we have the following check

(uint256 exitPrice, ) = IOracleModule(vault.moduleAddress(FlatcoinModuleKeys._ORACLE_MODULE_KEY)).getPrice({
            maxAge: maxAge
        });
        if (exitPrice < announcedClose.minFillPrice)
            revert FlatcoinErrors.HighSlippage(exitPrice, announcedClose.minFillPrice);

The above check will always go trough as the exitPrice can't be less than 0 (the function call will revert otherwise), which means that the limit order can be closed with a price that is below the priceLowerThreshold specified by the user. Thus the user will receive less rETH tokens back. The executeLimitOrder() can be called by anyone, once the min amount of time has passed.

Vulnerability Detail

Summary

Impact

User's long positions can be closed with a price less than the stop-loss price they specified when they created a limit order.

Code Snippet

LimitOrder.sol#L134-L177

Tool used

Manual Review

Recommendation

Consider rewriting this

if (price <= _limitOrder.priceLowerThreshold) {
            minFillPrice = 0; // can execute below lower limit price threshold
        }

so it adheres to the lowerThreshold the user has specified when creating the limit order

sherlock-admin commented 5 months ago

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

takarez commented:

invalid

nevillehuang commented 5 months ago

Invalid, this is how stop loss orders work, if price reaches lower than price threshold indicated, they should take any minimum price to limit losses from risk of price going even further down.