initialize will never execute in KatanaV3Factory and KatanaV3Pool

[howlbot-integration]

Lines of code

https://github.com/ronin-chain/katana-v3-contracts/blob/03c80179e04f40d96f06c451ea494bb18f2a58fc/src/core/KatanaV3Factory.sol#L46-L47
https://github.com/ronin-chain/katana-v3-contracts/blob/03c80179e04f40d96f06c451ea494bb18f2a58fc/src/core/KatanaV3Pool.sol#L117

Vulnerability details

Proof of Concept

This vulnerability can be found in both KatanaV3Factory::initialize and KatanaV3Pool::initializeImmutables

However, let us take a look at KatanaV3Factory::initialize from the code below, we can observe that in the constructor beacon=address(1) which causes initialize not to execute but revert on the require statement.

why? This is because when you look at the function the first line in the body is a require statement that makes sure our beacon==address(0) but from our constructor you can see the beacon is already initialised to address(1) there by causing the require statement to always revert.

  constructor() {
    // disable initialization
>    beacon = address(1);
  }

  modifier onlyOwner() {
    _checkOwner();
    _;
  }

  function _checkOwner() internal view virtual {
    require(owner == msg.sender, "KatanaV3Factory: FORBIDDEN");
  }
//@audit 
  function initialize(address beacon_, address owner_, address treasury_) external {
>    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");

    // this beacon is treated as immutable
    // so there is no need to emit an event
    beacon = beacon_;

    // owner is also treated as immutable
    owner = owner_;

    treasury = treasury_;
    emit TreasuryChanged(address(0), treasury_);

    _enableFeeAmount(100, 1, 5, 10);
    _enableFeeAmount(3000, 60, 5, 30);
    _enableFeeAmount(10000, 200, 15, 100);
  }

When this happens this will cause the onlyOwner modifier to always revert since owner has not been initialized and the _checkOwner will revert. This further causes all onlyOwner functions to be unusable.

Looking at the KatanaV3Pool::initializeImmutables which has the same vulnerability present we can further confirm from the docs/natspec we can confirm that the initializeImmutables is meant to be called once during deployment.

but from our analysis earlier we can also conclude that this function will never run because of the constructor which sets factory=address(1) and the require statement that will always revert because factory is not address(0).

Recommended Mitigation Steps

Implement a new logic in such a way that contract can only be initialized once.

Assessed type

Other

[nevillehuang]

This is likely invalid, KatanaV3Factory and KatanaV3Pools are likely proxy contracts that have uninitialized state variables, in which case the initialization will succeed.

[khangvv]

As @nevillehuang mentioned, both KatanaV3Pool and KatanaV3Factory are proxy contracts, so they must be capable of being initialized.

[alex-ppg]

The submission and its duplicates seem to stem from a misunderstanding of how the proxy model separates the storage and logic of a contract. The constructor implementations affect the storage of the logic implementation, whereas the initialization functions are expected to affect the storage state of the proxies. All functions appear to have been properly implemented and no submission has outlined a material vulnerability regarding them.

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid