Tigris Trade contest - hansfriese's results

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/BondNFT.sol#L178 https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Lock.sol#L38

Vulnerability details

Impact

LP Rewards can be increased infinitely by a malicious liquidity provider

Proof of Concept

A Bond NFT holder can claim pending rewards from a bond using the function Lock.claim().

function claim(
    uint256 _id
) public returns (address) {
    claimGovFees();
    (uint _amount, address _tigAsset) = bondNFT.claim(_id, msg.sender);
    IERC20(_tigAsset).transfer(msg.sender, _amount);
    return _tigAsset;
}

This claim() function calls the BondNFT.claim() and if the bond is an expired one there is an additional process to reimburse the rewards for that expired bond.

So the protocol calculates how much rewards were allocated to that expired bond from the bond.expireEpoch to epoch[bond.asset] and reimburse that to the current accRewardsPerShare.

The problem is it calculates the _pendingDelta using the bond.expireEpoch and epoch[bond.asset] while this can be called multiple times.

    function claim(
        uint _id,
        address _claimer
    ) public onlyManager() returns(uint amount, address tigAsset) {
        Bond memory bond = idToBond(_id);
        require(_claimer == bond.owner, "!owner");
        amount = bond.pending;
        tigAsset = bond.asset;
        unchecked {
            if (bond.expired) {
                uint _pendingDelta = (bond.shares * accRewardsPerShare[bond.asset][epoch[bond.asset]] / 1e18 - bondPaid[_id][bond.asset]) - (bond.shares * accRewardsPerShare[bond.asset][bond.expireEpoch-1] / 1e18 - bondPaid[_id][bond.asset]);
                if (totalShares[bond.asset] > 0) {
                    accRewardsPerShare[bond.asset][epoch[bond.asset]] += _pendingDelta*1e18/totalShares[bond.asset];
                }
            }
            bondPaid[_id][bond.asset] += amount;
        }
        IERC20(tigAsset).transfer(manager, amount);
        emit ClaimFees(tigAsset, amount, _claimer, _id);
    }

As we can see, the claim() function does not do anything to release the expired bond or remember the time that this reimbursement happens. So an expired bond holder can trigger this function by calling Lock.claim() repeatedly to increase the accRewardsPerShare. This means the rewards for liquidity providers can be increased to infinite.

Tools Used

Manual Review

Recommended Mitigation Steps

Release the bond if expired in the function BondNFT.claim() or remember the last time that the _pendingDelta was calculated to make sure the reimbursement does not happen more than once.

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/b2ebb8ea1def4927a747e7a185174892506540ab/contracts/Trading.sol#L625-L627

Vulnerability details

Impact

Users can bypass the maxWinPercent limit using a partial closing.

As a result, users can receive more funds than their upper limit from the protocol.

Proof of Concept

As we can see from the documentation, there is limitation of a maximum PnL.

Maximum PnL is +500%. The trade won't be closed unless the user sets a Take Profit order or closes the position manually.

And this logic was implemented like below in _closePosition().

File: 2022-12-tigris\contracts\Trading.sol
624:                 _toMint = _handleCloseFees(_trade.asset, uint256(_payout)*_percent/DIVISION_CONSTANT, _trade.tigAsset, _positionSize*_percent/DIVISION_CONSTANT, _trade.trader, _isBot);
625:                 if (maxWinPercent > 0 && _toMint > _trade.margin*maxWinPercent/DIVISION_CONSTANT) { //@audit bypass limit
626:                     _toMint = _trade.margin*maxWinPercent/DIVISION_CONSTANT;
627:                 }

But it checks the maxWinPercent between the partial payout and full margin so the below scenario is possible.

