0xNorman - Lack of two-step role transfer

[sherlock-admin]

0xNorman

medium

Lack of two-step role transfer

Summary

TransferOwnership() without two-step pattern might result in ownership accidentally transferred into unknown or malicious address, cause panic to the entire access control system.

Vulnerability Detail

In the OwnableImpl.transferOwnership() lacks two-step role transfer. Also the basic validation whether the address is not a zero address is not performed. Though the Owner is trusted, two-step role transfer should be preferable to avoid accidental wrong ownership transfer.

Impact

if ownership is transferred to an unknown or malicious address, the corresponding oracle, swapper, & pass-through-wallet may get into panic.

Code Snippet

https://github.com/sherlock-audit/2023-04-splits/blob/main/splits-utils/src/OwnableImpl.sol#L45-L48

Tool used

Manual Review

Recommendation

Implement zero address check and Consider implementing a two step process where the owner nominates an account and the nominated account needs to call an claimOwnership() function for the transfer of ownership to fully succeed. This ensures the nominated EOA account is a valid and active account.

    address internal $owner;
+   address internal pending$owner;
    function transferOwnership(address owner_) public virtual onlyOwner{
-        $owner = owner_;
+        require(owner_ != address(0), "owner_ is zero address");
+        pending$owner = owner_;
    }

+    function claimOwnership() public virtual {
+        require(msg.sender == pending$owner, "Ownable: caller is not the pending $owner");
+        emit OwnershipTransferred($owner, pending$owner);
+        $owner = pending$owner;
+        pending$owner = address(0);
+    }
}