Improper constructor implementation in a proxy context.

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0xe4495010bcfd16cdb31e17485d23b4136359c097522a02693466d044fea295ca Severity: low

Description:

Issue Description

In the USDE contract, the constructor currently initializes the validator address and calls _disableInitializers() to prevent re-initializations. However, in a proxy-based contract, the constructor should only contain _disableInitializers() without any other initialization logic. This is because, in a proxy deployment, any setup outside the initialize function might be inaccessible or function incorrectly.

Currently, the constructor includes validator assignment, which is problematic in a proxy context, as all initialization logic should be explicitly included in the initialize function to be executed upon proxy deployment.

Impact of the Vulnerability

1. Risk of contract takeover: By not following proper initialization practices, the implementation contract could be re-initialized or taken over by an attacker.
2. Initialization inconsistencies: Assigning values in the constructor can lead to issues in synchronization and data integrity when used in a proxy environment, potentially compromising contract behavior.

Proof of Concept

The current constructor in the USDE contract is implemented as follows:

constructor(IValidator _validator) {
    validator = _validator;
    _disableInitializers();
}

This approach is inappropriate for a proxy-based contract, where the constructor should only contain _disableInitializers().

Recommendation

To mitigate these risks, it is recommended to only include _disableInitializers() in the constructor. All other initialization logic, such as the assignment of validator, should be moved to the initialize function.

Example Fix:

1. Remove any assignment or additional logic in the constructor:

   constructor() {
       _disableInitializers();  // Properly disables initializers
   }

2. Move the initialization logic to the initialize function:

   function initialize(address _initialOwner, IValidator _validator) public initializer {
       __ERC20_init("EuroDollar", "USDE");
       __ERC20Pausable_init();
       __ERC20Permit_init("EuroDollar");
       __AccessControl_init();
       __UUPSUpgradeable_init();
   
       validator = _validator;  // Initialize validator in proxy context
       _grantRole(DEFAULT_ADMIN_ROLE, _initialOwner);
   }

This approach ensures that the contract is properly initialized in a proxy context and eliminates the risk of unauthorized re-initializations.

Additional Resources

For further information on _disableInitializers() and safe initialization practices in upgradeable contracts, refer to the OpenZeppelin documentation: OpenZeppelin Forum - _disableInitializers().

[AndreiMVP]

The current implementation allows for setting the implementation contract's constructor variables as immutable (for gas savings) and be used by the proxy. These immutable vars should v rarely be updated, in which case it would be like an upgrade to the protocol. Unless there are concrete issues with this, it's the intended effect.