Buyer can skip paying fees, if he repeatedly buy small amounts

[hats-bug-reporter[bot]]

Github username: @NicolaMirchev Twitter username: EgisSec Submission hash (on-chain): 0x604b983f899823829c9cbdc1af002fdfab0f8d953c478d5e7fd4b68839dc9f91 Severity: medium

Description: Description\ Currently FM_BC_Bancor_Redeeming_VirtualSupply_v1 is the only module, which is being refered as a workflow. There is default workflow fees for executing specific operations. This fee is set by Inverter governance and is payed by users, who executes workflow operations, no matter orchestrator admin. There is also another fee, which is set by orchestrator admin and it is used to calculate his share of specific user deposit. Here is the part of the code, which is responsible for calculating protocol and orchestrator shares, when an end user want to buy an issuance token from a give bonding curve manager:

   IERC20 collateralToken = __Module_orchestrator.fundingManager().token();

        // Transfer collateral, confirming that correct amount == allowance
        collateralToken.safeTransferFrom(
            _msgSender(), address(this), _depositAmount
        );
        // Get protocol fee percentages and treasury addresses
        (
            address collateralTreasury,
            address issuanceTreasury,
            uint collateralBuyFeePercentage,
            uint issuanceBuyFeePercentage
        ) = _getFunctionFeesAndTreasuryAddresses(
            bytes4(keccak256(bytes("_buyOrder(address, uint, uint)"))) // @audit is it correct to use the internal func selector for this?
        );

        // Get net amount, protocol and workflow fee amounts
        (uint netDeposit, uint protocolFeeAmount, uint workflowFeeAmount) =
        _calculateNetAndSplitFees(
            _depositAmount, collateralBuyFeePercentage, buyFee
        );

        // collateral Fee Amount is the combination of protocolFeeAmount plus the workflowFeeAmount
        collateralFeeAmount = protocolFeeAmount + workflowFeeAmount;

        // Process the protocol fee
        _processProtocolFeeViaTransfer(
            collateralTreasury, collateralToken, protocolFeeAmount
        );

        // Add workflow fee if applicable
        if (workflowFeeAmount > 0) {
            projectCollateralFeeCollected += workflowFeeAmount;
        }

        // Calculate mint amount based on upstream formula
        uint issuanceMintAmount = _issueTokensFormulaWrapper(netDeposit);
        totalIssuanceTokenMinted = issuanceMintAmount;

        // Get net amount, protocol and workflow fee amounts. Currently there is no issuance project
        // fee enabled
        (issuanceMintAmount, protocolFeeAmount, /* workflowFeeAmount */ ) =
        _calculateNetAndSplitFees(
            issuanceMintAmount, issuanceBuyFeePercentage, 0
        );
        // collect protocol fee on outgoing issuance token
        _processProtocolFeeViaMinting(issuanceTreasury, protocolFeeAmount);

        // Revert when the mint amount is lower than minimum amount the user expects
        if (issuanceMintAmount < _minAmountOut) {
            revert Module__BondingCurveBase__InsufficientOutputAmount();
        }
        // Mint tokens to address
        _mint(_receiver, issuanceMintAmount);

And here is how shares for protocol and orchestrator are calculated:

    function _calculateNetAndSplitFees(
        uint _totalAmount,
        uint _protocolFee,
        uint _workflowFee
    )
        internal
        pure
        returns (uint netAmount, uint protocolFeeAmount, uint workflowFeeAmount)
    {
        if ((_protocolFee + _workflowFee) > BPS) { // @audit this is reachable for collateral fee
            revert Module__BondingCurveBase__FeeAmountToHigh();
        }
        protocolFeeAmount = _totalAmount * _protocolFee / BPS;
        workflowFeeAmount = _totalAmount * _workflowFee / BPS;
        netAmount = _totalAmount - protocolFeeAmount - workflowFeeAmount;
    }

We can notice that if totalAmount is low enough, so multiplied by the corresponding fee result in value < BPS (10_000), this would result in rounding down to zero and so user skipping fee payment. Value deposited should be low enough, but protocol would be deployed on L2 chains, where fees are neglectable, so it may be beneficial for the expoiter to make large large loop of buyOrder transaction with small amount, so he would be able to skip fee payment.

    function buyFor(address _receiver, uint _depositAmount, uint _minAmountOut)
        public
        virtual
        override(BondingCurveBase_v1)
        validReceiver(_receiver)
        buyingIsEnabled
    {
        (uint amountIssued, uint collateralFeeAmount) =
            _buyOrder(_receiver, _depositAmount, _minAmountOut);
        _addVirtualIssuanceAmount(amountIssued);
        _addVirtualCollateralAmount(_depositAmount - collateralFeeAmount);
    }

Attack Scenario\ Read the above statement

Attachments

1. Proof of Concept (PoC) File Will provide if needed.

2. Revised Code File (Optional) Introduce minDeposit variable, so it is not possible for expoiters to use a loops with small amounts to skip paying fees

[NicolaMirchev]

I am sorry for the spam submissions. We have already submitted this one: #22