Upgradeable contracts must disable initializers in the implementation contracts

[hats-bug-reporter[bot]]

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

Description: Description\

OpenZeppelin states:

Avoid leaving a contract uninitialized.

An uninitialized contract can be taken over by an attacker. This applies to both a proxy and its implementation contract, 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.

EthMultiVault.sol and AtomWallet.sol does not invoke the _disableInitializers function in their constructors so affected by this issue.

Recommendation\ Add the following code to the affected contracts:

+ constructor() {
+  _disableInitializers();
}

[mihailo-maksa]

After reviewing the reported issue, we have determined that the concern is not relevant to our implementation. Here's why:

1. Proxy Pattern Usage: Both EthMultiVault and AtomWallet contracts are designed to be used with the proxy pattern. In this pattern, the implementation contracts are never directly interacted with; instead, all interactions occur through the proxy. This ensures that the implementation contracts are not at risk of being taken over.
2. Controlled Deployment: The deployment process of our contracts is tightly controlled. The initialization function is called immediately upon deployment through our deployment scripts, ensuring that the contracts are never left in an uninitialized state.
3. No Practical Impact: Even if an implementation contract were somehow left uninitialized, it would not pose a practical risk because all interactions and state changes occur through the initialized proxy contracts. The implementation contracts do not hold any state or perform any actions on their own.
4. Mitigation Consideration: While we acknowledge OpenZeppelin's best practice recommendation, the specific architecture and deployment process of our system mitigate the potential risks identified. The suggested addition of _disableInitializers is not necessary given our controlled environment and usage patterns.

In conclusion, the reported issue does not apply to our specific use case and implementation. The proxy pattern and controlled deployment process we use ensure that our contracts are not vulnerable to the concerns raised. Therefore, we consider this issue to be invalid and not a threat to our system's security.

[0xRizwan]

@mihailo-maksa This issue highlights potential non-following of openzeppelin's upgradeable contracts usage as shared here.

While we acknowledge OpenZeppelin's best practice recommendation

Since, you have acknowledged this issue. If the issue is acknowledged then its considered valid. Fixing or not fixing the issue is upto protocol team. Since, the issue highlighted is correct and acknowledged then i think it should be considered as valid.