Tigris Trade contest - rbserver's results

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

[L-04] USDT IS NOT SUPPORTED FOR CALLING Trading._handleDeposit FUNCTION ON ETHEREUM MAINNET As shown by https://etherscan.io/token/0xdac17f958d2ee523a2206206994597c13d831ec7#code#L205, USDT on the Ethereum mainnet does not allow approving a new amount when the existing approved amount is not zero. On the Ethereum mainnet, when calling the following Trading._handleDeposit function with USDT being the margin asset, executing IStableVault(_stableVault).deposit(_marginAsset, _margin/_marginDecMultiplier) will not use all of the approved type(uint).max amount so executing IERC20(_marginAsset).approve(_stableVault, type(uint).max) can revert when calling this function for the second time. In the future, if the protocol wants to support USDT as a margin asset on the Ethereum mainnet, IERC20(_marginAsset).approve(_stableVault, 0); can be added before the line of IERC20(_marginAsset).approve(_stableVault, type(uint).max); in the Trading._handleDeposit function.

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L643-L659

function _handleDeposit(address _tigAsset, address _marginAsset, uint256 _margin, address _stableVault, ERC20PermitData calldata _permitData, address _trader) internal {
    IStable tigAsset = IStable(_tigAsset);
    if (_tigAsset != _marginAsset) {
        ...
        IERC20(_marginAsset).transferFrom(_trader, address(this), _margin/_marginDecMultiplier);
        IERC20(_marginAsset).approve(_stableVault, type(uint).max);
        IStableVault(_stableVault).deposit(_marginAsset, _margin/_marginDecMultiplier);
        ...
    } else {
        ...
    }        
}

Lines of code

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

Vulnerability details

Impact

According to https://docs.tigris.trade/protocol/governance, "Profits from trading fees are paid out to [Governance] NFT holders in real-time...Rewards are paid out in Tigris stablecoins." However, for some legitimate reasons, such as if the corresponding Tigris stablecoin has a bug, or if the owner of the GovNFT contract becomes compromised or malicious, this owner can call the following GovNFT.setAllowedAsset function to stop the corresponding Tigris stablecoin from being used as a reward token. After this happens, when calling functions like Trading._handleOpenFees and Trading._handleCloseFees, the GovNFT.distribute function below that is further called would not transfer the trade's DAO fee amount of the corresponding Tigris stablecoin from the Trading contract to the GovNFT contract. Instead, such amount can remain in the Trading contract without belonging to anyone when functions like Trading.initiateCloseOrder are called. As a result, the Governance NFT holders cannot receive more deserved rewards from the DAO fees generated by the trades as long as the corresponding Tigris stablecoin is not allowed for being used as a reward token, which can be permanent if such Tigris stablecoin can no longer be used due to a bug or the GovNFT contract's owner is compromised or malicious.

https://github.com/code-423n4/2022-12-tigris/blob/main/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/main/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;
        }
    }

Proof of Concept

