Add zero address check in Ownable2Step’s transferOwnership function

[PurrProof]

🧐 Motivation

In the Ownable2Step contract, the transferOwnership function does not check if the newOwner address is the zero address. While it's true that the zero-address owner cannot complete the second step to accept ownership, initiating the transfer to the zero address is a meaningless action. In the Ownable parent contract, this check is enforced, and for consistency and clarity, it would make sense to implement the same check in Ownable2Step.

📝 Details

The Ownable2Step contract allows calling transferOwnership with a zero address, as seen here:

    /**
     * @dev Starts the ownership transfer of the contract to a new account. Replaces the pending transfer if there is one.
     * Can only be called by the current owner.
     */
    function transferOwnership(address newOwner) public virtual override onlyOwner {
        _pendingOwner = newOwner;
        emit OwnershipTransferStarted(owner(), newOwner);
    }

However, in the Ownable parent contract, a check is included to ensure the newOwner is not the zero address, as seen here:

    /**
     * @dev Transfers ownership of the contract to a new account (`newOwner`).
     * Can only be called by the current owner.
     */
    function transferOwnership(address newOwner) public virtual onlyOwner {
        if (newOwner == address(0)) {
            revert OwnableInvalidOwner(address(0));
        }
        _transferOwnership(newOwner);
    }

While initiating a transfer to the zero address in Ownable2Step doesn't lead to incorrect behavior (since the zero address can't accept ownership), it's still a pointless action. Since we already prevent this in the Ownable contract, it would be more consistent and logical to include the same validation in Ownable2Step.

If this suggestion is agreeable, I’d be happy to submit a PR for it.

[PurrProof]

On the other hand, this might not be fully backward compatible, as someone could be using transferOwnership(0) to cancel a pending ownership transfer.

In that case, what do you think about documenting this behavior and adding corresponding tests to handle the scenario of canceling an ownership transfer with the zero address?

[Skyge]

Yeah, I think the most point is just like you said, someone could be using transferOwnership(0) to cancel a pending ownership transfer.

[Amxx]

Hello @PurrProof

initiating the transfer to the zero address is a meaningless action

That is incorrect. If a transfer has been initiated, the pending owner is set, and the current owner wants to cancel that transfer, then doing transferOwnership(0) would be a good way to cancel that.

In that case, we could imagine emitting a different event. This would be a backward incompatible change though.

[PurrProof]

@Amxx,

Yes, thank you, we've discussed this in the previous comments.

Could you please take a look at the proposed PR? A comment has been added to the Ownable2Step contract, and the tests have been updated for the zero address case.