Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Katana Governance will never be able to initialize the V3factory because of a wrong check #50

Closed
howlbot-integration bot opened this issue Nov 4, 2024 · 1 comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-26 🤖_primary AI based primary recommendation 🤖_100_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards

Comments

@howlbot-integration
Copy link

Lines of code

https://github.com/ronin-chain/katana-v3-contracts/blob/03c80179e04f40d96f06c451ea494bb18f2a58fc/src/core/KatanaV3Factory.sol#L32-L35
https://github.com/ronin-chain/katana-v3-contracts/blob/03c80179e04f40d96f06c451ea494bb18f2a58fc/src/core/KatanaV3Factory.sol#L46-L74

Vulnerability details

Proof of Concept

** Summary **
The Katana governance system cannot initialize the v3Factory due to an incorrect check within the initialize function. When the v3Factory is deployed, the constructor sets the beacon address to address(1), disabling initialization by default. However, when the initialize function is called, it checks if beacon is address(0) instead of address(1), causing the function to revert every time. This misconfiguration blocks the governance system from setting key parameters and using the v3Factory.

** Vulnerability Details **
In the current setup, when the v3Factory is deployed, the constructor includes this line:

constructor() {
    // disable initialization
@audit >>1. >>     beacon = address(1);

}

This line sets the beacon address to address(1), effectively disabling the initialization until it’s called with valid parameters. However, the initialize function attempts to validate the uninitialized state of the beacon by checking for address(0), which will always fail and cause a reversion. This prevents any further setup for governance, as initialization cannot proceed.

** Code Reference **

  1. Constructor Setting the Beacon
constructor() {
    // disable initialization

@audit >>1. >>      beacon = address(1);

}
  1. initialize Function with Incorrect Check
function initialize(address beacon_, address owner_, address treasury_) external {
    
@audit >>REVERT >> require(beacon == address(0), "KatanaV3Factory: ALREADY_INITIALIZED");

    require(beacon_ != address(0), "KatanaV3Factory: INVALID_BEACON");
    require(owner_ != address(0), "KatanaV3Factory: INVALID_OWNER");
    require(treasury_ != address(0), "KatanaV3Factory: INVALID_TREASURY");
    // Additional setup logic follows...
}

** Impact **
This misconfiguration in the initialize function has significant implications:

  1. Governance Lockout: The governance will not be able to initialize v3Factory, preventing essential operations, such as setting fees and controlling the factory.
  2. Operational Risk: As governance cannot proceed with the v3Factory configuration, the system becomes incomplete and non-functional in its intended capacity.

Recommended Mitigation Steps

To resolve this issue, update the check in the initialize function to confirm if beacon is set to address(1) instead of address(0). This will align with the constructor configuration and allow the initialize function to proceed only when beacon is address(1).

Updated initialize Function Recommendation:

function initialize(address beacon_, address owner_, address treasury_) external {


--    require(beacon == address(0), "KatanaV3Factory: ALREADY_INITIALIZED");
++    require(beacon == address(1), "KatanaV3Factory: ALREADY_INITIALIZED");

    require(beacon_ != address(0), "KatanaV3Factory: INVALID_BEACON");
    require(owner_ != address(0), "KatanaV3Factory: INVALID_OWNER");
    require(treasury_ != address(0), "KatanaV3Factory: INVALID_TREASURY");
    // Additional setup logic follows...
}

Assessed type

DoS

@howlbot-integration howlbot-integration bot added 3 (High Risk) Assets can be stolen/lost/compromised directly 🤖_100_group AI based duplicate group recommendation 🤖_primary AI based primary recommendation bug Something isn't working duplicate-26 sufficient quality report This report is of sufficient quality labels Nov 4, 2024
howlbot-integration bot added a commit that referenced this issue Nov 4, 2024
@c4-judge
Copy link

alex-ppg marked the issue as unsatisfactory:
Invalid

@c4-judge c4-judge added the unsatisfactory does not satisfy C4 submission criteria; not eligible for awards label Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-26 🤖_primary AI based primary recommendation 🤖_100_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards
Projects
None yet
Development

No branches or pull requests

1 participant