Tigris Trade contest - unforgiven's results

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/BondNFT.sol#L160-L187 https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Lock.sol#L29-L41

Vulnerability details

Impact

Function Lock.claim() claims pending rewards from a bond and it's callable for expired bond too. but if a user calls it multiple time after bond expired then function bondNFT.claim() would calculate _pendingDelta and increase accRewardsPerShare[][] value based on _pendingDelta multiple times and the value of accRewardsPerShare[][] would be higher than it should be and the rewards would get distributed wrongly. attacker can call claim() so many times and cause accRewardsPerShare[][] to be very wrong and then he can claim rewards for his other bond and withdraw all the rewards and steal other users rewards.

Proof of Concept

This is Lock.claim() code:

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

As you can see there is no check that this bond is not expired and code just calls bondNFT.claim() and code don't release the bond if the bond is expired and a user can call this function multiple times even if his bond is expired. this is bondNFT.claim() code:

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 you can see if the bond is expired code tries to calculating rewards assigned because of this expired bond during the epochs after the expiration time which is _pendingDelta and then user redistribute that _pendingDelta among current shares. but if this function gets called multiple times for expired bond the value of the _pendingDelta won't get decreased (only would get decreased the first time because of the bondPaid[][]) and code won't know that already calculated and redistributed _pendingDelta and it would increase accRewardsPerShare[][] value in each call by mistake and accRewardsPerShare[][] would show wrong value. This is the calculation of the _pendingDelata:

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

as you can see for expired bond bond.shares is constant, the value of accRewardsPerShare[bond.asset][epoch[bond.asset]] would only increase over time, the value of bondPaid[_id][bond.asset] would be constant after the first claim when bond expired and the value of accRewardsPerShare[bond.asset][bond.expireEpoch-1] would be constant too so _pendingDelta value would be increase after second call to claim() when bond is expired and attacker can call claim() as many time as he wants when bond expired and each time code would recalculate _pendingDelta and redistribute those rewards by increasing accRewardsPerShare[][]. to exploit this, attacker would perform this steps;

1. create bond1 with lots of amount for short period.
2. create another bond2 with larger period and never call claim() for bond2.
3. after bond1 expired attacker would call claim(bond1) for 1000 times and this would make code increase accRewardsPerShare[][] very very much by mistake.
4. attacker then call claim(bond2) and get a lot of rewards because rewards calculation has been manipulated. and attacker would steal other users rewards.

attacker can perform the attack in one transaction and by using smart contract so it's possible to do the attack confidently without any interrupts.

Tools Used

VIM

Recommended Mitigation Steps

don't allow claiming for expired bond and expired bond should only be get released.

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Position.sol#L126-L161

Vulnerability details

Impact

Function Position.mint() has been used in initiateLimitOrder() and initiateMarketOrder() and it doesn't follow check-effect-interaction pattern and code updates the values of _limitOrders, initId, _openPositions and position _tokenIds variables after making external call by using safeMint(). This would give attacker opportunity to reenter the Trading contract logics and perform malicious action while contract storage state is wrong. the only limitation of the attacker is that he need to bypass _checkDelay() checks. attacker can perform this action:

1. call initiateLimitOrder() and create limit order with id equal to ID1 reenter (while _limitOrders for ID1 is not yet settled) with cancelLimitOrder(ID1) (no checkDelay() check) and remove other users limit orders because code would try to remove _limitOrderIndexes[_asset][ID1] position but the value is 0 and code would remove limit order in the index 0 which belongs to another user in the Position.burn() code.
2. call initiateMarketOrder() and create a position with ID1 and while initId[ID1] has not yet settled reenter the Trading with addToPosition(ID1) function (bypass checkDelay() because both action is opening) and increase the position size which would set initId[ID1] according to new position values but then when code execution returns to rest of mint() logic initId[ID1] would set by initial values of the positions which is very lower than what it should be and initId[ID1] has been used for calculating accuredInterest of the position which is calculated for profit and loss of position and contract would calculate more profit for position and would pay attacker more profit from contract balances.

Proof of Concept

