Anyone can initialize `OrchestratorFactory_v1.sol` and `ModuleFactory_v1.sol` implementation contracts

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0xfe25a0c906257e4d19f35ede55b581b83d2a1dab4db90ac651fdcffa0cae5626 Severity: low

Description:

Description

OrchestratorFactory_v1.sol and ModuleFactory.sol are missing _disableInitializers() call in the constructor. An implementation contract can be taken over by an attacker with calling the init function which may cause unexpected functionality (it is very dangerous with delegatecalls).

Recommendation

From openzeppelin's documentation:

Do not leave an implementation contract uninitialized. An uninitialized implementation contract can be taken over by an attacker, which may impact the proxy. To prevent the implementation contract from being used, you should invoke the _disableInitializers function in the constructor to automatically lock it when it is deployed

[PlamenTSV]

selfdestruct() was removed, an initialized implementation is just a contract that's unusable. Since it cannot be destroyed to brick the proxy, this is a non-issue in my opinion.

[0xfuje]

selfdestruct() was removed, an initialized implementation is just a contract that's unusable. Since it cannot be destroyed to brick the proxy, this is a non-issue in my opinion.

selfdestruct()'s behaviour was changed, but only in compiler versions equal or above to 0.8.24: Solidity 0.8.24 announcement, and since the contracts use 0.8.23 the selfdestruct() proxy attack may still be viable. Note that there can be other potential issues if someone initializes the implementation contract.

[PlamenTSV]

PoC

[0xmahdirostami]

no issue here, but yes, security best practice says that it would be to call _disableInitializers in the constructor.