Governance NFT holder, whose NFT was minted before Trading._handleOpenFees function is called, can lose deserved rewards after Trading._handleOpenFees function is called

[code423n4]

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

[c4-judge]

GalloDaSballo marked the issue as primary issue

[GalloDaSballo]

This is even better (POC test vs POC log)

[TriHaz]

That will happen only with the first opened position until _handleCloseFees() is called.
Valid but I think it should be low risk as it will mostly not affect anyone.
Also the funds that are not distributed will be distributed later because of gov.distribute(_tigAsset, IStable(_tigAsset).balanceOf(address(this))); so no funds will be lost.

[c4-sponsor]

TriHaz marked the issue as disagree with severity

[c4-sponsor]

TriHaz marked the issue as sponsor confirmed

[GalloDaSballo]

The warden has shown how, due to a lack of approvals, the rewards earned until the first call to _handleCloseFees

We also know that _handleDeposit will burn the balance of tigAsset that is unused.

The risk however, is limited to the first (one or) few users, for this reason I believe that Medium Severity is more appropriate.

Adding an approval on deployment or before calling distribute should help mitigate

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)

[c4-judge]

GalloDaSballo marked the issue as selected for report

[GainsGoblin]

Mitigation: code-423n4/2022-12-tigris#2 (comment)