VaultHub Manual Review Findings

VHB-01M: Inexistent Initialization Protection of Base Implementation

Type	Severity	Location
Language Specific		VaultHub.sol:L28

Description:

The contract is meant to be upgradeable yet does not properly protect its logic deployment from malicious initializations.

Example:

Copy
28contract VaultHub is OwnableUpgradeable, VaultHubStorageV1 {
29    function initialize(address owner) public initializer {
30        __Ownable_init();
31        _transferOwnership(owner);
32    }

Recommendation:

We advise a constructor to be introduced that either invokes the initializer modifier of the Initializable contract or invokes the Initializable::_disableInitializers function to prevent the base implementation from ever being initialized.

Alleviation (58e8cc66dfa900c03c47df78f5170d9960005629):

A VaultHub::constructor has been introduced invoking the Initializable::initialize modifier thereby preventing re-initializations as long as the contract does not utilize a versioned initialization system.

If such a system is expected, we advise the Initializable::_disableInitializers function instead.

VHB-02M: Potentially Dangerous Invocation

Type	Severity	Location
Language Specific		VaultHub.sol:L39

Description:

The VaultHub::isVault function will invoke the IVault::vaultType getter function on its vault input argument, permitting a malicious contract to yield any value it wishes.

Impact:

While the referenced code is presently safe, a future upgrade of the system may misconceive that it is safe to invoke getter functions on input arguments supplied by users.

Example:

Copy
38function isVault(address vault) public view returns (bool) {
39    return IVaultFactory(factory(IVault(vault).vaultType())).isVault(vault);
40}

Recommendation:

While it is presently not a security vulnerability as the yielded value is used as a key to a factory implementation that is further validated, we still advise the call to an untrusted address to be avoided.

This can be achieved by looping through the VaultType values supported in the system one-by-one until the vault is either found or until all factories have been checked.

Alleviation (5297bb74fa):

The code was adjusted to perform a try-catch operation, however, this is still insecure as the getter call is still performed on the input argument.

We advise the Mitosis team to revisit this exhibit.

Alleviation (58e8cc66df):

The code was updated per our original recommendation, iterating through all vault types and evaluating whether the input vault is part of any in a sequential manner.

We consider this approach secure and thus the exhibit fully alleviated.