Please add the following test in the Trading using <18 decimal token describe block in test\07.Trading.js. This test will pass to demonstrate the described scenario. Please see the comments in this test for more details.

    it.only(`GovNFT contract's owner can stop Governance NFT holders from receiving more rewards from trades' DAO fees,
             and such reward amounts can remain in Trading contract without belonging to anyone`, async function () {
      let TradeInfo = [parseEther("1000"), MockUSDC.address, StableVault.address, parseEther("5"), 0, true, parseEther("0"), parseEther("0"), ethers.constants.HashZero];
      let openPriceData = [node.address, 0, parseEther("10000"), 0, 2000000000, false];
      let openMessage = ethers.utils.keccak256(
        ethers.utils.defaultAbiCoder.encode(
          ['address', 'uint256', 'uint256', 'uint256', 'uint256', 'bool'],
          [node.address, 0, parseEther("10000"), 0, 2000000000, false]
        )
      );
      let openSig = await node.signMessage(
        Buffer.from(openMessage.substring(2), 'hex')
      );
      let PermitData = [permitSigUsdc.deadline, ethers.constants.MaxUint256, permitSigUsdc.v, permitSigUsdc.r, permitSigUsdc.s, true];

      await trading.connect(owner).initiateMarketOrder(TradeInfo, openPriceData, openSig, PermitData, owner.address);

      let closePriceData = [node.address, 0, parseEther("9000"), 0, 2000000000, false];
      let closeMessage = ethers.utils.keccak256(
        ethers.utils.defaultAbiCoder.encode(
          ['address', 'uint256', 'uint256', 'uint256', 'uint256', 'bool'],
          [node.address, 0, parseEther("9000"), 0, 2000000000, false]
        )
      );
      let closeSig = await node.signMessage(
        Buffer.from(closeMessage.substring(2), 'hex')
      );

      // one Governance NFT is minted to owner and transferred to user before initiateCloseOrder function is called
      const GovNFT = await deployments.get("GovNFT");
      const govnft = await ethers.getContractAt("GovNFT", GovNFT.address);
      await govnft.connect(owner).mint();
      await govnft.connect(owner).transferFrom(owner.getAddress(), user.getAddress(), 1);

      // Calling initiateCloseOrder function for closing half of the corresponding position
      //   attempts to send 2238750000000000000 tigAsset as DAO fees to GovNFT contract.
      await expect(trading.connect(owner).initiateCloseOrder(1, 0.5e10, closePriceData, closeSig, StableVault.address, MockUSDC.address, owner.address))
        .to.emit(trading, 'FeesDistributed')
        .withArgs(stabletoken.address, "2238750000000000000", "0", "0", "0", ethers.constants.AddressZero);

      // user's pending reward amount is 2238750000000000000 because her or his Governance NFT was minted before initiateCloseOrder function was called
      expect(await govnft.pending(user.getAddress(), stabletoken.address)).to.equal("2238750000000000000");

      // GovNFT contract's owner is able to call setAllowedAsset function to immediately prevent the previously allowed tigAsset from being used as reward token
      await govnft.connect(owner).setAllowedAsset(stabletoken.address, false);

      // Calling initiateCloseOrder function again for closing the other half of the corresponding position
      //   attempts to send another 2238750000000000000 tigAsset as DAO fees to GovNFT contract.
      await expect(trading.connect(owner).initiateCloseOrder(1, 1e10, closePriceData, closeSig, StableVault.address, MockUSDC.address, owner.address))
        .to.emit(trading, 'FeesDistributed')
        .withArgs(stabletoken.address, "2238750000000000000", "0", "0", "0", ethers.constants.AddressZero);

      // However, because of the previous setAllowedAsset function call,
      //   the attempt for sending more tigAsset as DAO fees to GovNFT contract by the previous initiateCloseOrder function call fails,
      //   and user's pending reward amount is still 2238750000000000000.
      expect(await govnft.pending(user.getAddress(), stabletoken.address)).to.equal("2238750000000000000");

      // the 2238750000000000000 tigAsset, which were supposed to be sent to GovNFT contract, remain in Trading contract without belonging to anyone
      expect(await stabletoken.balanceOf(trading.address)).to.equal("2238750000000000000");
    });

Tools Used

VSCode

Recommended Mitigation Steps

An upgradeable backup Tigris stablecoin can be set up. When calling the GovNFT.distribute function, if the Tigris stablecoin that is originally used as a reward token is no longer allowed which causes the !_allowedAsset[_tigAsset] condition to be true, then the trade's DAO fee amount of the backup Tigris stablecoin can be minted to the GovNFT contract for the Governance NFT holders to claim later.

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

