Government NFT opening fees are burned instead of correctly distributed to holders

[code423n4]

Lines of code

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

Vulnerability details

Impact

Outgoing opening fees are not being approved before calling the GovNFT.sol: distribute() function in Trading.sol: _handleOpenFees(). Thus they cannot be transferred to the Government NFT contract and will be burned during opening.
This makes the NFTs not worth as much and also breaks the intended flow of funds in the protocol.

The faulty _handleOpenFees() function is called in:

• Trading.sol: initiateMarketOrder()
• Trading.sol: addToPosition()
• Trading.sol: executeLimitOrder()

The approval of _tigAsset is missing from Trading.sol: _handleOpenFees() function:

function _handleOpenFees(
    uint _asset,
    uint _positionSize,
    address _trader,
    address _tigAsset,
    bool _isBot
)
    internal
    returns (uint _feePaid)
{
    IPairsContract.Asset memory asset = pairsContract.idToAsset(_asset);
    Fees memory _fees = openFees;
    unchecked {
        _fees.daoFees = _fees.daoFees * asset.feeMultiplier / DIVISION_CONSTANT;
        _fees.burnFees = _fees.burnFees * asset.feeMultiplier / DIVISION_CONSTANT;
        _fees.referralFees = _fees.referralFees * asset.feeMultiplier / DIVISION_CONSTANT;
        _fees.botFees = _fees.botFees * asset.feeMultiplier / DIVISION_CONSTANT;
    }
    address _referrer = tradingExtension.getRef(_trader); //referrals.getReferral(referrals.getReferred(_trader));
    if (_referrer != address(0)) {
        unchecked {
            IStable(_tigAsset).mintFor(
                _referrer,
                _positionSize
                * _fees.referralFees // get referral fee%
                / DIVISION_CONSTANT // divide by 100%
            );
        }
        _fees.daoFees = _fees.daoFees - _fees.referralFees*2;
    }
    if (_isBot) {
        unchecked {
            IStable(_tigAsset).mintFor(
                _msgSender(),
                _positionSize
                * _fees.botFees // get bot fee%
                / DIVISION_CONSTANT // divide by 100%
            );
        }
        _fees.daoFees = _fees.daoFees - _fees.botFees;
    } else {
    _fees.botFees = 0;
    }
    unchecked {
        uint _daoFeesPaid = _positionSize * _fees.daoFees / DIVISION_CONSTANT;
        _feePaid =
            _positionSize
            * (_fees.burnFees + _fees.botFees) // get total fee%
            / DIVISION_CONSTANT // divide by 100%
            + _daoFeesPaid;
        emit FeesDistributed(
            _tigAsset,
            _daoFeesPaid,
            _positionSize * _fees.burnFees / DIVISION_CONSTANT,
            _referrer != address(0) ? _positionSize * _fees.referralFees / DIVISION_CONSTANT : 0,
            _positionSize * _fees.botFees / DIVISION_CONSTANT,
            _referrer
        );
        IStable(_tigAsset).mintFor(address(this), _daoFeesPaid);
    }
        
    gov.distribute(_tigAsset, IStable(_tigAsset).balanceOf(address(this)));
}

The approve() function should be before the last line of the function above.

Here is the distribute function, notice that it doesn't revert if the transfer of funds fails, it only returns which is why this bug is harder to notice:

/**
* @notice add rewards for NFT holders
* @param _tigAsset reward token address
* @param _amount amount to be distributed
*/
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

This test displays the issue, please edit test/07.Trading.js to make this work. Also add the govnft and GovNFT variables from test/05.GovNFT.js.

it("Distributing open fees does not correctly distribute to NFT holders", async function () {
  // Mint GOV NFT
  await govnft.connect(owner).mint();
  expect(await govnft.totalSupply()).to.equal(1);
  console.log("pending rewards before opening:\n ", parseInt(Number(await govnft.connect(owner).pending(owner.address, stabletoken.address)), 10));
  await stabletoken.connect(owner).setMinter(owner.address, true)
  await stabletoken.connect(owner).mintFor(owner.address, parseEther("20000"))
  await trading.connect(owner).setFees(true,3e8,1e8,1e8,1e8,1e8);
  let TradeInfo = [parseEther("1000"), MockDAI.address, StableVault.address, parseEther("10"), 0, false, parseEther("10000"), parseEther("0")/* SL = 0*/, 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];
  // Initiate a market order, this will call the faulty _handleOpenFees() function
  await trading.connect(owner).initiateMarketOrder(TradeInfo, PriceData, sig, PermitData, owner.address);
  expect(await position.assetOpenPositionsLength(0)).to.equal(1); // Trade has opened

  console.log("pending rewards after opening:\n ", parseInt(Number(await govnft.connect(owner).pending(owner.address, stabletoken.address)), 10));
});

Adding the approve function in the smart contract can be seen affect the pending rewards in the test.

Tools Used

Manual review with Visual Studio Code

Recommended Mitigation Steps

Call IStable(_tigAsset).approve(address(gov), type(uint).max); before distributing the funds to NFT holders as it is done correctly in Trading.sol: _handleCloseFees().
After thinking for a while I will be submitting this as high since assets (GOV NFT rewards) can and will be lost (burned) if not fixed.

[GalloDaSballo]

Making primary because it has coded POC

[c4-judge]

GalloDaSballo marked the issue as primary issue

[c4-judge]

GalloDaSballo marked the issue as duplicate of #649

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)

[c4-judge]

GalloDaSballo marked the issue as satisfactory