Initiating a new authorizer makes every module lose their roles

[hats-bug-reporter[bot]]

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

Description:

Impact

Temporary or permanent denial of service, breaks functionality of permissioned module roles across orchestrator system

Description

The whole orchestrator system is role-based where many modules can have different roles. The problem is when executing a new Authorizer set these modules will lose their respective roles since every module and the orchestrator queries Authorizer for onlyModuleRole() and onlyOrchestratorAdmin().

Orchestrator_v1.sol - initiateSetAuthorizerWithTimelock()

    function initiateSetAuthorizerWithTimelock(IAuthorizer_v1 authorizer_)
        external
        onlyOrchestratorAdmin
    {
        address authorizerContract = address(authorizer_);
        bytes4 authorizerInterfaceId = type(IAuthorizer_v1).interfaceId;

        _enforcePrivilegedModuleInterfaceCheck(
            authorizerContract, authorizerInterfaceId
        );

        _initiateAddModuleWithTimelock(authorizerContract);
        _initiateRemoveModuleWithTimelock(address(authorizer));
    }

Proof of Concept

Orchestrator role

This is less likely, but certainly possible without safeguards.

1. New authorizer is initiated and executed without the core role for the current orchestrator admin
2. This makes every onlyOrchestratorOwner() restricted functions permanently unaccessible
3. If module roles are also not set before executing the Authorizer update the modules will lose their roles permanently
4. This will cause permanent system wide damage including permanent denial of service and loss of funds (e.g. no one can transfer orchestrator funds out from fundingManager, etc.)

Module roles

1. New authorizer is initiated and executed with the core role, however module roles are not initiated
2. This makes the permissioned actors of modules lose their access control in LM_PC_Bounties_v1, LM_PC_KPIRewarder_v1, LM_PC_PaymentRouter_v1 and more contracts
3. This causes temporary denial of service since these roles can't execute their functions until the module roles are restored by the owner

Recommendation

Note: The safeguards can be added to executeSetAuthorizer() instead of initiateSetAuthorizerWithTimelock()

No complexity solution

Consider to add disclaimers to both initiateSetAuthorizerWithTimelock() and to the AUT_Roles_v1 contract that warn the orchestrator to set up every role before initiating a new authorizer instance.

Less complexity, higher safety solution

Consider to enforce initiateSetAuthorizerWithTimelock() to at least query the main role DEFAULT_ADMIN_ROLE of orchestrator owner and enforce it is the same address with the new authorizer. Note that the orchestrator owner can transfer ownership later if needed.

Highest complexity, highest safety solution

If additional safety is needed consider to enforce every role to remain the same in initiateSetAuthorizerWithTimelock().

[PlamenTSV]

A module's permission can be set before it is added. Thus the new Authorizer would already have the roles ready for when the change occurs. That's how I see it imo

[FHieser]

We actually added a disclaimer in the IOrchestrator interface for this