[L-05] MARGIN ASSET TOKENS WITH MORE THAN 18 DECIMALS ARE NOT SUPPORTED As shown below, arithmetic operations of the StableVault.deposit, StableVault.withdraw, Trading._handleDeposit, and Trading._handleWithdraw functions that subtract the margin asset tokens' decimals will underflow if these decimals are more than 18; in this case, calling these functions will revert. This means that the protocol cannot scale to support margin asset tokens that have more than 18 decimals in the future. To prevent the described issue, please consider updating these functions' arithmetic operations to use divisions that divide 1018 by 10n, where n is the corresponding token decimals instead of subtracting such token decimals from 18.

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/StableVault.sol#L44-L51

function deposit(address _token, uint256 _amount) public {
    ...
    IERC20Mintable(stable).mintFor(
        _msgSender(),
        _amount*(10**(18-IERC20Mintable(_token).decimals()))
    );
}

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/StableVault.sol#L65-L72

function withdraw(address _token, uint256 _amount) external returns (uint256 _output) {
    ...
    _output = _amount/10**(18-IERC20Mintable(_token).decimals());
    ...
}

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L643-L659

function _handleDeposit(address _tigAsset, address _marginAsset, uint256 _margin, address _stableVault, ERC20PermitData calldata _permitData, address _trader) internal {
    ...
    if (_tigAsset != _marginAsset) {
        ...
        uint _marginDecMultiplier = 10**(18-ExtendedIERC20(_marginAsset).decimals());
        ...
    } else {
        ...
    }        
}

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L668-L678

function _handleWithdraw(IPosition.Trade memory _trade, address _stableVault, address _outputToken, uint _toMint) internal {
    ...
    if (_outputToken == _trade.tigAsset) {
        ...
    } else {
        ...
        if (IERC20(_outputToken).balanceOf(address(this)) != _balBefore + _toMint/(10**(18-ExtendedIERC20(_outputToken).decimals()))) revert BadWithdraw();
        ...
    }        
}

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L689-L750 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L762-L810 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/GovNFT.sol#L287-L294

Vulnerability details

Impact

Calling the following Trading._handleOpenFees function does not approve the GovNFT contract for spending any of the Trading contract's _tigAsset balance, which is unlike calling the Trading._handleCloseFees function below that executes IStable(_tigAsset).approve(address(gov), type(uint).max). Due to this lack of approval, when calling the Trading._handleOpenFees function without the Trading._handleCloseFees function being called for the same _tigAsset beforehand, the GovNFT.distribute function's execution of IERC20(_tigAsset).transferFrom(_msgSender(), address(this), _amount) in the try...catch... block will not transfer any _tigAsset amount as the trade's DAO fees to the GovNFT contract. In this case, although the Governance NFT holder, whose NFT was minted before the Trading._handleOpenFees function is called, deserves the rewards from the DAO fees generated by the trade, this holder does not have any pending rewards after such Trading._handleOpenFees function call because none of the DAO fees were transferred to the GovNFT contract. Hence, this Governance NFT holder loses the rewards that she or he is entitled to.

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L689-L750

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

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L762-L810

    function _handleCloseFees(
        uint _asset,
        uint _payout,
        address _tigAsset,
        uint _positionSize,
        address _trader,
        bool _isBot
    )
        internal
        returns (uint payout_)
    {
        ...
        IStable(_tigAsset).mintFor(address(this), _daoFeesPaid);
        IStable(_tigAsset).approve(address(gov), type(uint).max);
        gov.distribute(_tigAsset, _daoFeesPaid);
        return payout_;
    }

https://github.com/code-423n4/2022-12-tigris/blob/main/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;
        }
    }

Proof of Concept

Functions like Trading.initiateMarketOrder further call the Trading._handleOpenFees function so this POC uses the Trading.initiateMarketOrder function.

