Tigris Trade contest - noot's results

Lines of code

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

Vulnerability details

Impact

In Trading.sol limitClose, the user feeds in a boolean _tp value which is used in tradingExtension._limitClose to return the _limitPrice, which is then used to calculate _payout inside _closePosition. Since the _limitPrice returned is the trade's _trade.tpPrice or _trade.slPrice, depending what the boolean value is, and these values are also set by the user when the position is created, the user can easily manipulate what the payout is. This can result in the vault being drained or an arbitrary amount of tokens being minted.

Proof of Concept

    function limitClose(
        uint _id,
        bool _tp,
        PriceData calldata _priceData,
        bytes calldata _signature
    )
        external
    {
        _checkDelay(_id, false);
        //@audit _tp can be fed in, _limitPrice is returned based on it and what the trade's TP and SL are
        // which can also be set by us in initiateMarketPosition.
        // _limitPrice is used as _price in _closePosition, which is used to calculate (proportionally) the payout
        // returned from _closePosition, thus you can manipulate the payout really easily,
        // since _limitPrice is set to trade.tpPrice which can also be arbitrarity set in initiateMarketPosition
        (uint _limitPrice, address _tigAsset) = tradingExtension._limitClose(_id, _tp, _priceData, _signature);
        _closePosition(_id, DIVISION_CONSTANT, _limitPrice, address(0), _tigAsset, true);
    }

TradingExtension._limitClose essentially uses _tp as a flag. The checks can easily be bypassed, as the user sets tpPrice and slPrice when opening the position.

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

Finally, in Trading._closePosition, the _payout is calculated:

    function _closePosition(
        uint _id,
        uint _percent,
        uint _price,
        address _stableVault,
        address _outputToken,
        bool _isBot
    )
        internal
    {
        (IPosition.Trade memory _trade, uint256 _positionSize, int256 _payout) = tradingExtension._closePosition(_id, _price, _percent);
        // code omitted

The payout is calculated using:

        // _price here is _limitPrice passed in from limitClose
        (_positionSize, _payout) = TradingLibrary.pnl(_trade.direction, _price, _trade.price, _trade.margin, _trade.leverage, _trade.accInterest);

This function returns a value proportional to _price passed in, thus can easily be manipulated by a user setting the trade's tpPrice.

Steps:

1. An attacker calls initiateMarketOrder, passing in some arbitrarily high _tradeInfo.tpPrice. Note that this is not validated anywhere inside initiateMarketOrder. This value should pass the checks in TradingExtension._limitClose which can be done by setting the trade direction as desired.
2. The attacker then calls limitClose, which calls _closePosition, passing tpPrice as the _limitPrice. _closePosition then sends the attacker the payout which is proportional to this value. This can be used to drain the vault or mint unlimited funds.

Tools Used

n/a

Recommended Mitigation Steps

initiateMarketOrder and initiateLimitOrder should validate the _tradeInfo.tpPrice values passed in. This check should also be added to updateTpSl. However, even with validation, the payout is still somewhat manipulable, but setting a limit on it will reduce the amount of manipulation possible. Additionally, limitClose should calculate whether profit can be taken instead of allowing the user to pass it in.

Overall, the usage of tpPrice and slPrice should be re-assessed. initiateCloseOrder, which does not use these values, is not subject to the same vulnerability. These values should be set by the contract instead of by the user, or much more heavily validated, potentially based on asset and other factors.

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/TradingExtension.sol#L163-L188 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/utils/TradingLibrary.sol#L91-L122

Vulnerability details

Impact

The price and signature from an oracle is valid within a certain time period, determined by block timestamps. However, if multiple prices and signatures are released within this time period, the user can pick and choose which one to use, thus manipulating the price of positions.

Proof of Concept

TradingLibrary.verifyPrice verifies the signature of the price:

    function verifyPrice(
        uint256 _validSignatureTimer,
        uint256 _asset,
        bool _chainlinkEnabled,
        address _chainlinkFeed,
        PriceData calldata _priceData,
        bytes calldata _signature,
        mapping(address => bool) storage _isNode
    )
        external view
    {
        address _provider = (
            keccak256(abi.encode(_priceData))
        ).toEthSignedMessageHash().recover(_signature);
        require(_provider == _priceData.provider, "BadSig");
        require(_isNode[_provider], "!Node");
        require(_asset == _priceData.asset, "!Asset");
        require(!_priceData.isClosed, "Closed");
        require(block.timestamp >= _priceData.timestamp, "FutSig");
        require(block.timestamp <= _priceData.timestamp + _validSignatureTimer, "ExpSig");
        require(_priceData.price > 0, "NoPrice");
        if (_chainlinkEnabled && _chainlinkFeed != address(0)) {
            int256 assetChainlinkPriceInt = IPrice(_chainlinkFeed).latestAnswer();
            if (assetChainlinkPriceInt != 0) {
                uint256 assetChainlinkPrice = uint256(assetChainlinkPriceInt) * 10**(18 - IPrice(_chainlinkFeed).decimals());
                require(
                    _priceData.price < assetChainlinkPrice+assetChainlinkPrice*2/100 &&
                    _priceData.price > assetChainlinkPrice-assetChainlinkPrice*2/100, "!chainlinkPrice"
                );
            }
        }
    }

However, the signature is valid for _validSignatureTimer amount of time. Thus, if multiple signatures are released during a time period less than _validSignatureTimer, either of them are valid. The validSignatureTimer value is set by the owner in TradingExtension.sol. If this is too long, users are able to pick and choose what price to use.

Tools Used

n/a

Recommended Mitigation Steps

Carefully set validSignatureTimer and ensure it's the exact duration between prices/signatures being released from the oracle.