Direct module updates are very dangerous

[hats-bug-reporter[bot]]

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

Description:

Impact

Total loss of user funds in any module contracts that currently hold funds or users used to approve funds to. Since anyone can be a module owner that exploits this and the initial module seems safe it's high severity. Non-malicious updates can also break functionality.

Description

Anyone can create a module and be it's owner via createModule() by setting workflowConfig.independentUpdates to be true. It's initialized as a InverterTransparentUpgradeableProxy_v1 and the module owner has full control of updating the proxy.

ModuleFactory_v1 - createModule()

    function createModule(
        IModule_v1.Metadata memory metadata,
        IOrchestrator_v1 orchestrator,
        bytes memory configData,
        IOrchestratorFactory_v1.WorkflowConfig memory workflowConfig
    ) external returns (address) {
        // Note that the metadata's validity is not checked because the
        // module's `init()` function does it anyway.

        IInverterBeacon_v1 beacon;
        (beacon, /*id*/ ) = getBeaconAndId(metadata);

        if (address(beacon) == address(0)) {
            revert ModuleFactory__UnregisteredMetadata();
        }

        address proxy;
        // If the workflow should fetch their updates themselves
        if (workflowConfig.independentUpdates) {
            // 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));
        }

        IModule_v1(proxy).init(orchestrator, metadata, configData);

        _orchestratorOfProxy[proxy] = address(orchestrator);

        emit ModuleCreated(
            address(orchestrator), proxy, LibMetadata.identifier(metadata)
        );

        return proxy;
    }

Proof of Concept

1. LM_PC_Staking_v1 contract is initialized with createModule() as an InverterTransparentUpgradeableProxy_v1, meaning the module creator is the independent update admin
2. Upon careful inspection by the orchestrator owner, since the module contract is completely safe: the module is added to the orchestrator system in Orchestrator_v1
3. Numerous users use the contract to stake() their funds
4. The update admin decides to steal all of the staked funds via updating the implementation contract so he can send all staked funds to himself

Note that the orchestrator owner could also exploit this as he can be the module owner, but this is an independent external actor in this case.

Recommendation

While this functionality for module proxies to have independent admins would add decentralization to the system, it is quite dangerous as it's easily exploitable. Consider to completely disallow independentUpdates of modules.

[PlamenTSV]

Seems the same as #92, but with a different actor. These are both issues regarding the independent admin, which should be a trusted entity. He does not abuse functionality he was not granted to begin with. It's like the owner losing his private key imo, will wait for another comment

[0xmahdirostami]

Just like previous issue, the issue is invalid, using InverterTransparentUpgradeableProxy_v1 allows the admin to catch the last implementation address which means that before each update, admin could take some time to decide to update to the updated address or not. (not anything more than that) https://github.com/hats-finance/Inverter-Network-0xe47e52c4fea05e555920f1dcdcc6fb8eca103eeb/blob/62892384fd7d0ce4d0e389c530200c69921473f7/src/proxies/InverterTransparentUpgradeableProxy_v1.sol#L116