Please add the following test in the Signature verification describe block in test\07.Trading.js. This test will pass to demonstrate the described scenario. Please see the comments in this test for more details.

    it.only("Governance NFT holder, whose NFT was minted before initiateMarketOrder function is called, can lose deserved rewards after initiateMarketOrder function is called", async function () {
      let TradeInfo = [parseEther("1000"), MockDAI.address, StableVault.address, parseEther("10"), 0, true, parseEther("30000"), parseEther("10000"), ethers.constants.HashZero];
      let PriceData = [node.address, 0, parseEther("20000"), 0, 2000000000, false];
      let message = ethers.utils.keccak256(
        ethers.utils.defaultAbiCoder.encode(
          ['address', 'uint256', 'uint256', 'uint256', 'uint256', 'bool'],
          [node.address, 0, parseEther("20000"), 0, 2000000000, false]
        )
      );
      let sig = await node.signMessage(
        Buffer.from(message.substring(2), 'hex')
      );
      
      let PermitData = [permitSig.deadline, ethers.constants.MaxUint256, permitSig.v, permitSig.r, permitSig.s, true];

      // one Governance NFT is minted to owner before initiateMarketOrder function is called
      const GovNFT = await deployments.get("GovNFT");
      const govnft = await ethers.getContractAt("GovNFT", GovNFT.address);
      await govnft.connect(owner).mint();

      // calling initiateMarketOrder function attempts to send 10000000000000000000 tigAsset as DAO fees to GovNFT contract
      await expect(trading.connect(owner).initiateMarketOrder(TradeInfo, PriceData, sig, PermitData, owner.address))
        .to.emit(trading, 'FeesDistributed')
        .withArgs(stabletoken.address, "10000000000000000000", "0", "0", "0", ethers.constants.AddressZero);

      // another Governance NFT is minted to owner and then transferred to user after initiateMarketOrder function is called
      await govnft.connect(owner).mint();
      await govnft.connect(owner).transferFrom(owner.getAddress(), user.getAddress(), 1);

      // user's pending reward amount should be 0 because her or his Governance NFT was minted after initiateMarketOrder function was called
      expect(await govnft.pending(user.getAddress(), stabletoken.address)).to.equal("0");

      // owner's Governance NFT was minted before initiateMarketOrder function was called so her or his pending reward amount should be 10000000000000000000.
      // However, owner's pending reward amount is still 0 because DAO fees were not transferred to GovNFT contract successfully.
      expect(await govnft.pending(owner.getAddress(), stabletoken.address)).to.equal("0");
    });

Furthermore, as a suggested mitigation, please add IStable(_tigAsset).approve(address(gov), type(uint).max); in the _handleOpenFees function as follows in line 749 of contracts\Trading.sol.

689:     function _handleOpenFees(
690:         uint _asset,
691:         uint _positionSize,
692:         address _trader,
693:         address _tigAsset,
694:         bool _isBot
695:     )
696:         internal
697:         returns (uint _feePaid)
698:     {
699:         IPairsContract.Asset memory asset = pairsContract.idToAsset(_asset);
...
732:         unchecked {
733:             uint _daoFeesPaid = _positionSize * _fees.daoFees / DIVISION_CONSTANT;
734:             _feePaid =
735:                 _positionSize
736:                 * (_fees.burnFees + _fees.botFees) // get total fee%
737:                 / DIVISION_CONSTANT // divide by 100%
738:                 + _daoFeesPaid;
739:             emit FeesDistributed(
740:                 _tigAsset,
741:                 _daoFeesPaid,
742:                 _positionSize * _fees.burnFees / DIVISION_CONSTANT,
743:                 _referrer != address(0) ? _positionSize * _fees.referralFees / DIVISION_CONSTANT : 0,
744:                 _positionSize * _fees.botFees / DIVISION_CONSTANT,
745:                 _referrer
746:             );
747:             IStable(_tigAsset).mintFor(address(this), _daoFeesPaid);
748:         }
749:         IStable(_tigAsset).approve(address(gov), type(uint).max);   // @audit add this line of code for POC purpose
750:         gov.distribute(_tigAsset, IStable(_tigAsset).balanceOf(address(this)));
751:     }

