Omniscia 0xPhase Audit

VaultInitializer Manual Review Findings

VaultInitializer Manual Review Findings

VIR-01M: Improper Initializer Definition

TypeSeverityLocation
Standard ConformityVaultInitializer.sol:L87, L103

Description:

The VaultInitializer::initializeVaultOwner function appears to be incorrect as it will invoke a transfer of ownership after the original one performed in VaultInitializer::initializeVaultV1 as well as disable initialization, meaning that it must be invoked after VaultInitializer::initializeVaultV1.

Example:

vault/diamond/VaultInitializer.sol
27/// @notice Initializes the Vault contract on version v1
28/// @param db_ The db contract
29/// @param varStorage_ The var Ssorage contract
30/// @param asset_ The asset token contract
31/// @param priceOracle_ The price oracle contract
32/// @param interest_ The interest contract
33/// @param initialMaxMint_ The initial max mint
34/// @param initialMaxCollateralRatio_ The initial max collateral ratio
35/// @param initialBorrowFee_ The initial borrow fee
36/// @param initialLiquidationFee_ The initial liquidation fee
37/// @param initialHealthTargetMinimum_ The initial health target minimum
38/// @param initialHealthTargetMaximum_ The initial health target maximum
39/// @param adapter_ The optional adapter address
40/// @param adapterData_ The optional adapter data
41function initializeVaultV1(
42 IDB db_,
43 Storage varStorage_,
44 IERC20 asset_,
45 IOracle priceOracle_,
46 IInterest interest_,
47 uint256 initialMaxMint_,
48 uint256 initialMaxCollateralRatio_,
49 uint256 initialBorrowFee_,
50 uint256 initialLiquidationFee_,
51 uint256 initialHealthTargetMinimum_,
52 uint256 initialHealthTargetMaximum_,
53 address adapter_,
54 bytes memory adapterData_
55) external initialize("v1") {
56 _initializeElement(db_);
57
58 address managerAddress = db_.getAddress("MANAGER");
59
60 _s.varStorage = varStorage_;
61 _s.asset = asset_;
62 _s.priceOracle = priceOracle_;
63 _s.interest = interest_;
64
65 _s.systemClock = ISystemClock(db_.getAddress("SYSTEM_CLOCK"));
66 _s.manager = Manager(managerAddress);
67 _s.creditAccount = ICreditAccount(db_.getAddress("CREDIT_ACCOUNT"));
68 _s.cash = IPegToken(db_.getAddress("CASH"));
69 _s.treasury = ITreasury(db_.getAddress("TREASURY"));
70 _s.bond = IBond(db_.getAddress("BOND"));
71 _s.balancer = IBalancer(db_.getAddress("BALANCER"));
72
73 _s.maxMint = initialMaxMint_;
74 _s.maxCollateralRatio = initialMaxCollateralRatio_;
75 _s.borrowFee = initialBorrowFee_;
76 _s.liquidationFee = initialLiquidationFee_;
77 _s.healthTargetMinimum = initialHealthTargetMinimum_;
78 _s.healthTargetMaximum = initialHealthTargetMaximum_;
79
80 _s.adapter = adapter_;
81 _s.adapterData = adapterData_;
82
83 _s.lastDebtUpdate = _s.systemClock.time();
84
85 _grantRoleKey(VaultConstants.MANAGER_ROLE, keccak256("MANAGER"));
86 _grantRoleKey(VaultConstants.DEV_ROLE, keccak256("DEV"));
87 _transferOwnership(managerAddress);
88
89 emit PriceOracleSet(priceOracle_);
90 emit InterestSet(interest_);
91 emit MaxCollateralRatioSet(initialMaxCollateralRatio_);
92 emit BorrowFeeSet(initialBorrowFee_);
93 emit LiquidationFeeSet(initialLiquidationFee_);
94 emit HealthTargetMinimumSet(initialHealthTargetMinimum_);
95 emit HealthTargetMaximumSet(initialHealthTargetMaximum_);
96 emit AdapterSet(adapter_);
97 emit AdapterDataSet(adapterData_);
98}
99
100/// @notice Initializes the target diamond to allow for cutting
101/// @param owner The diamond owner
102function initializeVaultOwner(address owner) public initialize("owner") {
103 _transferOwnership(owner);
104 _disableInitialization();
105}

Recommendation:

We advise the code to be revised and the second initialization methodology to be potentially omitted as it is presently possible to invoke the initialization of the vault owner after the initialization of the vault to take over it.

Additionally, two initializers instead of one is inherently incompatible with the Diamond system as it is expected to invoke only one, opening the contract up to on-chain race conditions.

Alleviation (3dd3d7bf0c2693b2f9c23bacedfa420393f7ea84):

The 0xPhase team evaluated this exhibit and has stated that they wish to retain the current structure in place to save on gas costs. As such, we consider this exhibit acknowledged.