Incorrect dev comment FM_BC_Bancor_Redeeming_VirtualSupply_v1

[hats-bug-reporter[bot]]

Github username: -- Twitter username: rnemes4 Submission hash (on-chain): 0x80554ec32b829953ed3817e0c35779f375e0ad6a17f9ce3b6d1761958592d1c7 Severity: low

Description: Description\ Incorrect dev comment in relation to the maximum _depositAmount allowed before revert due to:

/// @dev Redirects to the internal function _buyOrder by passing the receiver address and deposit amount. /// Important: The Bancor Formula has an upper computational limit of (10^38). For tokens with /// 18 decimal places, this effectively leaves a maximum allowable deposit amount of (10^20). /// While this is substantially large, it is crucial to be aware of this constraint. /// Transactions exceeding this limit will be reverted

This is not the case and the maximum depositValue I was able to test was `100_000_000_000_000_000_000e18`, significantly larger that `10e20`.

Attack Scenario\

    /// @notice Sell collateral for the sender's address. This function is subject
    /// to a transactional limit, determined by the issuing token's decimal precision and the underlying
    /// bonding curve algorithm.
    /// @dev Redirects to the internal function `_sellOrder` by passing the sender's address and deposit amount.
    /// Important: The Bancor Formula has an upper computational limit of (10^26). For tokens with
    /// 18 decimal places, this effectively leaves a maximum allowable deposit amount of (10^8), or
    /// 100,000,000. Transactions exceeding this limit will be reverted.
    /// @param _depositAmount The amount of issued token depoisited.
    /// @param _minAmountOut The minimum acceptable amount the user expects to receive from the transaction.
    /// @param _minAmountOut The minimum acceptable amount the user expects to receive from the transaction.
    function sell(uint _depositAmount, uint _minAmountOut)
        public
        virtual
        override(RedeemingBondingCurveBase_v1)
        sellingIsEnabled
    {
        sellFor(_msgSender(), _depositAmount, _minAmountOut);
    }

Attachments Add the following test to the E2E test suit which shows that a deposit value of 100_000_000_000_000_000_000e18 does not revert. Increasing this value by one decimal place will cause the test to revert.

1. Proof of Concept (PoC) File

   
   // SPDX-License-Identifier: LGPL-3.0-only
   pragma solidity ^0.8.0;

import "forge-std/console.sol";

// Internal Dependencies import { E2ETest, IOrchestratorFactory_v1, IOrchestrator_v1 } from "test/e2e/E2ETest.sol";

import {ERC20Issuance_v1} from "@fm/bondingCurve/tokens/ERC20Issuance_v1.sol";

// SuT import { FM_BC_Bancor_Redeeming_VirtualSupply_v1, IFM_BC_Bancor_Redeeming_VirtualSupply_v1 } from "test/modules/fundingManager/bondingCurve/FM_BC_Bancor_Redeeming_VirtualSupply_v1.t.sol"; import {IBondingCurveBase_v1} from "@fm/bondingCurve/interfaces/IBondingCurveBase_v1.sol";