Then, as a comparison, the following test can be added in the Signature verification describe block in test\07.Trading.js. This test will pass to demonstrate that the Governance NFT holder's pending rewards is no longer 0 after implementing the suggested mitigation. Please see the comments in this test for more details.

    it.only(`If calling initiateMarketOrder function can correctly send DAO fees to GovNFT contract, Governance NFT holder, whose NFT was minted before initiateMarketOrder function is called,
             can receive deserved rewards after initiateMarketOrder function is called`, async function () {
      let TradeInfo = [parseEther("1000"), MockDAI.address, StableVault.address, parseEther("10"), 0, true, parseEther("30000"), parseEther("10000"), ethers.constants.HashZero];
      let PriceData = [node.address, 0, parseEther("20000"), 0, 2000000000, false];
      let message = ethers.utils.keccak256(
        ethers.utils.defaultAbiCoder.encode(
          ['address', 'uint256', 'uint256', 'uint256', 'uint256', 'bool'],
          [node.address, 0, parseEther("20000"), 0, 2000000000, false]
        )
      );
      let sig = await node.signMessage(
        Buffer.from(message.substring(2), 'hex')
      );
      
      let PermitData = [permitSig.deadline, ethers.constants.MaxUint256, permitSig.v, permitSig.r, permitSig.s, true];

      // one Governance NFT is minted to owner before initiateMarketOrder function is called
      const GovNFT = await deployments.get("GovNFT");
      const govnft = await ethers.getContractAt("GovNFT", GovNFT.address);
      await govnft.connect(owner).mint();

      // calling initiateMarketOrder function attempts to send 10000000000000000000 tigAsset as DAO fees to GovNFT contract
      await expect(trading.connect(owner).initiateMarketOrder(TradeInfo, PriceData, sig, PermitData, owner.address))
        .to.emit(trading, 'FeesDistributed')
        .withArgs(stabletoken.address, "10000000000000000000", "0", "0", "0", ethers.constants.AddressZero);

      // another Governance NFT is minted to owner and then transferred to user after initiateMarketOrder function is called
      await govnft.connect(owner).mint();
      await govnft.connect(owner).transferFrom(owner.getAddress(), user.getAddress(), 1);

      // user's pending reward amount should be 0 because her or his Governance NFT was minted after initiateMarketOrder function was called
      expect(await govnft.pending(user.getAddress(), stabletoken.address)).to.equal("0");

      // If calling initiateMarketOrder function can correctly send DAO fees to GovNFT contract, owner's pending reward amount should be 10000000000000000000
      //   because her or his Governance NFT was minted before initiateMarketOrder function was called.
      expect(await govnft.pending(owner.getAddress(), stabletoken.address)).to.equal("10000000000000000000");
    });

Tools Used

VSCode

Recommended Mitigation Steps

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L749 can be updated to the following code.

        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/main/contracts/utils/TradingLibrary.sol#L91-L122

Vulnerability details

Impact

As mentioned by https://docs.tigris.trade/protocol/oracle, "Prices provided by the oracle network are also compared to Chainlink's public price feeds for additional security. If prices have more than a 2% difference the transaction is reverted." The Chainlink price verification logic in the following TradingLibrary.verifyPrice function serves this purpose. However, besides that IPrice(_chainlinkFeed).latestAnswer() uses Chainlink's deprecated latestAnswer function, this function also does not guarantee that the price returned by the Chainlink price feed is not stale. When assetChainlinkPriceInt != 0 is true, it is still possible that assetChainlinkPriceInt is stale in which the Chainlink price verification would compare the off-chain price against a stale price returned by the Chainlink price feed. For a off-chain price that has more than a 2% difference when comparing to a more current price returned by the Chainlink price feed, this off-chain price can be incorrectly considered to have less than a 2% difference when comparing to a stale price returned by the Chainlink price feed. As a result, a trading transaction that should revert can go through, which makes the price verification much less secure.

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

    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());
                require(
                    _priceData.price < assetChainlinkPrice+assetChainlinkPrice*2/100 &&
                    _priceData.price > assetChainlinkPrice-assetChainlinkPrice*2/100, "!chainlinkPrice"
                );
            }
        }
    }

