Trading.sol : unsafe fee calculation

[code423n4]

Lines of code

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

Vulnerability details

Impact

When fee calculation underflow, the calculated fee value would be very huge. The fee could be at the value of max of uint 256.

Proof of Concept

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

/**
 * @dev handle fees distribution for closing
 * @param _asset asset id
 * @param _payout payout to trader before fees
 * @param _tigAsset margin asset
 * @param _positionSize position size
 * @param _trader trader address
 * @param _isBot false if closed via market order
 * @return payout_ payout to trader after fees
 */
function _handleCloseFees(
    uint _asset,
    uint _payout,
    address _tigAsset,
    uint _positionSize,
    address _trader,
    bool _isBot
)
    internal
    returns (uint payout_)
{
    IPairsContract.Asset memory asset = pairsContract.idToAsset(_asset);
    Fees memory _fees = closeFees;
    uint _daoFeesPaid;
    uint _burnFeesPaid;
    uint _referralFeesPaid;
    unchecked {
        _daoFeesPaid = (_positionSize*_fees.daoFees/DIVISION_CONSTANT)*asset.feeMultiplier/DIVISION_CONSTANT;
        _burnFeesPaid = (_positionSize*_fees.burnFees/DIVISION_CONSTANT)*asset.feeMultiplier/DIVISION_CONSTANT;
    }
    uint _botFeesPaid;
    address _referrer = tradingExtension.getRef(_trader);//referrals.getReferral(referrals.getReferred(_trader));
    if (_referrer != address(0)) {
        unchecked {
            _referralFeesPaid = (_positionSize*_fees.referralFees/DIVISION_CONSTANT)*asset.feeMultiplier/DIVISION_CONSTANT;
        }
        IStable(_tigAsset).mintFor(
            _referrer,
            _referralFeesPaid
        );
         _daoFeesPaid = _daoFeesPaid-_referralFeesPaid*2;
    }
    if (_isBot) {
        unchecked {
            _botFeesPaid = (_positionSize*_fees.botFees/DIVISION_CONSTANT)*asset.feeMultiplier/DIVISION_CONSTANT;
            IStable(_tigAsset).mintFor(
                _msgSender(),
                _botFeesPaid
            );
        }
        _daoFeesPaid = _daoFeesPaid - _botFeesPaid;
    }
    emit FeesDistributed(_tigAsset, _daoFeesPaid, _burnFeesPaid, _referralFeesPaid, _botFeesPaid, _referrer);
    payout_ = _payout - _daoFeesPaid - _burnFeesPaid - _botFeesPaid;
    IStable(_tigAsset).mintFor(address(this), _daoFeesPaid);
    IStable(_tigAsset).approve(address(gov), type(uint).max);
    gov.distribute(_tigAsset, _daoFeesPaid);
    return payout_;
}

both _handleOpenFees and _handleCloseFees hanlde calcualtion inside the unchecked block. I believe that the protocol team is aware of the underflow/overflow and revert due to this.

In case underflow happen, the fee value would shoot up to max of uint256.

in one of the case the blow line will be executed and the _daoFeesPaid would be max of unit256

IStable(_tigAsset).mintFor(address(this), _daoFeesPaid);

minting for max uint256 would be meaningless.

Tools Used

Manual review

Recommended Mitigation Steps

It is suggested to check the each fee value for a max of some threshold limit. If it exceeds that limit, update the codes to set pre-determined value.

[TriHaz]

Invalid, because fee setter function makes sure the values are set in a way that it wouldn't underflow.

[c4-sponsor]

TriHaz marked the issue as sponsor disputed

[GalloDaSballo]

I mostly agree with the sponsor, but because of other reports concerning fees having an uncapped value, as well as this report pointing to the usage of unchecked, I believe this report to be valid, but of Low Severity as it requires setting the fees to an unreasonable value, which can only be done by governance

[GalloDaSballo]

L

[c4-judge]

Duplicate of https://github.com/code-423n4/2022-12-tigris-findings/issues/465