code423n4 commented 1 year ago

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.

TriHaz commented 1 year ago

We are aware of the centralization risks, initially, all contracts will have a multi-sig as owner to prevent a sole owner, later on a DAO could be the owner.

c4-sponsor commented 1 year ago

TriHaz marked the issue as sponsor acknowledged

c4-judge commented 1 year ago

GalloDaSballo marked the issue as duplicate of #377