code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

In the function _limitClose from the TradingExtension contract the _priceData.price is not verified with the getVerifiedPrice function instead its value is directly used, and because the the getVerifiedPrice internally calls the function TradingLibrary.verifyPrice which is responsible of verifying that the price is signed by a whitelisted node, the _limitClose function can potentiely use a price that is not signed by a whitelisted node.

This will cause problems in the limitClose function from the Trade contract which calls _limitClose, and it will affect the closing of the position.

Proof of Concept

The issue occurs in the _limitClose function :

File: contracts/TradingExtension.sol Line 88-120

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;

    /* @audit
       _price is not verified and is used directely
    */
    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
        }
        _limitPrice = _trade.slPrice;
    }
}

As you can see from the code above the getVerifiedPrice function is called but its return value is not used instead the price is used directly from _priceData.price.

The _limitClose function will be called by limitClose function from the Trading contract when trying to close a position at pre-set price :

File: contracts/Trading.sol Line 565-576

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);
}

Because the returned price _limitPrice from _limitClose is calculated using non verified price it can cause issues when the _closePosition function is called and user might lose funds.

Tools Used

Manual review

Recommended Mitigation Steps

To avoid this issue the function _limitClose must use the verified price to calculate the limitPrice value, the _limitClose function should be modified as follow :

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;

    /* @audit
       verify the price and use the returned  verified _price
    */
    (uint256 _price,) = getVerifiedPrice(_trade.asset, _priceData, _signature, 0);

    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
        }
        _limitPrice = _trade.slPrice;
    }
}

c4-judge commented 1 year ago

GalloDaSballo marked the issue as primary issue

TriHaz commented 1 year ago

getVerifiedPrice() would revert if price is invalid.

c4-sponsor commented 1 year ago

TriHaz marked the issue as sponsor disputed

GalloDaSballo commented 1 year ago

Agree with th esponsor https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/utils/TradingLibrary.sol#L106-L107

        require(_isNode[_provider], "!Node");

GalloDaSballo commented 1 year ago

Logically equivalent to just using the priceData.price after validation, no risk was shown https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/TradingExtension.sol#L181-L182

        _price = _priceData.price;

Could downgrade to QA, but am closing as invalid