Lack of two-step process for ownership transfer

[hats-bug-reporter[bot]]

Github username: @0xfuje Submission hash (on-chain): 0x1532c351c658e2dba37e7d192a6ac6b5177f60f95b51e7ee246ffcd030da57fe Severity: low

Description:

Description

The transferOwnership() function is used to change the owner of the DappNodeSmoothingPool via openzeppelin's OwnableUpgradeable helper contract. This function immediately sets the contract’s new owner. Transferring ownership in one function call is error-prone and could result in irrevocable mistakes.

openzeppelin/contracts-upgradable/access/OwnableUpgradeable.sol

    function transferOwnership(address newOwner) public virtual onlyOwner {
        require(newOwner != address(0), "Ownable: new owner is the zero address");
        _transferOwnership(newOwner);
    }

If the wrong address is mistakenly provided in transferOwnership(), the mistaken owner will be permanently set without recovery of the contract's ownership.

Recommended Mitigation

It's best practice to implement a two-step process to transfer contract ownership, in which the owner proposes a new address, then the new address can execute a call to accept their new ownership. Instead of OwnableUpgradeable consider using Ownable2StepUpgradeable

    function transferOwnership(address newOwner) public virtual override onlyOwner {
        _pendingOwner = newOwner;
        emit OwnershipTransferStarted(owner(), newOwner);
    }

    function acceptOwnership() public virtual {
        address sender = _msgSender();
        require(pendingOwner() == sender, "Ownable2Step: caller is not the new owner");
        _transferOwnership(sender);
    }

[invocamanman]

I would say that the label "bug" isn't appropriate for this issue, since it's a recommendation. So i would lower the severity to recommnedation or informational.

We will discuss if we end up adding the 2 steps owner. Since the owner is meant to be a gnosis multisig, i think there's already a double check by the participants of the gnosis safe to assure the correct transfer of the owner.