Tigris Trade contest - bin2chen's results

Lines of code

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

Vulnerability details

Impact

user can have a profit greater than maxWinPercent

Proof of Concept

_closePosition() will limit the user's profit to no greater than maxWinPercent But there is a problem with the check here: When the user is not completely closed (_percent < DIVISION_CONSTANT,example:50%) The error is still compared to 100% of the maximum profit: _payout <= (_trade.marginmaxWinPercent/DIVISION_CONSTANT) It should be multiplied by the percentage of the maximum profit to be normal as: _payout <= _trade.margin_percent*maxWinPercent/DIVISION_CONSTANT/DIVISION_CONSTANT

example: margin = 100 maxWinPercent = 500% The current profit is 800 which is closed twice (50% each time):

First close 50 : payout = 800 * 50% = 400 (it's ok because 400 < 100 * 5) Second closing of the remaining 50: payout = 400 * 100% = 400, since 400 > 50 * 5 (current maxWinPercent), only 250 can be taken Finally the total profit of 400 + 250 = 650 is greater than maxWinPercent(100 * 5 = 500)

    function _closePosition(
        uint _id,
        uint _percent,
        uint _price,
        address _stableVault,
        address _outputToken,
        bool _isBot
    )
        internal
    {
...

        if (_payout > 0) {
            unchecked {
                _toMint = _handleCloseFees(_trade.asset, uint256(_payout)*_percent/DIVISION_CONSTANT, _trade.tigAsset, _positionSize*_percent/DIVISION_CONSTANT, _trade.trader, _isBot);
                if (maxWinPercent > 0 && _toMint > _trade.margin*maxWinPercent/DIVISION_CONSTANT) { //****** need * _percent***//
                    _toMint = _trade.margin*maxWinPercent/DIVISION_CONSTANT;  //****** need * _percent***//
                }
...

Tools Used

Recommended Mitigation Steps

    function _closePosition(
        uint _id,
        uint _percent,
        uint _price,
        address _stableVault,
        address _outputToken,
        bool _isBot
    )
        internal
    {
...

            unchecked {
                _toMint = _handleCloseFees(_trade.asset, uint256(_payout)*_percent/DIVISION_CONSTANT, _trade.tigAsset, _positionSize*_percent/DIVISION_CONSTANT, _trade.trader, _isBot);
-               if (maxWinPercent > 0 && _toMint > _trade.margin*maxWinPercent/DIVISION_CONSTANT) {
-                   _toMint = _trade.margin*maxWinPercent/DIVISION_CONSTANT;
+               if (maxWinPercent > 0 && _toMint > _trade.margin*_percent*maxWinPercent/DIVISION_CONSTANT/DIVISION_CONSTANT) {
+                   _toMint = _trade.margin**_percent*maxWinPercent/DIVISION_CONSTANT/DIVISION_CONSTANT;
                }                
            }

Lines of code

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

Vulnerability details

Impact

addToPosition() user no pay the fees

Proof of Concept

In addToPosition(), call _handleDeposit() passes "_addMargin - _fee", which should be the full "_addMargin", not minus fee.

This way the user pays less

    function addToPosition(
        uint _id,
        uint _addMargin,
        PriceData calldata _priceData,
        bytes calldata _signature,
        address _stableVault,
        address _marginAsset,
        ERC20PermitData calldata _permitData,
        address _trader
    )
        external
    {
...
        _handleDeposit(
            _trade.tigAsset,
            _marginAsset,
            _addMargin - _fee, //****@audit pass "_addMargin - _fee" , should just pass _addMargin
            _stableVault,
            _permitData,
            _trader
        );

Tools Used

Recommended Mitigation Steps

    function addToPosition(
        uint _id,
        uint _addMargin,
        PriceData calldata _priceData,
        bytes calldata _signature,
        address _stableVault,
        address _marginAsset,
        ERC20PermitData calldata _permitData,
        address _trader
    )
        external
    {
...
        _handleDeposit(
            _trade.tigAsset,
            _marginAsset,
-           _addMargin - _fee,
+           _addMargin,
            _stableVault,
            _permitData,
            _trader
        );

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

Lines of code

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

Vulnerability details

Impact

_handleOpenFees() may not work

Proof of Concept

_handleOpenFees() will transfer the fees to the gov contract at the end, but calling gov.distribute() without token.approve(gov) first it will can't transfer the token

note: _handleCloseFees() has approve(gov,max) first, but in general _handleOpenFees will be called before _handleCloseFees() so it will be not work

    function _handleOpenFees(
        uint _asset,
        uint _positionSize,
        address _trader,
        address _tigAsset,
        bool _isBot
    )
...

               _positionSize * _fees.burnFees / DIVISION_CONSTANT,
                _referrer != address(0) ? _positionSize * _fees.referralFees / DIVISION_CONSTANT : 0,
                _positionSize * _fees.botFees / DIVISION_CONSTANT,
                _referrer
            );
            IStable(_tigAsset).mintFor(address(this), _daoFeesPaid);
        }
        //****@audit without _tigAsset.approve() first ***//
        gov.distribute(_tigAsset, IStable(_tigAsset).balanceOf(address(this)));
    }    

Tools Used

Recommended Mitigation Steps

    function _handleOpenFees(
        uint _asset,
        uint _positionSize,
        address _trader,
        address _tigAsset,
        bool _isBot
    )
...

               _positionSize * _fees.burnFees / DIVISION_CONSTANT,
                _referrer != address(0) ? _positionSize * _fees.referralFees / DIVISION_CONSTANT : 0,
                _positionSize * _fees.botFees / DIVISION_CONSTANT,
                _referrer
            );
            IStable(_tigAsset).mintFor(address(this), _daoFeesPaid);
        }
+       IStable(_tigAsset).approve(address(gov), type(uint).max);       
        gov.distribute(_tigAsset, IStable(_tigAsset).balanceOf(address(this)));
    }    

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/utils/TradingLibrary.sol#L113

Vulnerability details

Impact

verifyPrice() don't work

Proof of Concept

According to Chainlink’s documentation (Deprecated API Reference, Migration Instructions, and API Reference), the latestAnswer function is deprecated. This function does not throw an error if no answer has been reached, but instead returns 0, causing an incorrect price

https://docs.chain.link/data-feeds/price-feeds/api-reference#latestanswer

Tools Used

Recommended Mitigation Steps

Recommend using the latestRoundData function to get the price instead. Also recommend adding checks on the return data with proper revert messages if the price is stale or the round is incomplete, for example:

(uint80 roundID, int256 price, , uint256 timeStamp, uint80 answeredInRound) = IPrice(_chainlinkFeed).latestRoundData();
require(answeredInRound >= roundID, "...");
require(timeStamp != 0, "...");