0xKartikgiri00 - Not Calling _disableInitializers() function in `ClonableBeaconProxy` contract will lead to exploit.

[sherlock-admin4]

0xKartikgiri00

medium

Not Calling _disableInitializers() function in ClonableBeaconProxy contract will lead to exploit.

Summary

The ClonableBeaconProxy contract is missing a call to the _disableInitializers() function in the constructor of the, leaving the proxy vulnerable to potential reinitialization attacks.

Vulnerability Detail

The initialize function should be called only once by the proxy to initialize the contract's state. OpenZeppelin's Initializable contract provides the _disableInitializers() function, which disables any subsequent calls to the initializer functions of the implementation contract after it has been initialized by the proxy. However, this function is not being called in the constructor of the ClonableBeaconProxy contract, leaving it vulnerable to reinitialization attacks.

As it is stated in the OpenZeppelin's Initializable contract. "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"

Impact

The not calling of _disableInitializers() in the constructor of the ClonableBeaconProxy contract allows potential attackers to exploit the proxy by reinitializing the beacon.

Code Snippet

https://github.com/sherlock-audit/2024-02-telcoin-platform-audit-update/blob/main/telcoin-contracts/contracts/external/openzeppelin/ClonableBeaconProxy.sol#L39

Tool used

Manual Review

Recommendation

The recommended mitigation is to call _disableInitializers() function in the constructor of the ClonableBeaconProxy contract . This will prevent the initialization function from being called more than once, safeguarding the proxy against reinitialization attacks.

Recommendation in code:-

 /// @custom:oz-upgrades-unsafe-allow constructor
  constructor() {
      _disableInitializers();
  }

[sherlock-admin3]

3 comment(s) were left on this issue during the judging contest.

WangAudit commented:

as Watson said themselves; _disableInitializers should be called in implementation contract; not the proxy and initializer modifier solves the problems

takarez commented:

invalid

takarez commented:

invalid; according to the sherkock's rule VII. Num 7 this should be invalid because admin can just redeploy

[kartik-giri]

" because admin can just redeploy" is this necessary to have a exploit at a first place than. Where we can just stop this by implementing _disableInitializers() function in contract which locked the initialization from the implementation contract.

And one more thing the initilaize() function can be called from implementation contract even when it has been deployed if we don't implement _disableInitializers().