code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/TradingExtension.sol#L88-L120

Vulnerability details

Impact

User can abuse how stop losses are priced to open high leverage trades with huge upside and very little downside

Proof of Concept

function limitClose(
    uint _id,
    bool _tp,
    PriceData calldata _priceData,
    bytes calldata _signature
)
    external
{
    _checkDelay(_id, false);
    (uint _limitPrice, address _tigAsset) = tradingExtension._limitClose(_id, _tp, _priceData, _signature);
    _closePosition(_id, DIVISION_CONSTANT, _limitPrice, address(0), _tigAsset, true);
}

function _limitClose(
    uint _id,
    bool _tp,
    PriceData calldata _priceData,
    bytes calldata _signature
) external view returns(uint _limitPrice, address _tigAsset) {
    _checkGas();

    IPosition.Trade memory _trade = position.trades(_id);
    _tigAsset = _trade.tigAsset;
    getVerifiedPrice(_trade.asset, _priceData, _signature, 0);
    uint256 _price = _priceData.price;
    if (_trade.orderType != 0) revert("4"); //IsLimit
    if (_tp) {
        if (_trade.tpPrice == 0) revert("7"); //LimitNotSet
        if (_trade.direction) {
            if (_trade.tpPrice > _price) revert("6"); //LimitNotMet
        } else {
            if (_trade.tpPrice < _price) revert("6"); //LimitNotMet
        }
        _limitPrice = _trade.tpPrice;
    } else {
        if (_trade.slPrice == 0) revert("7"); //LimitNotSet
        if (_trade.direction) {
            if (_trade.slPrice < _price) revert("6"); //LimitNotMet
        } else {
            if (_trade.slPrice > _price) revert("6"); //LimitNotMet
        }
        //@audit stop loss is closed at user specified price NOT market price
        _limitPrice = _trade.slPrice;
    }
}

When closing a position with a stop loss the user is closed at their SL price rather than the current price of the asset. A user could abuse this in directional markets with high leverage to make nearly risk free trades. A user could open a long with a stop loss that in $0.01 below the current price. If the price tanks immediately on the next update then they will be closed out at their entrance price, only out the fees to open and close their position. If the price goes up then they can make a large gain.

Tools Used

Manual Review

Recommended Mitigation Steps

Take profit and stop loss trades should be executed at the current price rather than the price specified by the user:

         if (_trade.tpPrice == 0) revert("7"); //LimitNotSet
        if (_trade.direction) {
            if (_trade.tpPrice > _price) revert("6"); //LimitNotMet
        } else {
            if (_trade.tpPrice < _price) revert("6"); //LimitNotMet
        }
-       _limitPrice = _trade.tpPrice;
+       _limitPrice = _price;
    } else {
        if (_trade.slPrice == 0) revert("7"); //LimitNotSet
        if (_trade.direction) {
            if (_trade.slPrice < _price) revert("6"); //LimitNotMet
        } else {
            if (_trade.slPrice > _price) revert("6"); //LimitNotMet
        }
-       _limitPrice = _trade.slPrice;
+       _limitPrice = _price;

GalloDaSballo commented 1 year ago

Effectively same as #515

But I think for now High Severity is more appropriate

c4-judge commented 1 year ago

GalloDaSballo marked the issue as primary issue

TriHaz commented 1 year ago

Because of open fees, close fees and spread, that wouldn't be profitable. We also have a cooldown after a trade is opened so there will be enough time for price to move freely past the sl.

c4-sponsor commented 1 year ago

TriHaz marked the issue as sponsor disputed

GalloDaSballo commented 1 year ago

The warden has shown a flaw in how the protocol offers Stop Losses.

By using the originally stored value for Stop Loss, instead of just using it as a trigger, an attacker can perform a highly profitable strategy on the system as they know that their max risk is capped by the value of the Stop Loss, instead of the current asset price.

This will happen at the detriment of LPs

Because the attack breaks an important invariant, causing a loss to other users, I agree with High Severity