Contract initialization is unprotected and is vulnerable to front-running

[hats-bug-reporter[bot]]

Github username: @0xfuje Submission hash (on-chain): 0x2b440f1884c88f8ca016bc058bc3cf6d07f58a0152b675f34384192a5c9f3b66 Severity: medium

Description:

Impact

Contract have to be redeployed with a fix. Funds can be lost if an attacker's initialization remains undetected

Description

DappnodeSmoothingPool implements openzeppelin's upgradeable model. The problem is that it's unprotected from an attacker initializing the contract. The uninitialized contract can be taken over by the attacker for example by front-running the original deployer intialize() call. This applies to both the proxy and its implementation contract.

From openzeppelin's documentation:

Do not leave an implementation contract uninitialized. An uninitialized implementation contract can be taken over by an attacker, which may impact the proxy. To prevent the implementation contract from being used, you should invoke the _disableInitializers function in the constructor to automatically lock it when it is deployed

DappNodeSmoothingPool.sol - initialize()

    function initialize(
        address _governance,
        uint256 _subscriptionCollateral,
        uint256 _poolFee,
        address _poolFeeRecipient,
        uint64 _checkpointSlotSize,
        uint64 _quorum
    ) external initializer {
        // Initialize requires
        require(
            _poolFee <= 10000,
            "DappnodeSmoothingPool::initialize: Pool fee cannot be greater than 100%"
        );

        require(
            _quorum != 0,
            "DappnodeSmoothingPool::initialize: Quorum cannot be 0"
        );

        // Set initialize parameters
        governance = _governance;
        subscriptionCollateral = _subscriptionCollateral;

        checkpointSlotSize = _checkpointSlotSize;
        quorum = _quorum;

        poolFee = _poolFee;
        poolFeeRecipient = _poolFeeRecipient;
        deploymentBlockNumber = block.number;

        // Initialize OZ libs
        __Ownable_init();

        // Emit events
        emit UpdatePoolFee(_poolFee);
        emit UpdatePoolFeeRecipient(_poolFeeRecipient);
        emit UpdateCheckpointSlotSize(_checkpointSlotSize);
        emit UpdateQuorum(_quorum);
    }

Recommended Mitigation

Invoke _disableInitializers() in the constructor of the contract to prevent the implementation contract from being abused by an attacker. In the future always make sure to call _disableInitializers() in the constructor of any upgradeable contracts.

/// @custom:oz-upgrades-unsafe-allow constructor
constructor() {
    _disableInitializers();
}

More information about upgradeable contracts is in: openzeppelin's docs

[0xDetermination]

I don't think the recommendation given here is valid, since the protocol doesn't seem to be utilizing proxy-implementation architecture. In this case calling _disableInitializers() in the constructor will prevent the contract from being initialized.

[invocamanman]

About the contract initialization front-run, the issue is duplicated here, and it's explained why cannot be front-runned due the OZ scripts deploys and initialize the proxies in the same transaction https://github.com/hats-finance/Smooth-0x64bc275b37e62eec81a00ecaecd2b9567058f990/issues/21

About the recommendation, since this is not an UUPS proxy, adding this constructor would not add any security to the current system. Anyway i will consult with the team if we add it as a best practices, if that's the case this issue should be informational/recommendation but i would not labeled as a "bug" since it's not exploitable and it does not add any security adding this code.

[invocamanman]

We end up deciding to not add this recommendation, since we are using transparent proxies and it's not necessary ^^