OwnableImpl() lacks some sanity check and a two-step procedure to change ownership.
Summary
OwnableImpl() lacks some sanity check and a two-step procedure to change ownership.
Vulnerability Detail
OwnableImpl is an abstract contract that allows an admin to manage the ownership of a contract including setting ownership and the transfer of ownership.
1) lack of zero address check for owner_ for the following two functions, __initOwnable() and transferOwnership(). If a zero address is entered by mistake or on purpose for owner_, then the ownership is lost forever.
2) The transferOwnership() uses a one-step procedure to change ownership. This might be dangerous, if an invalid address is entered, either by mistake or by a compromised owner, then the ownership is lost forever. Current practice uses a two-step procedure to change ownership, using a pendingOwner as a proposed new owner, and then only the pendingOwner can accept the owernership.
Impact
Due to lack of these checks, the ownership of a contract might be lost forever.
chaduke
medium
OwnableImpl() lacks some sanity check and a two-step procedure to change ownership.
Summary
OwnableImpl() lacks some sanity check and a two-step procedure to change ownership.
Vulnerability Detail
OwnableImplis an abstract contract that allows an admin to manage the ownership of a contract including setting ownership and the transfer of ownership.https://github.com/sherlock-audit/2023-04-splits/blob/main/splits-utils/src/OwnableImpl.sol#L7-L57
However, it has two issues:
1) lack of zero address check for
owner_for the following two functions,__initOwnable()andtransferOwnership(). If a zero address is entered by mistake or on purpose forowner_, then the ownership is lost forever.2) The
transferOwnership()uses a one-step procedure to change ownership. This might be dangerous, if an invalid address is entered, either by mistake or by a compromised owner, then the ownership is lost forever. Current practice uses a two-step procedure to change ownership, using apendingOwneras a proposed new owner, and then only thependingOwnercan accept the owernership.Impact
Due to lack of these checks, the ownership of a contract might be lost forever.
Code Snippet
See above
Tool used
Remix
Manual Review
Recommendation
Use OpenZeppelin's Ownable2Step. https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable2Step.sol