Initialization Function Lacks Access Control and Standard Modifier

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0xf1abed6ea711730080ea3f3c25bdefcba22caf0a995bd770b03425dff5f00cb5 Severity: medium

Description: Description:

The Market contract contains an initialize function that is intended to set up the initial state of the contract. However, this function lacks proper access control and doesn't use a standard initializer modifier. While it does include a check to prevent multiple initializations, it falls short of best practices for secure contract initialization.

This vulnerability could allow any external actor to initialize the contract, potentially setting critical parameters to malicious values. Additionally, while multiple initializations are prevented, the lack of a standard initializer modifier might lead to unexpected behaviour in more complex scenarios.

Attachments

Proof of Concept (PoC) File:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;

contract Market {
    bool public initialized;

    // Other state variables...

    function initialize(
        string memory _marketName,
        string[] memory _outcomes,
        uint256 _lowerBound,
        uint256 _upperBound,
        ConditionalTokensParams memory _conditionalTokensParams,
        RealityParams memory _realityParams,
        RealityProxy _realityProxy
    ) external {
        require(!initialized, "Already initialized.");

        // Initialization logic...

        initialized = true;
    }

    // Rest of the contract...
}

The initialize function is external and can be called by anyone. While it does check for previous initialization, it lacks proper access control and doesn't use a standard initializer modifier.

Revised Code File:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;

import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";

// Fix: Inherit from Initializable and OwnableUpgradeable
contract Market is Initializable, OwnableUpgradeable {

    // State variables...

    /// @dev Initializer with access control and standard modifier.
    /// @param _marketName The name of the market.
    /// @param _outcomes The market outcomes, doesn't include the INVALID_RESULT outcome.
    /// @param _lowerBound Lower bound, only used for scalar markets.
    /// @param _upperBound Upper bound, only used for scalar markets.
    /// @param _conditionalTokensParams Conditional Tokens params.
    /// @param _realityParams Reality params.
    /// @param _realityProxy Oracle contract.
    function initialize(
        string memory _marketName,
        string[] memory _outcomes,
        uint256 _lowerBound,
        uint256 _upperBound,
        ConditionalTokensParams memory _conditionalTokensParams,
        RealityParams memory _realityParams,
        RealityProxy _realityProxy
    ) external initializer {
        __Ownable_init(); // Initialize the Ownable contract

        marketName = _marketName;
        outcomes = _outcomes;
        lowerBound = _lowerBound;
        upperBound = _upperBound;
        conditionalTokensParams = _conditionalTokensParams;
        realityParams = _realityParams;
        realityProxy = _realityProxy;
    }

    // Rest of the contract...
}

The revised code addresses the vulnerability by:

1. Inheriting from OpenZeppelin's Initializable and OwnableUpgradeable contracts.
2. Using the initializer modifier to ensure the function can only be called once.
3. Implementing __Ownable_init() to set up ownership, providing access control.

These changes ensure that the initialization can only be performed once and only by the contract deployer or designated owner, significantly improving the contract's security posture.

[xyzseer]

You need to demonstrate how would a bad actor reinitialize an already initialized contract, otherwise the check is working fine and the contract can only be initialized once

[clesaege]

I think the hunter believes that those contracts are made to be deployed directly. That's not the case, they are created by the Market Factory and initialized in the same call.