Any module can drain the `FundingManager` of all funding tokens

[hats-bug-reporter[bot]]

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

Description:

Impact

Massive loss of user / orchestrator funds: attacker can transfer every FundingManager orchestrator token to themselves

Description

This vulnerability arises with a combination of two weaknesses of the system.

1. Every module has the same permission to call as the orchestrator owner

The Orchestrator_v1 contract inherits ModuleManagerBase_v1. The owner of the orchestrator contract has the power to make any calls to any contracts via the executeTx function.

Orchestrator_v1 - executeTx()

    function executeTx(address target, bytes memory data)
        external
        onlyOrchestratorOwner
        returns (bytes memory)
    {
        bool ok;
        bytes memory returnData;
        (ok, returnData) = target.call(data);

        if (ok) {
            return returnData;
        } else {
            revert Orchestrator__ExecuteTxFailed();
        }
    }

However, any module has the same permissions as well via using executeTxFromModule(), which should not be the case. This means that any module can pose as the orchestrator address as the msg.sender.

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

2. onlyOrchestrator in fundingManager uses vulnerable access control

Usually onlyOrchestratorOwner() access control is used by contracts, which checks if the msg.sender has the owner role in the authorizer contract. However the inherited onlyOrchestrator() of funding manager only checks if the msg.sender is the orchestrator.

Module_v1 - onlyOrchestrator() -> _onlyOrchestratorModifier()

    function _onlyOrchestratorModifier() internal view {
        if (_msgSender() != address(__Module_orchestrator)) {
            revert Module__OnlyCallableByOrchestrator();
        }
    }

The Exploit

The only function that uses the onlyOrchestrator() modifier is the funding manager contract's transferOrchestratorToken() function. Any module can exploit this and via calling executeTxFromModule() -> fundingManager.transferOchestratorToken() because the transaction will come from the orchestrator address, therefore bypass the access control.

Proof of Concept

1. navigate to test/modules/fundingManager/rebasing/FM_Rebasing_v1.t.sol
2. copy and paste the below proof of concept

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

   function testUnauthorizedTransfer() public {
       address haxor = vm.addr(420);
       _token.mint(address(fundingManager), 100_000);
   
       assertEq(_token.balanceOf(haxor), 0);
   
       // any random module works, for ease of use we make use of an existing one
       vm.prank(address(_authorizer));
       _orchestrator.executeTxFromModule(
           address(fundingManager),
           abi.encodeWithSelector(
               IFundingManager_v1.transferOrchestratorToken.selector,
               haxor,
               100_000
           )
       );
   
       assertEq(_token.balanceOf(haxor), 100_000);
   }

Recommendation

Consider to use the onlyOrchestratorOwner() modifier on transferOrchestratorToken() and to delete onlyOrchestrator() to avoid similar vulnerabilities in the future. I will provide additional recommendation in comments about the use of executeTxFromModule().

[PlamenTSV]

The modules are contracts. Inside the test you prank to the address of the authorizer in order to run the attack, but in a real scenario, how would you execute this tx from a regular LM or any of the 3 special modules, when they have specific interfaces?

[0xfuje]

True, the 3 special modules can't execute this, however executeTxFromModule() can be used from any module, so in a real life scenario a custom module added to the orchestrator (that is either upgradeable to or currently have a function with arbitrary execution that calls executeTxFromModule()).

Another possible scenario is that the voters from AUT_EXT_VotingRoles_v1 can call executeMotion() to call executeTxFromModule() to exploit this.

Let me know if you need more info or have more questions about this!

[PlamenTSV]

An owner adding a logic module allowing such authorized access should be completely aware of the risks he imposes. Not sure here since it targets the Orchestrators authority over such functionality being added, so will leave the issue for now.

[0xfuje]

An owner adding a logic module allowing such authorized access should be completely aware of the risks he imposes. Not sure here since it targets the Orchestrators authority over such functionality being added, so will leave the issue for now.

Highly disagree, since the owner wouldn't know he permits such authorized access. It's a vulnerability in the system itself, not the orchestrator owner's decision. The orchestrator owner has no way of knowing this attack path.

[0xfuje]

The orchestrator owner just simply adds a module to the system, there is no reason to think for them the module has the same permission as them.

[0xfuje]

Coming back to this I can kind of see your point @PlamenTSV, however I don't think the orchestrator owner can be aware of this particular attack path. I promised to add recommendation for executeTxFromModule() in my submission.

Recommendation

In my opinion it's quite dangerous to let all modules execute actions from Orchestrator_v1. Consider to restrict the function with access control (via Authorizer) so that the orchestrator owner can grant permission if a module needs to call executeTxFromModule(). Instead of my initial recommendation this also needs less changes in payment based modules.

[PlamenTSV]

Discussion in #71 I see your point. For this to be valid, my #71 extends on this since it is exploitable with the edge-case Voter issue. Will wait for further comments.

[PlamenTSV]

My bad for not clarifying.

The valid attack path here is a combination:

1. If a 0 threshold voter passes a vote he can execute a transaction from the Voter module
2. From here he goes inside the Orchestrator and does executeTxFromModule
3. executeTxFromModule goes inside transferOrchestratorToken and transfers any amount out of the funding manager.

[NicolaMirchev]

I disagree with the severity of the following issues. Both relies on trusted sources. Currently there is not danger for the system.

1. Modules are mainly developed and deployed by Inverter team and it is recommended to use ModuleFactory, which introduce a security layer. I think that owner adding arbitrary modules is OOS, such as owner acting malicious and stealing all users funds from his modules.

2. Having a voting mechanism with clear constraints such as threshold and votes and data to be executed is enough to consider "Malicious proposals can win voting" OOS. It is community responsibility to protect their interests and see what the majority wants. Otherwise we should report to any DAO "Malicious vote to withdraw all treasury funds could pass and drain the protocol"

[PlamenTSV]

I disagree with the severity of the following issues. Both relies on trusted sources. Currently there is not danger for the system.

1. Modules are mainly developed and deployed by Inverter team and it is recommended to use ModuleFactory, which introduce a security layer. I think that owner adding arbitrary modules is OOS, such as owner acting malicious and stealing all users funds from his modules.
2. Having a voting mechanism with clear constraints such as threshold and votes and data to be executed is enough to consider "Malicious proposals can win voting" OOS. It is community responsibility to protect their interests and see what the majority wants. Otherwise we should report to any DAO "Malicious vote to withdraw all treasury funds could pass and drain the protocol"

The code allows for the voting threshold to be 1 or 0 intentionally. However this case opens up possibility for a single malicious votes to drain the FM, through calling the Orchestrator and invoking a transaction from it's address. Voters are not trusted entities as they are added and removed by other votes accordingly.

You are right that custom modules are dangerous and OOS, I have commented on that. It is also inappropriate to assume all voters will dedicate such an attack. However the above edge-case is valid. Severity can be changed by the sponsor, my decision is not the final one.