Based on https://docs.chain.link/docs/historical-price-data, the followings can be done to avoid using a stale price returned by the Chainlink price feed.

1. The latestRoundData function can be used instead of the deprecated latestAnswer function.
2. roundId and answeredInRound are also returned. "You can check answeredInRound against the current roundId. If answeredInRound is less than roundId, the answer is being carried over. If answeredInRound is equal to roundId, then the answer is fresh."
3. "A read can revert if the caller is requesting the details of a round that was invalid or has not yet been answered. If you are deriving a round ID without having observed it before, the round might not be complete. To check the round, validate that the timestamp on that round is not 0."

Proof of Concept

The following steps can occur for the described scenario.

1. Alice calls the Trading.initiateMarketOrder function, which eventually calls the TradingLibrary.verifyPrice function, to initiate a market order.
2. When the TradingLibrary.verifyPrice function is called, the off-chain price is compared to the price returned by the Chainlink price feed for the position asset.
3. The price returned by the Chainlink price feed is stale, and the off-chain price has less than a 2% difference when comparing to this stale price.
4. Alice's Trading.initiateMarketOrder transaction goes through. However, this transaction should revert because the off-chain price has more than a 2% difference if comparing to a more current price returned by the Chainlink price feed.

Tools Used

VSCode

Recommended Mitigation Steps

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/utils/TradingLibrary.sol#L113 can be updated to the following code.

            (uint80 roundId, int256 assetChainlinkPriceInt, , uint256 updatedAt, uint80 answeredInRound) = IPrice(_chainlinkFeed).latestRoundData();
            require(answeredInRound >= roundId, "price is stale");
            require(updatedAt > 0, "round is incomplete");

[L-01] NO UPPER LIMIT ON NUMBER OF REWARD ASSET TOKENS IN GovNFT CONTRACT

When calling the following GovNFT._mint, GovNFT._bridgeMint, GovNFT._burn, and GovNFT._transfer functions, the added reward asset tokens are iterated over. After many reward asset tokens are added, assetsLength() can become quite large, and the gas cost for calling these functions can be hugely increased. When the number of reward asset tokens becomes large enough, it is possible that the block gas limit is eventually reached, which causes these function calls to revert, and the users can no longer use these functionalities for the Governance NFTs. To prevent the potential DOS issue, please consider enforcing a maximum number of reward asset tokens that can be added in the GovNFT contract.

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/GovNFT.sol#L50-L57

    function _mint(address to, uint256 tokenId) internal override {
        ...
        for (uint i=0; i<assetsLength(); i++) {
            userPaid[to][assets[i]] += accRewardsPerNFT[assets[i]];
        }
        ...
    }

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

    function _bridgeMint(address to, uint256 tokenId) public {
        ...
        for (uint i=0; i<assetsLength(); i++) {
            userPaid[to][assets[i]] += accRewardsPerNFT[assets[i]];
        }
        ...
    }

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/GovNFT.sol#L76-L84

    function _burn(uint256 tokenId) internal override {
        ...
        for (uint i=0; i<assetsLength(); i++) {
            userDebt[owner][assets[i]] += accRewardsPerNFT[assets[i]];
            userDebt[owner][assets[i]] -= userPaid[owner][assets[i]]/balanceOf(owner);
            userPaid[owner][assets[i]] -= userPaid[owner][assets[i]]/balanceOf(owner);            
        }
        ...
    }

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/GovNFT.sol#L89-L102

    function _transfer(
        address from,
        address to,
        uint256 tokenId
    ) internal override {
        ...
        for (uint i=0; i<assetsLength(); i++) {
            userDebt[from][assets[i]] += accRewardsPerNFT[assets[i]];
            userDebt[from][assets[i]] -= userPaid[from][assets[i]]/balanceOf(from);
            userPaid[from][assets[i]] -= userPaid[from][assets[i]]/balanceOf(from);
            userPaid[to][assets[i]] += accRewardsPerNFT[assets[i]];
        }
        ...
    }

