Hawk High

First Flight #39
Beginner FriendlySolidity
100 EXP
View results
Submission Details
Severity: medium
Valid

Missing `_disableInitializers()` in Constructor for Implementation Contract

Summary

This report identifies a deviation from recommended best practices in the LevelOne.sol implementation contract. The contract lacks a constructor that calls _disableInitializers(). This function, provided by OpenZeppelin's Initializable contract, is intended to prevent the initialize() function (or any other function with an initializer modifier) from being called on a standalone deployment of the implementation contract. While the initializer modifier on the initialize function protects the proxy's instance from re-initialization, adding this constructor call is a standard hardening measure for the implementation contract itself.

Vulnerability Details / Issue Description

Upgradeable smart contracts, particularly those using proxy patterns like UUPS, consist of a proxy contract (holding state and receiving calls) and an implementation contract (containing the logic). The LevelOne.sol contract serves as such an implementation.

The initialize function in LevelOne.sol is correctly protected by the initializer modifier, which prevents it from being called more than once on a given contract instance (i.e., the proxy's storage context).

However, the implementation contract (LevelOne.sol) can also be deployed directly as a standalone contract, separate from any proxy. In such a scenario, without a constructor that calls _disableInitializers(), its initialize() function could be called, setting its state. This is generally not the intended behavior, as the implementation contract is meant to provide logic for the proxy, not to operate as an independent, initialized entity.

The _disableInitializers() function, when called in the constructor of an implementation contract, sets an internal flag (_initialized) to true for that specific standalone instance at the moment of its deployment. This effectively prevents its initialize() function or any other initializer-protected functions from ever being callable on that standalone instance.

Proof Of Concept / Scenario

  1. An entity (e.g., a developer during testing, or an external party) deploys the LevelOne.sol contract directly to the blockchain, without associating it with a UUPS proxy.

  2. The same or another entity then calls the public initialize(address, uint256, address) function on this standalone LevelOne.sol instance.

  3. The initialize() call succeeds because the initializer modifier's protection is instance-specific, and this standalone instance has not yet been initialized.

  4. This results in a standalone, initialized instance of LevelOne.sol, which is not the intended operational mode for an implementation contract designed for a proxy.

While this does not directly affect the security of the correctly deployed and proxied Hawk High system, it represents a potential for misuse or confusion.

Impact

The impact of this omission is generally considered Low / Informational for the security of the main proxied system but represents a deviation from best practices:

  • Deviation from Best Practice: The contract does not follow the standard OpenZeppelin recommendation for hardening upgradeable implementation contracts.

  • Potential for Confusion/Misuse: An initialized standalone implementation contract could be mistaken for an active part of the system or be interacted with under false assumptions.

  • Unintended State Creation: Initialization of a standalone implementation consumes gas and creates state on the blockchain for a contract instance that is not meant to be used in that manner.

The core functionality and security of the Hawk High system, when deployed via its proxy and initialized correctly through that proxy, are not directly compromised by this specific omission in the implementation contract.

Tools Used

  • Manual Code Review

  • OpenZeppelin Contracts Documentation and Best Practices

Recommendations

It is recommended to add a constructor to the LevelOne.sol contract that calls _disableInitializers(). This will prevent the initialize() function from being callable on any standalone deployments of this implementation contract.

Proposed Code Change for LevelOne.sol:

///// /////
///// INITIALIZER /////
///// /////
+ ////////////////////////////////
+
+ /// @custom:oz-upgrades-unsafe-allow constructor
+ constructor() {
+ _disableInitializers();
+ }
////////////////////////////////
function initialize(address _principal, uint256 _schoolFees, address _usdcAddress) public initializer {
if (_principal == address(0)) {
Updates

Lead Judging Commences

yeahchibyke Lead Judge 4 months ago
Submission Judgement Published
Validated
Assigned finding tags:

contract can be re-initialized

The system can be re-initialized by an attacker and its integrity tampered with due to lack of `disableInitializer()`

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.