Direct orchestrator updates can cause many serious problems

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

Description:

Impact

Any normal Orchestrator_v1 proxy update can break functionality across modules and cause denial of service. In case the independent update admin is different from orchestrator admin takeover of orchestrator admin role and many loss of funds scenarios for users are possible.

Description

The orchestrator owner can set up any owner for their Orchestrator_v1 proxy at orchestrator initialization createOrchestrator(). While it's likely they set themselves up as the owner of proxy, it's not impossible that they have chosen an independent external actor (e.g. someone who builds custom Orchestrators).

The independent update admin can set any new implementation for Orchestrator_v1 through the InverterProxyAdmin_v1 at any time. This is very dangerous as many things can go wrong with updates as the proxy owner has the power to change Orchestrator to any arbitrary implementation:

can break all modules that rely on Orchestrator_v1 and cause denial of service
if updater is external actor a malicious upgrade can take over orchestrator access control and cause loss of fund acting as orchestrator admin
breaks core module restrictions in Orchestrator_v1
and many many more issues

OrchestratorFactory_v1.sol - createOrchestrator()

    function createOrchestrator(
        WorkflowConfig memory workflowConfig,
        ModuleConfig memory fundingManagerConfig,
        ModuleConfig memory authorizerConfig,
        ModuleConfig memory paymentProcessorConfig,
        ModuleConfig[] memory moduleConfigs
    ) external returns (IOrchestrator_v1) {
        address proxy;
        // If the workflow should fetch their updates themselves
        if (workflowConfig.independentUpdates) {
            // Deploy a proxy admin contract that owns the invidual proxies
            // Overwriting the independentUpdateAdmin as the ProxyAdmin will
            // be the actual admin of the proxy
            workflowConfig.independentUpdateAdmin = address(
                new InverterProxyAdmin_v1(workflowConfig.independentUpdateAdmin)
            );

            // Use an InverterTransparentUpgradeableProxy as a proxy
            proxy = address(
                new InverterTransparentUpgradeableProxy_v1(
                    beacon, workflowConfig.independentUpdateAdmin, bytes("")
                )
            );
        }
        // If not then
        else {
            // Instead use the Beacon Structure Proxy
            proxy = address(new InverterBeaconProxy_v1(beacon));
        }

        // Map orchestrator proxy
        _orchestrators[++_orchestratorIdCounter] = proxy;

        // Deploy and cache {IFundingManager_v1} module.
        address fundingManager = IModuleFactory_v1(moduleFactory).createModule(
            fundingManagerConfig.metadata,
            IOrchestrator_v1(proxy),
            fundingManagerConfig.configData,
            workflowConfig
        );

        // Deploy and cache {IAuthorizer_v1} module.
        address authorizer = IModuleFactory_v1(moduleFactory).createModule(
            authorizerConfig.metadata,
            IOrchestrator_v1(proxy),
            authorizerConfig.configData,
            workflowConfig
        );

        // Deploy and cache {IPaymentProcessor_v1} module.
        address paymentProcessor = IModuleFactory_v1(moduleFactory).createModule(
            paymentProcessorConfig.metadata,
            IOrchestrator_v1(proxy),
            paymentProcessorConfig.configData,
            workflowConfig
        );

        // Deploy and cache optional modules.
        address[] memory modules =
            createModules(moduleConfigs, proxy, workflowConfig);

        emit OrchestratorCreated(_orchestratorIdCounter, proxy);

        // Initialize orchestrator.
        IOrchestrator_v1(proxy).init(
            _orchestratorIdCounter,
            moduleFactory,
            modules,
            IFundingManager_v1(fundingManager),
            IAuthorizer_v1(authorizer),
            IPaymentProcessor_v1(paymentProcessor),
            IGovernor_v1(IModuleFactory_v1(moduleFactory).governor())
        );

        return IOrchestrator_v1(proxy);
    }

Proof of Concept

This scenario demonstrates how an Orchestrator_v1 can completely avoid governance fees from FeeManager_v1:

Orchestrator_v1 is initialized with an independent update admin as the orchestrator owner
Governance has always charged 20% fees for fundingManager buy and sell actions
Orchestrator owner upgrades the Orchestrator_v1 to an implementation that changes governor address to their own address
When fees are substracted in buyOrder() and sellOrder() the new governor implementation queries their "feeManager" that returns zero upon getIssuanceWorkflowFeeAndTreasury() and getCollateralWorkflowFeeAndTreasury() calls

Note that there are more serious issues, this is just one example.

Issues with independent updates

There are many possible serious issues if the control of a proxy is in the hands of an independent update admin and not the Governance. I demonstrated different scenarios with my other submissions. Note that a proxy owner doesn't have to be malicious to cause harm, it's likely they are breaking system wide functionality of modules with any regular update that changes Orchestrator_v1 or changes any Module_v1 implementation.

Recommendation

Consider to remove all independentUpdate proxies and functionality from Orchestrator_v1. Consider to completely disallow non-governance controlled proxies. While these changes would decrease decentralization, they marginally improve safety for users and orchestrator owners.

hats-finance / Inverter-Network-0xe47e52c4fea05e555920f1dcdcc6fb8eca103eeb