Improving Constructor Implementation and Preventing Uninitialized Contracts

[hats-bug-reporter]

Github username: --
Twitter username: --
Submission hash (on-chain): 0x7db08b2a478c99a3e1da4c411c55c57c04350e64ee660a8ded5d47548630f657
Severity: high

Description:
Proper initialization of smart contracts, including both proxies and implementations, is critical for security. An uninitialized contract can be exploited, allowing an attacker to take control. To mitigate this risk, it’s essential to implement _disableInitializers in the constructor of the implementation contract, ensuring it cannot be reinitialized after deployment.

Current Implementation

The Dispatcher contract currently uses a constructor to initialize the registry. However, this approach can be improved to align with best security practices:

constructor(address _registry) {
    if (_registry == address(0)) {
        revert AddressError();
    }
    registry = _registry;
}

Additionally, the separate initialization function __Dispatcher_init introduces risks if not properly invoked:

function __Dispatcher_init(
    address _routerUtil,
    address _kyberRouter,
    address _pendleRouter
) internal initializer {
    if (_routerUtil == address(0)) {
        revert AddressError();
    }
    routerUtil = _routerUtil;
    kyberRouter = _kyberRouter;
    pendleRouter = _pendleRouter;
}

Recommended Changes

To enhance security, the constructor should be updated, and initialization logic should be consolidated. Below is the revised implementation:

Secure Constructor with Initialization Lock

/// @custom:oz-upgrades-unsafe-allow constructor
constructor() {
    _disableInitializers();
}

Consolidated Initialization Function

Instead of splitting initialization logic, manage all dependencies in a single initialize function:

function initialize(
    address _registry,
    address _routerUtil,
    address _kyberRouter,
    address _pendleRouter
) external initializer {
    if (_registry == address(0) || _routerUtil == address(0)) {
        revert AddressError();
    }

    registry = _registry;
    routerUtil = _routerUtil;
    kyberRouter = _kyberRouter;
    pendleRouter = _pendleRouter;
}

Benefits of the Change

1. Implementation Lock: _disableInitializers prevents the implementation from being reinitialized by unauthorized parties.
2. Centralized Logic: The initialize function ensures that all dependencies are securely handled in one place.
3. Improved Proxy Security: Ensures that proxy contracts correctly initialize the implementation logic, mitigating vulnerabilities.

These changes align the contract with best security practices and significantly reduce the risk of exploitation.

OpenZeppelin Alert:

Avoid leaving a contract uninitialized.

An uninitialized contract can be taken over by an attacker. This applies to both a proxy and its implementation contract, which may impact the proxy. To prevent the implementation contract from being used, you should invoke the _disableInitializers function in the constructor to automatically lock it when it is deployed:

/// @Custom:oz-upgrades-unsafe-allow constructor
constructor() {
_disableInitializers();
}

[yanisepfl]

Hello,

We classified this issue as Invalid because:

• It does not result in theft of user funds.
• There is a _disableInitializers() call from the constructor of the router.sol contract that prevents later calls to the Dispatcher's initializer.

Thanks