code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

When calling the following Trading.setFees function to set fees for opening and closing positions, there are no upper limits for these fees. If the owner of the Trading contract becomes compromised or malicious, this owner can set these fees to be more than 100%. When this happens, as shown below, calling functions like Trading.initiateMarketOrder and Trading._handleCloseFees can revert due to underflowed arithmetic operations caused by the high fees that are more than 100%. It is possible that the fees for opening position are set to normal values but the fees for closing position are set to values that are larger than 100%. In this case, for example, a user can initiate a market order but will fail to initiate closing position for this market order. As a result, this user is forced to lose the deposited margin.

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

    function setFees(bool _open, uint _daoFees, uint _burnFees, uint _referralFees, uint _botFees, uint _percent) external onlyOwner {
        unchecked {
            require(_daoFees >= _botFees+_referralFees*2);
            if (_open) {
                openFees.daoFees = _daoFees;
                openFees.burnFees = _burnFees;
                openFees.referralFees = _referralFees;
                openFees.botFees = _botFees;
            } else {
                closeFees.daoFees = _daoFees;
                closeFees.burnFees = _burnFees;
                closeFees.referralFees = _referralFees;
                closeFees.botFees = _botFees;                
            }
            require(_percent <= DIVISION_CONSTANT);
            vaultFundingPercent = _percent;
        }
    }

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

    function initiateMarketOrder(
        TradeInfo calldata _tradeInfo,
        PriceData calldata _priceData,
        bytes calldata _signature,
        ERC20PermitData calldata _permitData,
        address _trader
    )
        external
    {
        ...
        uint256 _marginAfterFees = _tradeInfo.margin - _handleOpenFees(_tradeInfo.asset, _tradeInfo.margin*_tradeInfo.leverage/1e18, _trader, _tigAsset, false);
        ...
    }

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_)
    {
        ...
        payout_ = _payout - _daoFeesPaid - _burnFeesPaid - _botFeesPaid;
        ...
        return payout_;
    }

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(`Owner of Trading contract can set fees to be bigger than 100% for blocking users from taking important trading actions, such as initiating closing position`, 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];

      // owner is able to initiate a market order
      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')
      );

      // owner of Trading contract can set fees, such as DAO fees for closing position, to be bigger than 100%
      await trading.connect(owner).setFees(false, 2e10, 0, 0, 0, 0);

      // calling initiateCloseOrder function by owner for initiating closing position for the corresponding market order reverts
      await expect(
        trading.connect(owner).initiateCloseOrder(1, 1e10, closePriceData, closeSig, StableVault.address, MockUSDC.address, owner.address)
      ).to.be.reverted;

      // such reversion is due to the underflowed arithmetic operation caused by the high DAO fees for closing position that is more than 100%
      try {
        await trading.connect(owner).initiateCloseOrder(1, 1e10, closePriceData, closeSig, StableVault.address, MockUSDC.address, owner.address);
      }
      catch (err) {
        expect(err.toString()).to.equal(
          "Error: VM Exception while processing transaction: reverted with panic code 0x11 (Arithmetic operation underflowed or overflowed outside of an unchecked block)"
        );
      }
    });

Tools Used

VSCode

Recommended Mitigation Steps

In the Trading.setFees function, each fee, which would be set, needs to be capped below a sensible upper limit that is less than 100%.

TriHaz commented 1 year ago

Duplicate of #15

GalloDaSballo commented 1 year ago

Coded POC making it primary

c4-judge commented 1 year ago

GalloDaSballo marked the issue as primary issue

GalloDaSballo commented 1 year ago

Because this pertains to a risk that would happen only if triggered by the Admin, and we already have a generic Admin Privilege Report, am downgrading to Low Severity L