BasicVaultFactory Manual Review Findings

BVF-01M: Inexistent Initialization Protection of Base Implementation

Type	Severity	Location
Language Specific		BasicVaultFactory.sol:L41, L44

Description:

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

Example:

Copy
41contract BasicVaultFactory is IVaultFactory, OwnableUpgradeable, BasicVaultFactoryStorageV1 {
42    event VaultDeployed(address indexed instance, uint256 indexed id);
43
44    function initialize(address owner, address hub) public initializer {
45        __Ownable_init();
46        _transferOwnership(owner);
47
48        StorageV1 storage $ = _getStorageV1();
49
50        address initialVaultImpl = address(new BasicVault());
51        $._beacon = new UpgradeableBeacon(initialVaultImpl);
52        $._beacon.transferOwnership(owner);
53        $._hub = hub;
54    }

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 BasicVaultFactory::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.

BVF-02M: Insufficient Prevention of Vault Deployment

Type	Severity	Location
Logical Fault		BasicVaultFactory.sol:L113

Description:

The BasicVaultFactory::createVault function is meant to prevent the same vault to be deployed via the create2 deployment mechanism, however, it is still possible to re-deploy the same asset, name, and symbol combination for a vault by changing the OwnableUpgradeable::owner of the BasicVaultFactory.

Impact:

If the BasicVaultFactory owner is expected to change, a vault with an identical configuration will be deployable which we consider invalid.

Example:

Copy
138function _calcDeployMetadata(address asset, string memory name, string memory symbol)
139    internal
140    view
141    returns (bytes32, bytes memory)
142{
143    bytes32 salt = keccak256(abi.encode(asset, name, symbol));
144
145    // slither-disable-next-line too-many-digits
146    bytes memory bytecode = abi.encodePacked(
147        type(BeaconProxy).creationCode,
148        abi.encode(
149            _getStorageV1()._beacon,
150            abi.encodeWithSelector(BasicVault.initialize.selector, owner(), asset, name, symbol)
151        )
152    );
153
154    return (salt, bytecode);
155}

Recommendation:

We advise this trait to be evaluated, and an alternative mechanism to be implemented for initializing the owner of the BasicVault that does not affect its deployed address.

Alleviation (58e8cc66dfa900c03c47df78f5170d9960005629):

The OwnableUpgradeable::owner of the BasicVaultFactory is no longer part of the create2 deployment mechanism with ownership being transferred immediately after the contract has been deployed.

Therefore, the create2 mechanism is no longer influenced by the variable alleviating this exhibit.