Omniscia 0xPhase Audit
VaultInitializer Manual Review Findings
VaultInitializer Manual Review Findings
VIR-01M: Improper Initializer Definition
Type | Severity | Location |
---|---|---|
Standard Conformity | VaultInitializer.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:
27/// @notice Initializes the Vault contract on version v128/// @param db_ The db contract29/// @param varStorage_ The var Ssorage contract30/// @param asset_ The asset token contract31/// @param priceOracle_ The price oracle contract32/// @param interest_ The interest contract33/// @param initialMaxMint_ The initial max mint34/// @param initialMaxCollateralRatio_ The initial max collateral ratio35/// @param initialBorrowFee_ The initial borrow fee36/// @param initialLiquidationFee_ The initial liquidation fee37/// @param initialHealthTargetMinimum_ The initial health target minimum38/// @param initialHealthTargetMaximum_ The initial health target maximum39/// @param adapter_ The optional adapter address40/// @param adapterData_ The optional adapter data41function 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 cutting101/// @param owner The diamond owner102function 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.