_disableInitializers is missing in Bridge’s constructor

[ptisserand]

From https://codehawks.cyfrin.io/c/2024-07-ark-project/s/181

Summary

disableinitializers is not called in Starklane’s constructor

details

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. But in the case of the implementation contract, a disableinitializers() is necessary to be called in the constructor. This is because when the Bridge contract is deployed and initialized, the initialize method on the newly created proxy's implementation contract is never called. As such, anyone can call that method and pass in whatever values they want as arguments

Unit test must be provided.

[od-hunter]

Can I be assigned when od hack starts please! @ptisserand

[PoulavBhowmick03]

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

I'm Poulav Bhowmick, a software engineer at Invisible Studios with a robust background in TypeScript, Rust, Solidity Cairo, fullstack development and blockchain technology. My experience includes building robust applications, optimizing functionalities and blockchain integration. I have actively participated in events and open source contributions, enhancing my capability to tackle real-world tech challenges. My projects can be viewed on my GitHub Profile and OnlyDust Profile. Plus I´m active member of Starknet, Ethereum ecosystem.

How I plan on tackling this issue

I will resolve this issue by taking the following approach:

1. Adding _disableInitializers() to the Bridge Constructor:

   • The main issue here is that the Bridge contract’s implementation is vulnerable to being initialized multiple times, allowing potential attackers to exploit this. To fix this, I will add a constructor to the Bridge contract that calls _disableInitializers(), ensuring that the contract can only be initialized once.

   Example:

   constructor() {
      _disableInitializers();
   }

   This will ensure that the implementation contract is protected from further initialization once it is deployed.

2. Reviewing Starklane and Bridge Contracts:

   • I will review the existing implementation of both the Bridge and Starklane contracts to ensure that this issue doesn't exist elsewhere.
   • I will verify that _disableInitializers() is called in any other contract that is deployed via a proxy to avoid similar vulnerabilities.

3. Writing Unit Tests:

   • To prevent regressions and validate this fix, I will implement unit tests that check whether the constructor correctly disables initializers.
   • Specifically, I will test that:
     • After deploying the Bridge contract, any subsequent calls to initialize() should revert.
     • I will simulate an attacker trying to call initialize() and confirm that they are unable to pass arbitrary arguments due to _disableInitializers() being triggered.

   Example test case:

   function testCannotInitializeAfterDeployment() public {
      vm.expectRevert("Initializers are disabled");
      bridge.initialize(/* malicious arguments */);
   }

4. Manual and Automated Testing:

   • After making the changes, I will manually verify that the vulnerability is fixed by interacting with the contract and ensuring no unauthorized initializations can occur.
   • I will also add the new unit tests to the project’s CI pipeline to guarantee that future updates don’t reintroduce this issue.

By following these steps, I’ll make sure the Bridge contract is protected against unauthorized initialization, and I will ensure that robust tests are in place to confirm the fix. ETA - 1 day

[aji70]

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

i'm a solidity and cairo smart contract developer with over 2 years experience and believe i have the skill set for the task and i am also very good with smart contract testing

How I plan on tackling this issue

would create a private disableinitializer function with the needed logic and call it in the constructor

[ryzen-xp]

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

I'm a Solidity adn cairo developer specializing in NFT marketplaces and decentralized apps, with experience in multi-token support and integrating blockchain protocols. My work on projects like Worldcoin-Bridge-Linea equips me to handle tasks like adding ERC-20 support efficiently.

How I plan on tackling this issue

To handle the missing _disableInitializers in the Bridge’s constructor:

1. Add _disableInitializers()** call in the Bridge constructor to prevent unauthorized initialization.
2. Ensure that this is done before any other logic to secure the contract from potential attacks.
3. Write unit tests to confirm that the function is correctly called and that the contract cannot be initialized after deployment.

[DanielEmmanuel1]

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

Good Morning ArkProject, My name is Deon and I'd like to apply formally for the task presented. I am a Web and blockchain engineer with a passion for building user interfaces and Dapps that deliver meaningful experiences. With a background in Computer Science (BSc) and hands-on experience. If given the chance to contribute this will be my second official contribution via onlydust and I'm confident in my ability to deliver on the feature you're looking for.

How I plan on tackling this issue

I will employ the following approach to ensure disableInitializers is called to prevent uninitialized contracts from being exploited:

Understand the Contract Initialization Flow: I will review the Starklane contract’s initialization process, focusing on how the proxy pattern interacts with the implementation contract. This will help ensure that the proxy’s implementation contract cannot be initialized multiple times or taken over by an attacker.

Add disableInitializers() to the Constructor: I will update the Starklane contract’s constructor to include a call to disableInitializers(). This will lock the contract's initialization function, preventing any further calls to initialize, which could otherwise be exploited by an attacker to set arbitrary values.

Write Unit Tests for Verification: I will write unit tests to ensure that after deployment, the initialize function cannot be called on the implementation contract. These tests will confirm that calling initialize post-deployment throws an error, validating that the contract has been correctly locked.

Comprehensive Coverage: I will ensure that the solution applies across the entire upgradeable contract lifecycle, preventing any misuse of the initialization function and securing the proxy pattern implementation.

[onlydustapp[bot]]

The maintainer ptisserand has assigned PoulavBhowmick03 to this issue via OnlyDust Platform. Good luck!