thisvishalsingh - Update `initializer` modifier to `onlyInitializing` modifier to prevent reentrancy risk during initialization

[sherlock-admin]

thisvishalsingh

medium

Update initializer modifier to onlyInitializing modifier to prevent reentrancy risk during initialization

thisvishalsingh

Update initializer modifier to onlyInitializing modifier to prevent reentrancy risk during initialization

High

Summary

If the contract is supposed to be inherited by other contracts, onlyInitializing modifier MUST be used instead of initializer.

Vulnerability Detail

It is no longer possible to call an initializer-protected function from within another initializer function outside the context of a constructor. Project using OpenZeppelin upgradeable proxies should continue to work as is, since in the common case the initializer is invoked in the constructor directly. If this is not the case for you, the suggested change is to use the new onlyInitializing modifier in the following way:

contract A {
-  (-)  function initialize() public   initializer { ... }
-  (+)  function initialize() internal onlyInitializing { ... }
 }
 contract B is A {
     function initialize() public initializer {
         A.initialize();
     }
 }

initializer can only be called once, it can not be called once after every upgrade. When the concrete contract's initializer function (with a initializer modifier) is called by B initializer function, it will be mistook as Reentered and so that it will be reverted (unless in the context of a constructor).

See this reference: https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/v5.0.0/contracts/proxy/utils/Initializable.sol#L94-103

 /**
     * @dev A modifier that defines a protected initializer function that can be invoked at most once. In its scope,
     * `onlyInitializing` functions can be used to initialize parent contracts.
     *
     * Similar to `reinitializer(1)`, except that in the context of a constructor an `initializer` may be invoked any
     * number of times. This behavior in the constructor can be useful during testing and is not expected to be used in
     * production.
     *
     * Emits an {Initialized} event.
     */
    modifier initializer() {
        // solhint-disable-next-line var-name-mixedcase
        InitializableStorage storage $ = _getInitializableStorage();

        // Cache values to avoid duplicated sloads
        bool isTopLevelCall = !$._initializing;
        uint64 initialized = $._initialized;

        // Allowed calls:
        // - initialSetup: the contract is not in the initializing state and no previous version was
        //                 initialized
        // - construction: the contract is initialized at version 1 (no reininitialization) and the
        //                 current contract is just being deployed
        bool initialSetup = initialized == 0 && isTopLevelCall;
        bool construction = initialized == 1 && address(this).code.length == 0;

        if (!initialSetup && !construction) {
            revert InvalidInitialization();
        }
        $._initialized = 1;
        if (isTopLevelCall) {
            $._initializing = true;
        }
        _;
        if (isTopLevelCall) {
            $._initializing = false;
            emit Initialized(1);
        }
    }

Impact

HIGH Reentrancy risk during initialization

Code Snippet

https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/main/rio-sherlock-audit/contracts/restaking/RioLRT.sol#L40

function initialize(address initialOwner, string memory name, string memory symbol) external initializer {

https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/main/rio-sherlock-audit/contracts/restaking/RioLRTAVSRegistry.sol#L31

 function initialize(address initialOwner, address token_) external initializer {

https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/main/rio-sherlock-audit/contracts/restaking/RioLRTAssetRegistry.sol#L52

function initialize(
        address initialOwner,
        address token_,
        uint8 priceFeedDecimals_,
        AssetConfig[] calldata initialAssets
    ) external initializer {

https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/main/rio-sherlock-audit/contracts/restaking/RioLRTCoordinator.sol#L60

function initialize(address initialOwner, address token_) external initializer {

https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/main/rio-sherlock-audit/contracts/restaking/RioLRTDepositPool.sol#L38

function initialize(address initialOwner, address token_) external initializer {

https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/main/rio-sherlock-audit/contracts/restaking/RioLRTIssuer.sol#L85

function initialize(address initialOwner) external initializer {

https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/main/rio-sherlock-audit/contracts/restaking/RioLRTOperatorDelegator.sol#L75

function initialize(address token_, address operator) external initializer {

https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/main/rio-sherlock-audit/contracts/restaking/RioLRTOperatorRegistry.sol#L74

function initialize(address initialOwner, address token_) external initializer {

https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/main/rio-sherlock-audit/contracts/restaking/RioLRTRewardDistributor.sol#L37

function initialize(address initialOwner, address token_, address treasury_, address operatorRewardPool_) external initializer {

https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/main/rio-sherlock-audit/contracts/restaking/RioLRTWithdrawalQueue.sol#L41

function initialize(address initialOwner, address token_) external initializer {

Tool used

Manual Review

Recommendation

Make sure to use the correct modifier for the initializer function.

[nevillehuang]

Invalid, reentrancy is not possible, and certainly not proven, as required by sherlock rules