[L-02] super._safeMint FUNCTION CAN BE CALLED INSTEAD OF super._mint FUNCTION FOR MINTING GOVERNANCE NFTS

Calling the following GovNFT._mint and GovNFT._bridgeMint functions will mint the Governance NFT for tokenId to the to address. Both functions call the super._mint function. If the to address corresponds to a contract, calling the super._mint function does not check if the receiving contract supports the ERC721 protocol; if not supported, the minted NFT can be locked and cannot be retrieved. To make sure that the receiving contract supports the ERC721 protocol, please consider calling the super._safeMint function instead of the super._mint function in the GovNFT._mint and GovNFT._bridgeMint functions.

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/GovNFT.sol#L50-L57

    function _mint(address to, uint256 tokenId) internal override {
        ...
        super._mint(to, tokenId);
    }

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

    function _bridgeMint(address to, uint256 tokenId) public {
        ...
        super._mint(to, tokenId);
    }

[L-03] super._safeTransfer FUNCTION CAN BE CALLED INSTEAD OF super._transfer FUNCTION FOR TRANSFERRING GOVERNANCE NFTS

When the following GovNFT._transfer function is called, the super._transfer function is called to transfer the Governance NFT for tokenId from the from address to the to address. When the to address corresponds to a contract, calling the super._transfer function does not check if the receiving contract supports the ERC721 protocol; if not supported, the transferred NFT can be locked and cannot be retrieved. To ensure that the receiving contract supports the ERC721 protocol, please consider calling the super._safeTransfer function instead of the super._transfer function in the GovNFT._transfer function.

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/GovNFT.sol#L89-L102

    function _transfer(
        address from,
        address to,
        uint256 tokenId
    ) internal override {
        ...
        super._transfer(from, to, tokenId);
    }

[L-04] USDT IS NOT SUPPORTED FOR CALLING Trading._handleDeposit FUNCTION ON ETHEREUM MAINNET

As shown by https://etherscan.io/token/0xdac17f958d2ee523a2206206994597c13d831ec7#code#L205, USDT on the Ethereum mainnet does not allow approving a new amount when the existing approved amount is not zero. On the Ethereum mainnet, when calling the following Trading._handleDeposit function with USDT being the margin asset, executing IStableVault(_stableVault).deposit(_marginAsset, _margin/_marginDecMultiplier) will not use all of the approved type(uint).max amount so executing IERC20(_marginAsset).approve(_stableVault, type(uint).max) can revert when calling this function for the second time. In the future, if the protocol wants to support USDT as a margin asset on the Ethereum mainnet, IERC20(_marginAsset).approve(_stableVault, 0); can be added before the line of IERC20(_marginAsset).approve(_stableVault, type(uint).max); in the Trading._handleDeposit function.

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L643-L659

    function _handleDeposit(address _tigAsset, address _marginAsset, uint256 _margin, address _stableVault, ERC20PermitData calldata _permitData, address _trader) internal {
        IStable tigAsset = IStable(_tigAsset);
        if (_tigAsset != _marginAsset) {
            ...
            IERC20(_marginAsset).transferFrom(_trader, address(this), _margin/_marginDecMultiplier);
            IERC20(_marginAsset).approve(_stableVault, type(uint).max);
            IStableVault(_stableVault).deposit(_marginAsset, _margin/_marginDecMultiplier);
            ...
        } else {
            ...
        }        
    }

[L-05] MARGIN ASSET TOKENS WITH MORE THAN 18 DECIMALS ARE NOT SUPPORTED