contract BondingCurveFundingManagerE2E is E2ETest { // Module Configurations for the current E2E test. Should be filled during setUp() call. IOrchestratorFactory_v1.ModuleConfig[] moduleConfigurations;

ERC20Issuance_v1 issuanceToken;

address alice = address(0xA11CE);
uint aliceBuyAmount = 200e18;

address bob = address(0x606);
uint bobBuyAmount = 100_000_000_000_000_000_000e18;

function setUp() public override {
    // Setup common E2E framework
    super.setUp();

    // Set Up individual Modules the E2E test is going to use and store their configurations:
    // NOTE: It's important to store the module configurations in order, since _create_E2E_Orchestrator() will copy from the array.
    // The order should be:
    //      moduleConfigurations[0]  => FundingManager
    //      moduleConfigurations[1]  => Authorizer
    //      moduleConfigurations[2]  => PaymentProcessor
    //      moduleConfigurations[3:] => Additional Logic Modules

    // FundingManager
    setUpBancorVirtualSupplyBondingCurveFundingManager();

    // BancorFormula 'formula' is instantiated in the E2EModuleRegistry

    IBondingCurveBase_v1.IssuanceToken memory issuanceToken_properties =
    IBondingCurveBase_v1.IssuanceToken({
        name: "Bonding Curve Token",
        symbol: "BCT",
        decimals: 18,
        maxSupply: type(uint).max - 1
    });

    address issuanceTokenAdmin = address(this);

    IFM_BC_Bancor_Redeeming_VirtualSupply_v1.BondingCurveProperties memory
        bc_properties = IFM_BC_Bancor_Redeeming_VirtualSupply_v1
            .BondingCurveProperties({
            formula: address(formula),
            reserveRatioForBuying: 333_333,
            reserveRatioForSelling: 333_333,
            buyFee: 0,
            sellFee: 0,
            buyIsOpen: true,
            sellIsOpen: true,
            initialIssuanceSupply: 1,
            initialCollateralSupply: 3
        });

    moduleConfigurations.push(
        IOrchestratorFactory_v1.ModuleConfig(
            bancorVirtualSupplyBondingCurveFundingManagerMetadata,
            abi.encode(
                issuanceToken_properties,
                issuanceTokenAdmin,
                bc_properties,
                token
            )
        )
    );

    // Authorizer
    setUpRoleAuthorizer();
    moduleConfigurations.push(
        IOrchestratorFactory_v1.ModuleConfig(
            roleAuthorizerMetadata, abi.encode(address(this))
        )
    );

    // PaymentProcessor
    setUpSimplePaymentProcessor();
    moduleConfigurations.push(
        IOrchestratorFactory_v1.ModuleConfig(
            simplePaymentProcessorMetadata, bytes("")
        )
    );

    // Additional Logic Modules
    setUpBountyManager();
    moduleConfigurations.push(
        IOrchestratorFactory_v1.ModuleConfig(
            bountyManagerMetadata, bytes("")
        )
    );
}

function test_e2e_OrchestratorFundManagementPOC1() public {
    // address(this) creates a new orchestrator.
    IOrchestratorFactory_v1.WorkflowConfig memory workflowConfig =
    IOrchestratorFactory_v1.WorkflowConfig({
        independentUpdates: false,
        independentUpdateAdmin: address(0)
    });

    IOrchestrator_v1 orchestrator =
        _create_E2E_Orchestrator(workflowConfig, moduleConfigurations);

    FM_BC_Bancor_Redeeming_VirtualSupply_v1 fundingManager =
    FM_BC_Bancor_Redeeming_VirtualSupply_v1(
        address(orchestrator.fundingManager())
    );

    issuanceToken = ERC20Issuance_v1(fundingManager.getIssuanceToken());

    token.mint(alice, aliceBuyAmount);
    token.mint(bob, bobBuyAmount);

    uint buf_minAmountOut =
        fundingManager.calculatePurchaseReturn(aliceBuyAmount); // buffer variable to store the minimum amount out on calls to the buy and sell functions

    vm.startPrank(alice);
    {
        // Approve tokens to orchestrator.
        token.approve(address(fundingManager), aliceBuyAmount);

        // Deposit tokens, i.e. fund the fundingmanager.
        fundingManager.buy(aliceBuyAmount, buf_minAmountOut);

        // After the deposit, alice received some amount of receipt tokens
        // from the fundingmanager.
        assertTrue(issuanceToken.balanceOf(alice) > 0);
    }
    vm.stopPrank();

    buf_minAmountOut = fundingManager.calculatePurchaseReturn(bobBuyAmount);

    uint bobTokenBalanceBefore = token.balanceOf(bob);
    console.log("Bob token balance before: ", token.balanceOf(bob));

    // Bob performs a buy
    vm.startPrank(bob);
    {
        // Approve tokens to fundingmanager.
        token.approve(address(fundingManager), bobBuyAmount);

        // Deposit tokens, i.e. fund the fundingmanager.
        fundingManager.buy(bobBuyAmount, buf_minAmountOut);

        // After the deposit, bob received some amount of receipt tokens
        // from the fundingmanager.
        assertTrue(issuanceToken.balanceOf(bob) > 0);
    }
    vm.stopPrank();

    console.log(
        "token.balanceOf(address(fundingManager))",
        token.balanceOf(address(fundingManager))
    );

    vm.prank(address(this));
    fundingManager.setReserveRatioForSelling(331_333);

    buf_minAmountOut =
        fundingManager.calculateSaleReturn(issuanceToken.balanceOf(bob));

    // Bob Backruns Alice's buy
    vm.startPrank(bob);
    {
        // Approve tokens to fundingmanager.
        issuanceToken.approve(
            address(fundingManager), issuanceToken.balanceOf(bob)
        );

        fundingManager.sell(issuanceToken.balanceOf(bob), buf_minAmountOut);
        assertApproxEqRel(token.balanceOf(bob), bobBuyAmount, 0.00001e18); // ensures that the imprecision introduced by the math stays below 0.001%
    }
    vm.stopPrank();

    uint bobTokenBalanceAfter = token.balanceOf(bob);
    console.log("Bob token balance after: ", token.balanceOf(bob));
    // console.log(
    //     "Bob profit: ", bobTokenBalanceAfter - bobTokenBalanceBefore
    // );
}

}

2. **Revised Code File (Optional)**

[PlamenTSV]

Wrong comment, informational in my opinion. Will wait before labelling @0xmahdirostami

[Jonsey]

I put it as a low, because it could be very misleading