[H-02] Upgrading process uses initialize instead of reinitialize modifier

[Al-Qa-qa]

Description

The VaultManager is an upgradable contract and it is live on mainnet now. when first deploying the contract, we call initialize(), which is like constructor() for upgradable contracts. The issue lies is that the deployment script recalls initialize once again, which is like deploying the contract again, instead of upgrading it.

VaultManagerV3.sol#L51

  function initialize( ... ) 
    public 
 @>     initializer 
  { ... }

The initializer modifier, implements a mechanism like constructor(), where it can only be called once, ref.

Initializable.sol#L117-L122

        bool initialSetup = initialized == 0 && isTopLevelCall; // @audit checks that the function is executed for the first time
        bool construction = initialized == 1 && address(this).code.length == 0; // @audit prevent reentrancy in construction

        if (!initialSetup && !construction) {
            revert InvalidInitialization();
        }

The deployment script is done by calling initialize() function again which contains initialize modifier. which will make the call reverts, because the value of initialized != zero.

DeployVaultManagerV3.s.sol#L22-L27

    Upgrades.upgradeProxy(
      MAINNET_V2_VAULT_MANAGER,
      "VaultManagerV3.sol",
      abi.encodeCall(
@>      VaultManagerV3.initialize,
        ( ... )
      )
    );

For the severity of the issue, I am nearly sure that the old deployment process would fail totally, which makes the issue HIGH severity issue. But to be sure, the Sponser kindly will check if the old implementation upgrading would fail or not (upgrading an already live contract).

• If it does not revert, I will downguard the severity to MEDIUM. and check how it even passed, as it should not IMO.
• If it reverts, then it will be the thing that should occur, and the severity will be HIGH.

Recommended Mitigation

Change initialize modifier to reinitialize modifier, and pass the new version, which is 2 in that case, as the old version is 1.

VaultManagerV3.sol

function initialize( ... ) 
public 
-     initializer
+     reinitializer(2) 
{ ... }

[shafu0x]

should be fixed by this https://github.com/DyadStablecoin/contracts/pull/52/files

[Al-Qa-qa]

Mitigation introduced another issue, see M-03