This is mint() code in Position contract:

    function mint(
        MintTrade memory _mintTrade
    ) external onlyMinter {
        uint newTokenID = _tokenIds.current();

        Trade storage newTrade = _trades[newTokenID];
        newTrade.margin = _mintTrade.margin;
        newTrade.leverage = _mintTrade.leverage;
        newTrade.asset = _mintTrade.asset;
        newTrade.direction = _mintTrade.direction;
        newTrade.price = _mintTrade.price;
        newTrade.tpPrice = _mintTrade.tp;
        newTrade.slPrice = _mintTrade.sl;
        newTrade.orderType = _mintTrade.orderType;
        newTrade.id = newTokenID;
        newTrade.tigAsset = _mintTrade.tigAsset;

        _safeMint(_mintTrade.account, newTokenID);   // make external call because of safeMint() usage
        if (_mintTrade.orderType > 0) { // update the values of some storage functions
            _limitOrders[_mintTrade.asset].push(newTokenID);
            _limitOrderIndexes[_mintTrade.asset][newTokenID] = _limitOrders[_mintTrade.asset].length-1;
        } else {
            initId[newTokenID] = accInterestPerOi[_mintTrade.asset][_mintTrade.tigAsset][_mintTrade.direction]*int256(_mintTrade.margin*_mintTrade.leverage/1e18)/1e18;
            _openPositions.push(newTokenID);
            _openPositionsIndexes[newTokenID] = _openPositions.length-1;

            _assetOpenPositions[_mintTrade.asset].push(newTokenID);
            _assetOpenPositionsIndexes[_mintTrade.asset][newTokenID] = _assetOpenPositions[_mintTrade.asset].length-1;
        }
        _tokenIds.increment();
    }

As you can see by calling _safeMint() code would make external call to onERC721Received() function of the account address and the code sets the values for _limitOrders[], _limitOrderIndexes[], initId[], _openPositions[], _openPositionsIndexes[], _assetOpenPositions[], _assetOpenPositionsIndexes[] and _tokenIds. so code don't follow check-effect-interaction pattern and it's possible to perform reentrancy attack. there could be multiple scenarios that attacker can perform the attack and do some damage. two of them are:

scenario #1 where attacker remove other users limit orders and create broken storage state

1. attacker contract would call initiateLimitOrder() and code would create the limit order and mint it in the Position._safeMint() with ID1.
2. then code would call attacker address in _safeMint() function because of the onERC721Received() call check.
3. variables _limitOrders[], _limitOrderIndexes[ID1] are not yet updated for ID1 and _limitOrderIndexes[ID1] is 0x0 and ID1 is not in _limitOrder[] list.
4. attacker contract would reenter the Trading contract by calling cancelLimitOrder(ID1).
5. cancelLimitOrder() checks would pass and would tries to call Position.burn(ID1).
6. burn() function would tries to remove ID1 from _limitOrders[] list but because _limitOrderIndexes[ID1] is 0 so code would remove the 0 index limit order which is belongs to another user.
7. execution would return to Position.mint() logic and code would add burned id token to _limitOrder[] list.

so there is two impact here, first other users limit order got removed and the second is that contract storage had bad state and burned tokens get stock in the list.

scenario #2 where attacker steal contract/users funds by wrong profit calculation

