executeLimitOrder() lack check slPrice

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Trading.sol#L480

Vulnerability details

Impact

in executeLimitOrder() :

The final price of LimtOrder is uncertain, although it is limited to a certain range (limitOrderPriceRange)
but the final price is not guaranteed to meet the Stop loss price so it is still necessary to check

note:addToPosition() maybe has same problem, it will change price , Checking the slPrice again also feels reasonable

Proof of Concept

    function executeLimitOrder(
        uint _id, 
        PriceData calldata _priceData,
        bytes calldata _signature
    ) 
        external
    {
        unchecked {
            _checkDelay(_id, true);
            tradingExtension._checkGas();
            if (tradingExtension.paused()) revert TradingPaused();
            require(block.timestamp >= limitDelay[_id]);
            IPosition.Trade memory trade = position.trades(_id);
            uint _fee = _handleOpenFees(trade.asset, trade.margin*trade.leverage/1e18, trade.trader, trade.tigAsset, true);
            (uint256 _price, uint256 _spread) = tradingExtension.getVerifiedPrice(trade.asset, _priceData, _signature, 0);
            if (trade.orderType == 0) revert("5");
            if (_price > trade.price+trade.price*limitOrderPriceRange/DIVISION_CONSTANT || _price < trade.price-trade.price*limitOrderPriceRange/DIVISION_CONSTANT) revert("6"); //LimitNotMet
            if (trade.direction && trade.orderType == 1) {
                if (trade.price < _price) revert("6"); //LimitNotMet
            } else if (!trade.direction && trade.orderType == 1) {
                if (trade.price > _price) revert("6"); //LimitNotMet
            } else if (!trade.direction && trade.orderType == 2) {
                if (trade.price < _price) revert("6"); //LimitNotMet
                trade.price = _price;
            } else {
                if (trade.price > _price) revert("6"); //LimitNotMet
                trade.price = _price;
            } 
            if(trade.direction) {
                trade.price += trade.price * _spread / DIVISION_CONSTANT;
            } else {
                trade.price -= trade.price * _spread / DIVISION_CONSTANT;
            }
           /******@audit without _checkSl() ****//
            if (trade.direction) {
                tradingExtension.modifyLongOi(trade.asset, trade.tigAsset, true, trade.margin*trade.leverage/1e18);
            } else {
                tradingExtension.modifyShortOi(trade.asset, trade.tigAsset, true, trade.margin*trade.leverage/1e18);
            }
            _updateFunding(trade.asset, trade.tigAsset);
            position.executeLimitOrder(_id, trade.price, trade.margin - _fee);
            emit LimitOrderExecuted(trade.asset, trade.direction, trade.price, trade.leverage, trade.margin - _fee, _id, trade.trader, _msgSender());
        }
    }

Tools Used

Recommended Mitigation Steps

add check

    function executeLimitOrder(
        uint _id, 
        PriceData calldata _priceData,
        bytes calldata _signature
    ) 
        external
    {
....
            _updateFunding(trade.asset, trade.tigAsset);
+           _checkSl(_tradeInfo.slPrice, _tradeInfo.direction, _price);
            position.executeLimitOrder(_id, trade.price, trade.margin - _fee);
            emit LimitOrderExecuted(trade.asset, trade.direction, trade.price, trade.leverage, trade.margin - _fee, _id, trade.trader, _msgSender());
        }
    }

[GalloDaSballo]

Lacking in impact but seems to mostly be valid

[GalloDaSballo]

After re-reading, ultimately the lack of check is self-evident

That said I think the Warden may have missed a bigger exploit as they didn't go through writing a poc.

But I must agree that the lack of check is worth flagging by itself

[c4-judge]

GalloDaSballo marked the issue as duplicate of #512

[c4-judge]

GalloDaSballo marked the issue as satisfactory