1. Alice opened an order of margin = 100 and PnL = 1000 after taking closing fees.
2. If maxWinPercent = 500%, Alice should receive 500 at most.
3. But Alice closed 50% of the position and she got 500 for a 50% margin because it checks maxWinPercent with _toMint = 500 and _trade.margin = 100
4. After she closed 50% of the position, the remaining margin = 50 and PnL = 500 so she can continue step 3 again and again.
5. As a result, she can withdraw almost 100% of the initial PnL(1000) even though she should receive at most 500.

Tools Used

Manual Review

Recommended Mitigation Steps

We should check the maxWinPercent between the partial payout and partial margin like below.

    _toMint = _handleCloseFees(_trade.asset, uint256(_payout)*_percent/DIVISION_CONSTANT, _trade.tigAsset, _positionSize*_percent/DIVISION_CONSTANT, _trade.trader, _isBot);

    uint256 partialMarginToClose = _trade.margin * _percent / DIVISION_CONSTANT; //+++++++++++++++++++++++
    if (maxWinPercent > 0 && _toMint > partialMarginToClose*maxWinPercent/DIVISION_CONSTANT) { 
        _toMint = partialMarginToClose*maxWinPercent/DIVISION_CONSTANT;
    }

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/b2ebb8ea1def4927a747e7a185174892506540ab/contracts/TradingExtension.sol#L118

Vulnerability details

Impact

TradingExtension._limitClose() returns a wrong stoploss which is favorable for users and it would be a significant loss for the protocol.

Proof of Concept

TradingExtension._limitClose() is used to set takeprofit/stoploss prices for the pending order and execute them.

File: 2022-12-tigris\contracts\TradingExtension.sol
088:     function _limitClose(
089:         uint _id,
090:         bool _tp,
091:         PriceData calldata _priceData,
092:         bytes calldata _signature
093:     ) external view returns(uint _limitPrice, address _tigAsset) {
    ...
111:         } else {
112:             if (_trade.slPrice == 0) revert("7"); //LimitNotSet
113:             if (_trade.direction) {
114:                 if (_trade.slPrice < _price) revert("6"); //LimitNotMet
115:             } else {
116:                 if (_trade.slPrice > _price) revert("6"); //LimitNotMet
117:             }
118:             _limitPrice = _trade.slPrice; //@audit should be _price
119:         }
120:     }

It always returns a preset takeprofit or stoploss after checking prices and stoploss price shouldn't be set like that.

For example, a user opens a long position at 100 and sets stoploss = 90 and if the price drops to 80 rapidly, the position will be closed automatically at 80(not 90 the stoploss price).

I've been a trader for several years and it's normal for many trading platforms.

But in the documentation, it says Stop order prices are not guaranteed. which is absolutely correct but no mention about stoploss which isn't guaranteed as well.

Totally, the current implementation of stoploss is profitable for users as they can close orders at predetermined stoploss without any slippage.

It means the protocol would suffer some loss because of slippage instead of traders.

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend using a not guaranteed stoploss like normal trading platforms.

    function _limitClose(
        uint _id,
        bool _tp,
        PriceData calldata _priceData,
        bytes calldata _signature
    ) external view returns(uint _limitPrice, address _tigAsset) {
       ...
        } 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 = _price; //++++++++++++++++++++++++++++
        }
    }

Lines of code

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

Vulnerability details

Impact

Users should deposit a full amount of _addMargin but it deducts _fee now.

As a result, users will pay less funds than they should.

Proof of Concept

In addToPosition(), users deposits _addMargin - _fee after the fee calculation.

File: 2022-12-tigris\contracts\Trading.sol
274:         uint _fee = _handleOpenFees(_trade.asset, _addMargin*_trade.leverage/1e18, _trader, _trade.tigAsset, false);
275:         _handleDeposit(
276:             _trade.tigAsset,
277:             _marginAsset,
278:             _addMargin - _fee, //@audit -fee?
279:             _stableVault,
280:             _permitData,
281:             _trader
282:         );

Actually, users should deposit _addMargin because several kinds of fees are paid in _handleOpenFees() already.