As shown below, arithmetic operations of the StableVault.deposit, StableVault.withdraw, Trading._handleDeposit, and Trading._handleWithdraw functions that subtract the margin asset tokens' decimals will underflow if these decimals are more than 18; in this case, calling these functions will revert. This means that the protocol cannot scale to support margin asset tokens that have more than 18 decimals in the future. To prevent the described issue, please consider updating these functions' arithmetic operations to use divisions that divide 10**18 by 10**n, where n is the corresponding token decimals instead of subtracting such token decimals from 18.

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/StableVault.sol#L44-L51

    function deposit(address _token, uint256 _amount) public {
        ...
        IERC20Mintable(stable).mintFor(
            _msgSender(),
            _amount*(10**(18-IERC20Mintable(_token).decimals()))
        );
    }

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/StableVault.sol#L65-L72

    function withdraw(address _token, uint256 _amount) external returns (uint256 _output) {
        ...
        _output = _amount/10**(18-IERC20Mintable(_token).decimals());
        ...
    }

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L643-L659

    function _handleDeposit(address _tigAsset, address _marginAsset, uint256 _margin, address _stableVault, ERC20PermitData calldata _permitData, address _trader) internal {
        ...
        if (_tigAsset != _marginAsset) {
            ...
            uint _marginDecMultiplier = 10**(18-ExtendedIERC20(_marginAsset).decimals());
            ...
        } else {
            ...
        }        
    }

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L668-L678

    function _handleWithdraw(IPosition.Trade memory _trade, address _stableVault, address _outputToken, uint _toMint) internal {
        ...
        if (_outputToken == _trade.tigAsset) {
            ...
        } else {
            ...
            if (IERC20(_outputToken).balanceOf(address(this)) != _balBefore + _toMint/(10**(18-ExtendedIERC20(_outputToken).decimals()))) revert BadWithdraw();
            ...
        }        
    }

[L-06] UNSAFE decimals() CALL FOR MARGIN ASSET TOKENS THAT DO NOT IMPLEMENT decimals()

The following StableVault.deposit, StableVault.withdraw, Trading._handleDeposit, and Trading._handleWithdraw functions call decimals() to get the decimals of the corresponding margin asset tokens. However, according to https://eips.ethereum.org/EIPS/eip-20, decimals() is optional so it is possible that some margin asset tokens do not implement decimals(). For such tokens, calling these functions will revert. To scale the protocol for supporting margin asset tokens that do not implement decimals() in the future, helper functions like BoringCrypto's safeDecimals can be used instead of directly calling decimals() in these functions.

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/StableVault.sol#L44-L51

    function deposit(address _token, uint256 _amount) public {
        ...
        IERC20Mintable(stable).mintFor(
            _msgSender(),
            _amount*(10**(18-IERC20Mintable(_token).decimals()))
        );
    }

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/StableVault.sol#L65-L72

    function withdraw(address _token, uint256 _amount) external returns (uint256 _output) {
        ...
        _output = _amount/10**(18-IERC20Mintable(_token).decimals());
        ...
    }

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L643-L659

    function _handleDeposit(address _tigAsset, address _marginAsset, uint256 _margin, address _stableVault, ERC20PermitData calldata _permitData, address _trader) internal {
        ...
        if (_tigAsset != _marginAsset) {
            ...
            uint _marginDecMultiplier = 10**(18-ExtendedIERC20(_marginAsset).decimals());
            ...
        } else {
            ...
        }        
    }

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L668-L678

    function _handleWithdraw(IPosition.Trade memory _trade, address _stableVault, address _outputToken, uint _toMint) internal {
        ...
        if (_outputToken == _trade.tigAsset) {
            ...
        } else {
            ...
            if (IERC20(_outputToken).balanceOf(address(this)) != _balBefore + _toMint/(10**(18-ExtendedIERC20(_outputToken).decimals()))) revert BadWithdraw();
            ...
        }        
    }