1. attacker's contract would call initiateMarketOrder(lowMargin) to create position with ID1 while the margin is low.
2. code would mint position token for attacker and in _safeMint() would make external call and call onERC721Received() function of attacker address.
3. the value of initId[ID1] is not yet set for ID1.
4. attacker contract would call addToPosition(ID1, bigMargin) to increase the margin of the position the _checkDelay() check would pass because both actions are opening position.
5. code would increase the margin of the position and set the value of the initId[ID1] by calling position.addToPosition() and the value were be based on the newMargin.
6. the execution flow would receive the rest of Position.mint() function and code would set initId[ID1] based on old margin value.
7. then the value of initId[ID1] for attacker position would be very low which would cause accInterest to be very higher than it supposed to be for position(in Position.trades() function calculations ) and would cause _payout value to be very high (in pnl() function's calculations) and when attacker close position ID1 attacker would receive a lot more profit from it.

so attacker created a position with a lot of profit by reentering the logics and manipulating calculation of the profits for the position.

there can be other scenarios possible to perform and damage the protocol or users because there is no reentrancy protection mechanism and attacker only need to bypass validity checks of functions.

Tools Used

VIM

Recommended Mitigation Steps

follow the check-effect-interaction pattern.

Judge has assessed an item in Issue #445 as M risk. The relevant finding follows:

[[5]] Function crossChain() in GovNFT should have limit for maximum tokens allowed to be transferred, because of gas limit in the dest chain. if a user transferred a lot of tokens because there was two loop inside each other in handling logic(one loop through NFT token ids and one loop through assets list so the execution is in order #NFTs * #Assets) so transaction can be revert in the destination chain. https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/GovNFT.sol#L124-L165

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Referrals.sol#L20-L24 https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/TradingExtension.sol#L148-L152

Vulnerability details

Impact

By default the value of _referred[user] is 0x0 for all users and if one set 0x0 as his referral hash then he would become referral for all the users who didn't set referral by default and he would earn a lot of referral funds that users didn't approve it.

Proof of Concept

This is createReferralCode() code:

    function createReferralCode(bytes32 _hash) external {
        require(_referral[_hash] == address(0), "Referral code already exists");
        _referral[_hash] = _msgSender();
        emit ReferralCreated(_msgSender(), _hash);
    }

As you can see attacker can become set 0x0 as his hash referral by calling createReferralCode(0x0) and code would set _referral[0x0] = attackerAddress (attacker needs to be the first one calling this). Then in the getRef() code the logic would return attackerAddress as referral for all the users who didn't set referral.

    function getRef(
        address _trader
    ) external view returns(address) {
        return referrals.getReferral(referrals.getReferred(_trader));
    }

in the code, getReferred(trader) would return 0x0 because trader didn't set referred and getReferral(0x0) would return attackerAddress. _handleOpenFees() and _handleCloseFees() function in the Trading contract would use getRef(trader) and they would transfer referral fee to attackerAddress and attacker would receive fee form a lot of users which didn't set any referral, those users didn't set any referral and didn't approve attacker receiving referral fees from them and because most of the users wouldn't know about this and referral codes so attacker would receive a lot of funds.

Tools Used

VIM

Recommended Mitigation Steps

prevent some one from setting 0x0 hash for their referral code.

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/BondNFT.sol#L206-L228 https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/BondNFT.sol#L48-L86

Vulnerability details

Impact

Function BondNFT.createLock() creates a bond and it sets bond's mint epoch as epoch[asset], function Lock.lock() first calls claimGovFees() which calls BondNFT.distribute() for all assets and updates the epoch[assets] for all assets. so during normal bond creation the value of epoch[asset] would be updated and bond would be created from today epoch to today+period epoch. but if totalShares[tigAsset] == 0 for an asset, then distribute() won't update epoch[asset] for that asset and epoch[asset] will be some old epoch(will be the start time where asset is added or the time where totalShares[_tigAsset] != 0). This would make createLock() to set very wrong value for bond's mint epoch when totalShares[tigAsset] == 0. This would happen for the first bond that has been created for that asset always and it will happen again if for some period totalShares[asset] become 0, then the next bond would have wrong mint epoch. or setAllowedAsset(asset, false) has been called for that asset.

Proof of Concept

This is distribute() code in BondNFT contract:

function distribute(
        address _tigAsset,
        uint _amount
    ) external {
        if (totalShares[_tigAsset] == 0 || !allowedAsset[_tigAsset]) return;
        IERC20(_tigAsset).transferFrom(_msgSender(), address(this), _amount);
        unchecked {
            uint aEpoch = block.timestamp / DAY;
            if (aEpoch > epoch[_tigAsset]) {
                for (uint i=epoch[_tigAsset]; i<aEpoch; i++) {
                    epoch[_tigAsset] += 1;
                    accRewardsPerShare[_tigAsset][i+1] = accRewardsPerShare[_tigAsset][i];
                }
            }
            accRewardsPerShare[_tigAsset][aEpoch] += _amount * 1e18 / totalShares[_tigAsset];
        }
        emit Distribution(_tigAsset, _amount);
    }

As you can see when totalShares[_tigAsset] == 0 then the value of epoch[_tigAsset] won't get updated to the today. and there is no other logics in the code to update epoch[tigAsset]. so when totalShares[_tigAsset] == 0 then the value of the epoch[tigAsset] would be out dated. this would happen when asset is recently added to the BondNFT assets or when in some time there is no bond left. When this condition happens and a user call Lock.lock() to create a bond the lock() function would call claimGovFees() to update rewards in BondNFT but because for that asset the value of totalShares are 0 so for that asset epoch[] won't get updated and in the BondNFT.createLock() the wrong value would set as bond't mint epoch. This is Lock.lock() code:

    function lock(
        address _asset,
        uint _amount,
        uint _period
    ) public {
        require(_period <= maxPeriod, "MAX PERIOD");
        require(_period >= minPeriod, "MIN PERIOD");
        require(allowedAssets[_asset], "!asset");

        claimGovFees();

        IERC20(_asset).transferFrom(msg.sender, address(this), _amount);
        totalLocked[_asset] += _amount;
        
        bondNFT.createLock( _asset, _amount, _period, msg.sender);
    }

And this is BondNFT.createLock() code:

    function createLock(
        address _asset,
        uint _amount,
        uint _period,
        address _owner
    ) external onlyManager() returns(uint id) {
        require(allowedAsset[_asset], "!Asset");
        unchecked {
            uint shares = _amount * _period / 365;
            uint expireEpoch = epoch[_asset] + _period;
            id = ++totalBonds;
            totalShares[_asset] += shares;
            Bond memory _bond = Bond(
                id,             // id
                address(0),     // owner
                _asset,         // tigAsset token
                _amount,        // tigAsset amount
                epoch[_asset],  // mint epoch
                block.timestamp,// mint timestamp
                expireEpoch,    // expire epoch
                0,              // pending
                shares,         // linearly scaling share of rewards
                _period,        // lock period
                false           // is expired boolean
            );
            _idToBond[id] = _bond;
            _mint(_owner, _bond);
        }
        emit Lock(_asset, _amount, _period, _owner, id);
    }

if a bond get wrong value for mint epoch it would have wrong value for expire epoch and user would get a lot of share by lock for small time. for example this scenario:

1. let's assume epoch[asset1] is out dated and it shows 30 days ago epoch. (allowedAsset[asset1] was false so locking was not possible and then is set as true after 30 days)
2. during this time because totalShare[asset1] was 0 so distribute() function won't udpate epoch[asset1] and epoch[asset1] would show 30 days ago.
3. attacker would create a lock for 32 days by calling Lock.lock(asset1). code would call BondNFT.createLock() and would create a bond for attacker which epoch start time is 30 days ago and epoch expire time is 2 days later and attacker receives shares for 32 days.
4. some reward would get distributed into the BondNFT for the asset1.
5. other users would create lock too.
6. attacker would claim his rewards and his rewards would be for 32 day locking but attacker lock his tokens for 2 days in reality.

so attacker was able to create lock for long time and get shares and rewards based on that but attacker can release lock after short time.

Tools Used

VIM

Recommended Mitigation Steps

update epoch[asset] in distribute() function even when totalShares[_tigAsset] is equal to 0. only the division by zero and fund transfer should be prevented when totalShare is zero and epoch[asset] index should be updated.

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Trading.sol#L649-L651 https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Trading.sol#L675 https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/utils/TradingLibrary.sol#L114-L117

Vulnerability details

Impact

Some tokens may have 18 digit decimal or have price feed more than 18 digit decimal and in current implementation code won't support them.

Proof of Concept

This is verfiyPrice() code in TradingLibrary:

    function verifyPrice(
        uint256 _validSignatureTimer,
        uint256 _asset,
        bool _chainlinkEnabled,
        address _chainlinkFeed,
        PriceData calldata _priceData,
        bytes calldata _signature,
        mapping(address => bool) storage _isNode
    )
        external view
    {
...............
..............
        if (_chainlinkEnabled && _chainlinkFeed != address(0)) {
            int256 assetChainlinkPriceInt = IPrice(_chainlinkFeed).latestAnswer();
            if (assetChainlinkPriceInt != 0) {
                uint256 assetChainlinkPrice = uint256(assetChainlinkPriceInt) * 10**(18 - IPrice(_chainlinkFeed).decimals());
         ...........
         ...........
    }

As you can see in the line 18 - IPrice(_chainlinkFeed).decimals() if the chainlinkFeed's price has more than 18 digit decimals then code would revert. function _handleDeposit() and _handleWithdraw() in Trading contract has similar code for _marginAsset (outputToken) and they don't support tokens with more than 18 decimals.

Tools Used

VIM

Recommended Mitigation Steps

support tokens with more than 18 decimals by adding IF based on token decimals and if decimals are bigger than 18 calculate like this (decimal-18).

Lines of code

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

Vulnerability details

Impact

Function _handleOpenFees() handles fees distribution for opening positions and it distribute DAO fees by calling distribute() function of govNFT contract but in current implementation there is no spending approval allowance code before calling distribute() so govNFT can't transfer fees and they would stuck in the Trading contract and they would be burned in the next order opening. the code of distribute() is robust and has try/catch it makes funds to be stuck in the Trading contract, if there wasn't try/catch in side distribute() then the transaction would revert.

Proof of Concept

This is _handleOpenFees() code:

    function _handleOpenFees(
        uint _asset,
        uint _positionSize,
        address _trader,
        address _tigAsset,
        bool _isBot
    )
        internal
        returns (uint _feePaid)
    {
..................
..................
..................
        unchecked {
            uint _daoFeesPaid = _positionSize * _fees.daoFees / DIVISION_CONSTANT;
..................
..................
..................
            IStable(_tigAsset).mintFor(address(this), _daoFeesPaid);
        }
        gov.distribute(_tigAsset, IStable(_tigAsset).balanceOf(address(this)));
    }

As you can see contract calls gov.distribute(_tigAsset, IStable(_tigAsset).balanceOf(address(this))); without giving spending allowance. the code in distribute() would try to transfer tokens but it would fail and because try/catch has been used in distribute() the transaction won't revert but the DAO fee would stuck in trading contract and won't get distributed. those stocked fees can be burned or can be transferred in the next actions based on the action type. if another market order got created next, then those tokens would get burned because in function _handleDeposit() code burns tigAsset balance of contract. this is _handleDeposit() code:

function _handleDeposit(address _tigAsset, address _marginAsset, uint256 _margin, address _stableVault, ERC20PermitData calldata _permitData, address _trader) internal {
        IStable tigAsset = IStable(_tigAsset);
        if (_tigAsset != _marginAsset) {
           ............
           ............
            tigAsset.burnFrom(address(this), tigAsset.balanceOf(address(this)));
        } else {
            tigAsset.burnFrom(_trader, _margin);
        }        
    }

This function is get called in creating positions or increasing positions margins and as you can see it burns all the tigAsset balance of the contract.

Tools Used

VIM

Recommended Mitigation Steps

give spending allowance before calling distribute()

[[1]]

safeTransferMany() in GoveNFT/BondNFT contracts are not using safe transferring, onERC721() function of receiver address doesn't get called because code calls _transfer()

    function safeTransferMany(address _to, uint[] calldata _ids) external {
        for (uint i=0; i<_ids.length; i++) {
            _transfer(_msgSender(), _to, _ids[i]);
        }
    }

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/BondNFT.sol#L282-L288 https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/GovNFT.sol#L245-L249

[[2]]

wrong condition for checking that if tigAsset is not in assets list. condition assets[assetsIndex[_tigAsset]] == address(0) will always be false (if assets[0] has value) because assetsIndex[nonAddedAsset] would be 0 and assets[0] would have some value if assets.leng() >0. the correct condition is assets[assetsIndex[_tigAsset]] != _tigAsset

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

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

[[3]]

there is no functionality in GovNFT and BondNFT to remove assets and if assets length increase too much(by mistake or intentionaly) then all the logics would fail because of the looping through assets. also the gas would go higher.

[[4]]

function _bridgeMint() uses hardcoded value 10000 instead of using MAX variable. if for some reason the number of NFT tokens changes and MAX has new value but 10000 didn't get changed then it won't be possible to transfer tokens with id>10000 to other chains (code won't mint them in the dest chain) and transferring them would cause those tokens to be lost.

    function _bridgeMint(address to, uint256 tokenId) public {
        require(msg.sender == address(this) || _msgSender() == owner(), "NotBridge");
        require(tokenId <= 10000, "BadID");
        for (uint i=0; i<assetsLength(); i++) {
            userPaid[to][assets[i]] += accRewardsPerNFT[assets[i]];
        }
        super._mint(to, tokenId);
    }

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/GovNFT.sol#L64-L71

[[5]]

Function crossChain() in GovNFT should have limit for maximum tokens allowed to be transferred, because of gas limit in the dest chain. if a user transferred a lot of tokens because there was two loop inside each other in handling logic(one loop through NFT token ids and one loop through assets list so the execution is in order #NFTs * #Assets) so transaction can be revert in the destination chain. https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/GovNFT.sol#L124-L165

[[6]]

setNode() in TradingExtension contract shouldn't allow owner to set 0x0 address as a valid node, specially when some ESCDA libraries return 0x0 address when signature is invalid and if code uses it in bad way (isNode[signer(message, hash)] == true) then all the messages with wrong signature would be calculated as valid messages.

    /**
     * @dev whitelists a node
     * @param _node node address
     * @param _bool bool
     */
    function setNode(address _node, bool _bool) external onlyOwner {
        isNode[_node] = _bool;
    }

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