ArkProject: NFT Bridge

Summary

The L1 Starklane contract implements a custom initialization mechanism for a UUPSUpgradeable contract using a onlyInit modifier and an initialize function, rather than using the standard Initializable contract provided by OpenZeppelin. This approach introduces several potential issues related to reinitialization, improper locking of the implementation contract, and inconsistent initialization flow.

Vulnerability Details

Issue 1: No Protection Against Reinitialization

The custom onlyInit modifier prevents the same implementation from being initialized multiple times but does not prevent reinitialization if a different implementation is deployed. This creates a risk that the contract could be unintentionally reinitialized with different parameters, potentially leading to security vulnerabilities or unexpected behavior.

Issue 2: Missing _disableInitializers Function

The contract does not include the _disableInitializers function. The implementation contract should only serve as a logic contract without holding any state or requiring initialization. However, if _disableInitializers is not called, the implementation contract itself remains unprotected. This could lead to accidental or malicious initialization, where the state could be improperly set or reset, causing unexpected behavior in the proxy contract.

Issue 3: Inconsistent Initialization Flow

By deviating from the standard Initializable pattern, the contract introduces complexity and inconsistency in the initialization process. This can lead to errors during the upgrade process, as developers and auditors may not be familiar with the custom logic. The standard Initializable contract is well understood and helps prevent common initialization-related vulnerabilities.Impact

Issue 4: Initialization Function Should Be External

The initialization function in the provided contract is currently marked as public, but in UUPSUpgradeable contracts, the initialize function is generally expected to be external. This distinction is important due to several reasons related to security, gas optimization, and best practices in Solidity development.

Impact

• Reinitialization Risk: The contract could be unintentionally or maliciously reinitialized, leading to potential vulnerabilities, such as unauthorized access or incorrect state configuration.

• Implementation Vulnerability: The lack of _disableInitializers leaves the implementation contract exposed, increasing the risk of accidental or malicious reinitialization.

• Increased Complexity: The custom initialization logic increases the complexity of the contract, making it more difficult to audit and understand, potentially leading to errors during upgrades.

Tools Used

Manual code review

Recommendations

Implement _disableInitializers: Add the _disableInitializers function to the contract to lock the implementation contract in the constructor. This will prevent initialization of the implementation contract and keep it clean and empty.

Use Initializable: Replace the custom onlyInit logic with the standard Initializable contract from OpenZeppelin. This will provide a more secure and well-understood initialization mechanism.