The revert() statement in RootPort::renounceOwnership() & BranchPort::renounceOwnership() will cause all affected initialize() functions to revert, preventing initializations of many state variables and resulting in permanent DoS of critical protocol functionalities.

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/746905cdd3aa165ba1f9274c360dfd9f0c52e781/src/RootPort.sol#L165-L168 https://github.com/code-423n4/2023-09-maia/blob/746905cdd3aa165ba1f9274c360dfd9f0c52e781/src/BranchPort.sol#L134-L137 https://github.com/Vectorized/solady/blob/d6b02fd7dcbaebc6c32004c1fbe0c2c91dddfa82/src/auth/Ownable.sol#L147-L150

Vulnerability details

Impact

(Please Note: this finding is NOT related to the issue from the "Publicly Known Issues")

SUMMARY/DESCRIPTION: The standard approach of solady's renounceOwnership() function is to remove the ability to change ownership after contract deployment/initialization, for security purposes and probably to help boost permissionless nature of protocols.

However, since it seems the Ulysses protocol has decided not to implement this functionality for these affected contracts (yet), the devs made it impossible to renounce ownership by adding the revert() statement to the two renounceOwnership() functions.

The idea was correct, but the implementation was incorrect.

RISKS/EFFECTS:

• It will be impossible to initialize many state variables from at least 6 contracts, effectively permanently DoS-ing many of the protocol's critical functionalities, including:

  -- BaseBranchRouter - For interfacing with Branch Bridge Agents. -- BranchBridgeAgentFactory - For allowing permissionless deployment of new Branch Bridge Agents. -- ArbitrumBranchBridgeAgentFactory - For allowing permissionless deployment of new Arbitrum Branch Bridge Agents. -- MulticallRootRouter - For interfacing with third-party dApps present in the Root Omnichain Environment. -- ERC20hTokenBranchFactory - For permissionless deployment of new Branch hTokens in Branch Chains of Ulysses Omnichain -- ERC20hTokenRootFactory - For permissionless deployment of new Root hTokens in the Root Chain of Ulysses Omnichain

Proof of Concept

The problem is that most of the initializable contracts' initialize() functions are calling this modified renounceOwnership() function, which will cause all these affected initialize() functions to permanently revert.

The following is the Ulysses protocol's modified implementation of the renounceOwnership() function from "solady/auth/Ownable.sol" library:

    /// @notice Function being overriden to prevent mistakenly renouncing ownership.
    function renounceOwnership() public payable override onlyOwner {
        revert("Cannot renounce ownership");    /// @audit-issue this revert statement is problematic.
    }

The modified implementation above overrides the standard implementation below from "solady/auth/Ownable.sol" library:

    /// @dev Allows the owner to renounce their ownership.
    function renounceOwnership() public payable virtual onlyOwner {
        _setOwner(address(0));
    }

One example of many:

ERC20hTokenRootFactory::initialize()

    /**
     * @notice Function to initialize the contract.
     * @param _coreRouter Address of the Root Chain's Core Router.
     */
    function initialize(address _coreRouter) external onlyOwner {
        require(_coreRouter != address(0), "CoreRouter address cannot be 0");
        coreRootRouterAddress = _coreRouter;
        renounceOwnership();                        /// @audit-issue this line will always revert
    }

All 6 affected initialize() functions/contracts: https://github.com/code-423n4/2023-09-maia/blob/746905cdd3aa165ba1f9274c360dfd9f0c52e781/src/BaseBranchRouter.sol#L66 https://github.com/code-423n4/2023-09-maia/blob/746905cdd3aa165ba1f9274c360dfd9f0c52e781/src/MulticallRootRouter.sol#L114 https://github.com/code-423n4/2023-09-maia/blob/746905cdd3aa165ba1f9274c360dfd9f0c52e781/src/factories/ArbitrumBranchBridgeAgentFactory.sol#L67 https://github.com/code-423n4/2023-09-maia/blob/746905cdd3aa165ba1f9274c360dfd9f0c52e781/src/factories/BranchBridgeAgentFactory.sol#L103 https://github.com/code-423n4/2023-09-maia/blob/746905cdd3aa165ba1f9274c360dfd9f0c52e781/src/factories/ERC20hTokenBranchFactory.sol#L76 https://github.com/code-423n4/2023-09-maia/blob/746905cdd3aa165ba1f9274c360dfd9f0c52e781/src/factories/ERC20hTokenRootFactory.sol#L52

Tools Used

VSC. Manual.

Recommended Mitigation Steps

Some options:

• either remove the renounceOwnership() method from the affected initialize() functions or
• remove the revert() statement from the renounceOwnership() method, which I show below:

    /// @notice Function being overriden to prevent mistakenly renouncing ownership.
    function renounceOwnership() public payable override onlyOwner {
    --  revert("Cannot renounce ownership");
    ++  // leave empty
    }

OR

    /// @notice Function being overriden to prevent mistakenly renouncing ownership.
    function renounceOwnership() public payable override onlyOwner {
    --  revert("Cannot renounce ownership");
    ++  emit CannotRenounceOwnership(); /// @audit obviously this will require adding a new event
    }

This effectively eliminates the problem of most initialize() functions permanently reverting, and at the same time still prevents renouncing of ownership of these contracts. So the idea/intention of the devs is maintained, but without the revert bug, so the contracts/state variables can now be initialized without DoS-ing critical protocol functionalities.

Assessed type

DoS

[c4-pre-sort]

0xA5DF marked the issue as low quality report

[0xA5DF]

They are not the same contracts, some revert on renounceOwnership(), others use this in initialization

[c4-judge]

alcueca marked the issue as unsatisfactory: Invalid