Other deposit logics like initiateMarketOrder() were implemented correctly but it's wrong only in addToPosition().

Tools Used

Manual Review

Recommended Mitigation Steps

_handleDeposit() should use _addMargin instead of _addMargin - _fee.

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/b2ebb8ea1def4927a747e7a185174892506540ab/contracts/Trading.sol#L952 https://github.com/code-423n4/2022-12-tigris/blob/b2ebb8ea1def4927a747e7a185174892506540ab/contracts/Trading.sol#L493

Vulnerability details

Impact

New fee settings shouldn't be applied to already existing orders.

Otherwise, users might pay more fees than they've expected with the old fee settings.

Proof of Concept

Both OpenFees and CloseFees can be changed anytime by admin using setFees().

And when it calculates fees using _handleOpenFees() and _handleCloseFees(), it uses current fee settings and the below scenario would be possible.

1. At the first time, the open fee was 0.05% and a user opened a limit order using initiateLimitOrder() as the fee is low.
2. After that, the open fee was changed to 0.1% which is not good for a user.
3. When the pending order is executed using executeLimitOrder(), new fee(0.1%) will be used and it's not fair for the user.

Similar cases would happen with close fees as well.

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend storing original fee settings when the order is opened and using those settings for the order.

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/b2ebb8ea1def4927a747e7a185174892506540ab/contracts/Trading.sol#L506

Vulnerability details

Impact

The open price of a stop order might be changed during execution but it doesn't validate StopLoss/TakeProfit for the changed price.

As a result, the executed market order might be closed immediately and there would be an unexpected loss for users.

Proof of Concept

As we can see from executeLimitOrder(), the open price might be changed to the current price for the stop order.

File: 2022-12-tigris\contracts\Trading.sol
480:     function executeLimitOrder(
481:         uint _id, 
482:         PriceData calldata _priceData,
483:         bytes calldata _signature
484:     ) 
485:         external
486:     {
487:         unchecked {
488:             _checkDelay(_id, true);
489:             tradingExtension._checkGas();
490:             if (tradingExtension.paused()) revert TradingPaused();
491:             require(block.timestamp >= limitDelay[_id]);
492:             IPosition.Trade memory trade = position.trades(_id);
493:             uint _fee = _handleOpenFees(trade.asset, trade.margin*trade.leverage/1e18, trade.trader, trade.tigAsset, true);
494:             (uint256 _price, uint256 _spread) = tradingExtension.getVerifiedPrice(trade.asset, _priceData, _signature, 0);
495:             if (trade.orderType == 0) revert("5");
496:             if (_price > trade.price+trade.price*limitOrderPriceRange/DIVISION_CONSTANT || _price < trade.price-trade.price*limitOrderPriceRange/DIVISION_CONSTANT) revert("6"); //LimitNotMet
497:             if (trade.direction && trade.orderType == 1) {
498:                 if (trade.price < _price) revert("6"); //LimitNotMet
499:             } else if (!trade.direction && trade.orderType == 1) {
500:                 if (trade.price > _price) revert("6"); //LimitNotMet
501:             } else if (!trade.direction && trade.orderType == 2) {
502:                 if (trade.price < _price) revert("6"); //LimitNotMet
503:                 trade.price = _price;
504:             } else {
505:                 if (trade.price > _price) revert("6"); //LimitNotMet
506:                 trade.price = _price; //@audit check sl/tp
507:             } 
508:             if(trade.direction) {
509:                 trade.price += trade.price * _spread / DIVISION_CONSTANT;
510:             } else {
511:                 trade.price -= trade.price * _spread / DIVISION_CONSTANT;
512:             }

But it doesn't validate sl/tp again for the new price so the order might have an invalid sl/tp.

The new price wouldn't satisfy the sl/tp requirements when the price was changed much from the original price due to the high slippage and the order might be closed immediately by sl or tp in this case.

Originally, the protocol validates stoploss only but I say to validate both of stoploss and takeprofit. (I submitted it as another issue to validate tp as well as sl).

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend validating sl/tp for the new trade.price in Trading.executeLimitOrder().

504:             } else {
505:                 if (trade.price > _price) revert("6"); //LimitNotMet
506:                 trade.price = _price; //@audit check sl/tp
507:             } 

[L-01] GovNFT.setAllowedAsset() should check if the _asset is in assets array

• https://github.com/code-423n4/2022-12-tigris/blob/b2ebb8ea1def4927a747e7a185174892506540ab/contracts/GovNFT.sol#L307-L309

    function setAllowedAsset(address _asset, bool _bool) external onlyOwner { 
        _allowedAsset[_asset] = _bool;
    }

• https://github.com/code-423n4/2022-12-tigris/blob/b2ebb8ea1def4927a747e7a185174892506540ab/contracts/GovNFT.sol#L287-L294

    function distribute(address _tigAsset, uint _amount) external {
        if (assets.length == 0 || assets[assetsIndex[_tigAsset]] == address(0) || totalSupply() == 0 || !_allowedAsset[_tigAsset]) return;
        try IERC20(_tigAsset).transferFrom(_msgSender(), address(this), _amount) {
            accRewardsPerNFT[_tigAsset] += _amount/totalSupply();
        } catch {
            return;
        }
    }

When it checks a valid asset in distribute(), it checks if _tigAsset is in the assets array first.

So even if _allowedAsset[_asset] is true, the _asset will be considered as invalid if it's not in assets array.

Also, it's meaningless to set false to the _asset that is not in the array.

We can modify setAllowedAsset() like below.

    function setAllowedAsset(address _asset, bool _bool) external onlyOwner {
        require(assets[assetsIndex[_asset]] == _asset, "Should be added first"); //++++++++++++++++++
        _allowedAsset[_asset] = _bool;
    }

[L-02] There is no option to remove elements from assets array

• https://github.com/code-423n4/2022-12-tigris/blob/b2ebb8ea1def4927a747e7a185174892506540ab/contracts/GovNFT.sol#L264

There should be a deletion logic for assets even though an asset can be disabled using _allowedAsset.

[L-03] Needless logic that might bring an uint underflow

• https://github.com/code-423n4/2022-12-tigris/blob/b2ebb8ea1def4927a747e7a185174892506540ab/contracts/PairsContract.sol#L161
• https://github.com/code-423n4/2022-12-tigris/blob/b2ebb8ea1def4927a747e7a185174892506540ab/contracts/PairsContract.sol#L181

File: 2022-12-tigris\contracts\PairsContract.sol
161:             if (_idToOi[_asset][_tigAsset].longOi < 1e9) {
162:                 _idToOi[_asset][_tigAsset].longOi = 0;
163:             }

It resets longOi/shortOi for dust amounts and it might bring an uint underflow later because the longOi/shortOi are smaller than real values.

I think it would be almost impossible to happen as we check a minPositionSize but such unnecessary logic should be removed.

[L-04] Possible rounding issue

• https://github.com/code-423n4/2022-12-tigris/blob/b2ebb8ea1def4927a747e7a185174892506540ab/contracts/Trading.sol#L295

File: 2022-12-tigris\contracts\Trading.sol
295:             uint _newPrice = _trade.price*_trade.margin/_newMargin + _price*_addMargin/_newMargin;

There might be a rounding issue as it divides seperately and we can modify like below.

uint _newPrice = (_trade.price * _trade.margin + _price * _addMargin) / _newMargin;

[L-05] There should an lower limit for maxWinPercent

• https://github.com/code-423n4/2022-12-tigris/blob/b2ebb8ea1def4927a747e7a185174892506540ab/contracts/Trading.sol#L926-L933
• https://github.com/code-423n4/2022-12-tigris/blob/b2ebb8ea1def4927a747e7a185174892506540ab/contracts/Trading.sol#L625

Currently, there is no lower limit of maxWinPercent and users will receive only 1/1e10 portion of margin when the admin sets maxWinPercent = 1 maliciously.

[L-06] Too strict requirements for _margin

• https://github.com/code-423n4/2022-12-tigris/blob/b2ebb8ea1def4927a747e7a185174892506540ab/contracts/Trading.sol#L651-L654

File: 2022-12-tigris\contracts\Trading.sol
651:             IERC20(_marginAsset).transferFrom(_trader, address(this), _margin/_marginDecMultiplier);
652:             IERC20(_marginAsset).approve(_stableVault, type(uint).max);
653:             IStableVault(_stableVault).deposit(_marginAsset, _margin/_marginDecMultiplier);
654:             if (tigAsset.balanceOf(address(this)) != _balBefore + _margin) revert BadDeposit(); //@audit bad accuracy
655:             tigAsset.burnFrom(address(this), tigAsset.balanceOf(address(this)));

It reverts if the increased amount is the same as _margin and users might get unnecessary reverts.

For example, if _marginAsset is an USDC of 6 decimals, _margin should be divided by 1e12 and it will revert otherwise.

It would be confusing for normal users as they can input any _margin amount for the _tigAsset.

We can modify like below to work for any cases.

    IERC20(_marginAsset).transferFrom(_trader, address(this), _margin/_marginDecMultiplier);
    IERC20(_marginAsset).approve(_stableVault, type(uint).max);
    IStableVault(_stableVault).deposit(_marginAsset, (_margin + _marginDecMultiplier - 1)/_marginDecMultiplier); //++++++++++
    if (tigAsset.balanceOf(address(this)) < _balBefore + _margin) revert BadDeposit(); //++++++++++++

[L-07] Wrong comment

• https://github.com/code-423n4/2022-12-tigris/blob/b2ebb8ea1def4927a747e7a185174892506540ab/contracts/TradingExtension.sol#L82

* @param _tp true if long, else short

It should be changed to @param _tp true if takeprofit, else stoploss

[L-08] Possible underflow

• https://github.com/code-423n4/2022-12-tigris/blob/b2ebb8ea1def4927a747e7a185174892506540ab/contracts/Trading.sol#L286
• https://github.com/code-423n4/2022-12-tigris/blob/b2ebb8ea1def4927a747e7a185174892506540ab/contracts/Trading.sol#L519

These parts are inside the unchecked block and the final amount after extracting fee would be huge for uint underflow.

This case would be happen when the fee is greater than margin which means feePercent * leverage > 1.

Currently it's not possible with the default fee/leverage settings but it might happen for some weird settings.

We should confirm the fee is less than margin for any cases.

[L-09] Not implemented like a documentation

• https://github.com/code-423n4/2022-12-tigris/blob/b2ebb8ea1def4927a747e7a185174892506540ab/contracts/Trading.sol#L295

    uint _newMargin = _trade.margin + _addMargin;
    uint _newPrice = _trade.price*_trade.margin/_newMargin + _price*_addMargin/_newMargin;

• https://docs.tigris.trade/protocol/trading-and-fees/opening-a-position-on-top-of-another-one

If you're adding to a long trade, your opening price will be higher and your liquidation price will be lower.
If you're adding to a short trade, your opening price will be lower and your liquidation price will be higher.

But during the _newPrice calculation, the new price can be higher or lower than the original price according to current open price.

I am wondering if there should be another requirement to add positions in order to work as the document.

[N-01] Typo

• https://github.com/code-423n4/2022-12-tigris/blob/b2ebb8ea1def4927a747e7a185174892506540ab/contracts/PairsContract.sol#L151
• https://github.com/code-423n4/2022-12-tigris/blob/b2ebb8ea1def4927a747e7a185174892506540ab/contracts/PairsContract.sol#L171

Change interesr to interest