In the above code, the onlyInitializing modifier restricts this function so it can only be invoked within the execution of a function marked with initializer() or reinitializer(). However, note that this behavior also means that any function with these modifiers must carefully avoid giving up control-flow during execution. If control-flow is lost (e.g. through an external call), the public initialize() function could potentially be invoked by an attacker, which would overwrite the _directory storage variable.
In most derived contracts, this risk is mitigated because they override the initialize() function, which prevents the UpgradeableBase version of initialize() from being directly invoked. For example:
contract MerkleClaimStreamer is UpgradeableBase {
// ...
function initialize(address _directory) public override initializer {
super.initialize(_directory);
// ...
}
// ...
}
However, there are some contracts that do not override the initialize() function, leaving the UpgradeableBase implementation exposed. For example:
contract WETHVault is UpgradeableBase, ERC4626Upgradeable, IRateProvider {
// ...
function initializeVault(address directoryAddress, address weth) public virtual initializer {
super.initialize(directoryAddress);
// ...
}
// ...
}
Fortunately, in the current codebase, no contracts with exposed initialize() functions have initializer() or reinitializer() implementations that give up control-flow, so this behavior does not create any immediate vulnerabilities.
**Recommendation: Since making significant changes to this logic would impact critical parts of the code, it is recommended to keep this behavior in mind for now, while continuing to avoid relinquishing control-flow in initializer() and reinitializer() functions.
In a future upgrade, consider refactoring the UpgradeableBase implementation to rename the initialize() function and mark it as internal.**
In the current codebase, many contracts inherit from the UpgradeableBase abstract contract, which includes a public initialize() function:
In the above code, the onlyInitializing modifier restricts this function so it can only be invoked within the execution of a function marked with initializer() or reinitializer(). However, note that this behavior also means that any function with these modifiers must carefully avoid giving up control-flow during execution. If control-flow is lost (e.g. through an external call), the public initialize() function could potentially be invoked by an attacker, which would overwrite the _directory storage variable.
In most derived contracts, this risk is mitigated because they override the initialize() function, which prevents the UpgradeableBase version of initialize() from being directly invoked. For example:
However, there are some contracts that do not override the initialize() function, leaving the UpgradeableBase implementation exposed. For example:
Fortunately, in the current codebase, no contracts with exposed initialize() functions have initializer() or reinitializer() implementations that give up control-flow, so this behavior does not create any immediate vulnerabilities.
**Recommendation: Since making significant changes to this logic would impact critical parts of the code, it is recommended to keep this behavior in mind for now, while continuing to avoid relinquishing control-flow in initializer() and reinitializer() functions.
In a future upgrade, consider refactoring the UpgradeableBase implementation to rename the initialize() function and mark it as internal.**