obront - Pools cannot be initialized because of incorrectly used initializer modifier

[sherlock-admin]

obront

high

Pools cannot be initialized because of incorrectly used initializer modifier

Summary

The MarketExtended.sol:initializePools() function is called as a delegatecall from MarketCore.sol. This function has an initializer modifier, but MarketCore.sol will have already run a function with an initializer modifier. The result is that this function cannot be called, and pools cannot be initialized.

Vulnerability Detail

The initializer modifier from OpenZeppelin only allows a function to be called once, and then writes the storage variable _initialized = true to ensure it is not called again.

    modifier initializer() {
        bool isTopLevelCall = !_initializing;
        require(
            (isTopLevelCall && _initialized < 1) || (!AddressUpgradeable.isContract(address(this)) && _initialized == 1),
            "Initializable: contract is already initialized"
        );
        _initialized = 1;
        if (isTopLevelCall) {
            _initializing = true;
        }
        _;
        if (isTopLevelCall) {
            _initializing = false;
            emit Initialized(1);
        }
    }

Because the initializePools() function is called as a delegatecall from MarketCore.sol, it will operate on the storage of the calling contract. This storage already has an _initialized variable, which will have been set to true by the initializer modifier on MarketCore.sol's constructor.

Here is a Gist with a copy & paste Forge test that can be used to verify the error.

Impact

The initializePools() function is required for the market to operate. Without this function being called, no market will be able to add an admin, setup roles, connect to an oracle, set a liquidity manager, set maxPercentChange or set latestExecutedEpochIndex correctly to begin operating.

Code Snippet

https://github.com/sherlock-audit/2022-11-float-capital/blob/main/contracts/market/template/MarketCore.sol#L677-L689

https://github.com/sherlock-audit/2022-11-float-capital/blob/main/contracts/market/template/MarketExtended.sol#L23

Tool used

Manual Review, Foundry

Recommendation

Remove the initializer modifier from the constructor in MarketCore.sol.

[JasoonS]

This is just simply wrong. We call the initializer in the constructor so that no-one can initialize the implementations again.

All contracts use proxies.

See the fix to your snippet:

The other contracts in that file also need to use a proxy.