Any one can remove modules due to missing access control

[hats-bug-reporter[bot]]

Github username: @0xRizwan Twitter username: 0xRizwann Submission hash (on-chain): 0xc4f8788179bd932039b1964c451d9fb1003f1b053db6ef27d3bfc56276d0aec2 Severity: high

Description: Description\

Orchestrator_v1 contract is the center and connecting block of all modules in a Inverter Network Workflow. Modules like fundingManager, authorizer, paymentProcessor are set during initialization of Orchestrator_v1 contract, Further the Orchestrator_v1 contract allows to replace these modules via initiate and execute functions. The maximum modules the contract can handle is 128 and when it exceeds, no further modules can be added/replaced.

There is missing access control on below functions:

1) initiateAddModuleWithTimelock() 2) initiateRemoveModuleWithTimelock() 3) executeAddModule() 4) executeRemoveModule()

Some major issues can happen due to missing access control on above functions:

1) By calling initiateRemoveModuleWithTimelock() and executeRemoveModule(), an attacker/malicious user can remove the already added modules by admin. This is due to both of these functions are not restricted via access control and vice versa for addition of modules.

2) The functions, initiateAddModuleWithTimelock(), initiateRemoveModuleWithTimelock(), executeAddModule(), executeRemoveModule() actually deviates from Intended protocol design in terms of secure access control as defined in these functions Natspec which is implemented as:

    /// @notice Initiates the adding of a module to the Orchestrator on a timelock
@>    /// @dev Only callable by authorized address.
    /// @dev Fails of adding module exeeds max modules limit
    /// @dev Fails if address invalid or address already added as module.
    /// @param module The module address to add.
    function initiateAddModuleWithTimelock(address module) external;

    /// @notice Initiate the removal of a module from the Orchestrator on a timelock
    /// @dev Reverts if module to be removed is the current authorizer/fundingManager/paymentProcessor.
    ///         The functions specific to updating these 3 module categories should be used instead
@>    /// @dev Only callable by authorized address.
    /// @dev Fails if address not added as module.
    function initiateRemoveModuleWithTimelock(address module) external;

    /// @notice Adds address `module` as module.
@>    /// @dev Only callable by authorized address.
    /// @dev Fails if adding of module has not been initiated.
    /// @dev Fails if timelock has not been expired yet.
    /// @param module The module address to add.
    function executeAddModule(address module) external;

    /// @notice Removes address `module` as module.
@>    /// @dev Only callable by authorized address.
    /// @dev Fails if removing of module has not been initiated.
    /// @dev Fails if timelock has not been expired yet.
    /// @param module The module address to remove.
    function executeRemoveModule(address module) external;

Therefore, to prevent accessing of these critical functions and as per Natspec of these function, An access control modifier must be implemented to prevent it from malicious users.

Impact\ Anyonce can call the above listed 4 functions and can make the changes due to missing access control, Also these functions are violating the intended design of access control stated in their Natspec

Recommendation to fix\ As stated in Natspec, Following functions must be called by Authorized address only.

OR, i believe, would be onlyOrchestratorAdmin. This is further confirmed from this line of contract.

Consider below changes:

-    function initiateAddModuleWithTimelock(address module_) external {
+     function initiateAddModuleWithTimelock(address module_) external onlyOrchestratorAdmin {
        _initiateAddModuleWithTimelock(module_);
    }

    /// @inheritdoc IOrchestrator_v1
    function initiateRemoveModuleWithTimelock(address module_)
        external
+      onlyOrchestratorAdmin
        onlyLogicModules(module_)
    {
        _initiateRemoveModuleWithTimelock(module_);
    }

    /// @inheritdoc IOrchestrator_v1
-    function executeAddModule(address module_) external {
+     function executeAddModule(address module_) external  onlyOrchestratorAdmin
 {
        _executeAddModule(module_);
    }

    /// @inheritdoc IOrchestrator_v1
    function executeRemoveModule(address module_)
        external
+     onlyOrchestratorAdmin
        onlyLogicModules(module_)
    {
        _executeRemoveModule(module_);
    }

[0xfuje]

Hey, just to note that the functions you listed all have __ModuleManager_onlyAuthorized() check in ModuleManagerBase_v1 contract that eventually checks if the caller has the highest admin role in the system, so only the admin can execute these.

    function __ModuleManager_isAuthorized(address who)
        internal
        view
        override(ModuleManagerBase_v1)
        returns (bool)
    {
        return authorizer.hasRole(authorizer.getAdminRole(), who);
    }

[PlamenTSV]

Hey, just to note that the functions you listed all have __ModuleManager_onlyAuthorized() check in ModuleManagerBase_v1 contract that eventually checks if the caller has the highest admin role in the system, so only the admin can execute these.

    function __ModuleManager_isAuthorized(address who)
        internal
        view
        override(ModuleManagerBase_v1)
        returns (bool)
    {
        return authorizer.hasRole(authorizer.getAdminRole(), who);
    }

Thanks @0xfuje !