Ownable Manual Review Findings

OWN-01M: Improper Ownership Renouncation

Description:

The renounceManagement function deletes the current _owner in place, however, the _newOwner remains especially so when the pullManagement function is invoked once as it does not delete the previous entry. This would permit ownership to be re-instated even after it has been renounced on a particular contract.

Example:

Copy
28function renounceManagement() public virtual override onlyOwner() {
29    emit OwnershipPushed( _owner, address(0) );
30    _owner = address(0);
31}

Recommendation:

We strongly recommend the _newOwner to also be deleted when renounceManagement is invoked to prevent improper state transitions.

Alleviation:

The new owner is properly deleted when ownership is renounced.

OWN-02M: Incorrect Events Emitted

Description:

The constructor and renounceManagement function of the Ownable contract incorrectly emit the OwnershipPushed event instead of the OwnershipPulled one.

Example:

Copy
11event OwnershipPushed(address indexed previousOwner, address indexed newOwner);
12event OwnershipPulled(address indexed previousOwner, address indexed newOwner);
13
14constructor () {
15    _owner = msg.sender;
16    emit OwnershipPushed( address(0), _owner );
17}
18
19function owner() public view override returns (address) {
20    return _owner;
21}
22
23modifier onlyOwner() {
24    require( _owner == msg.sender, "Ownable: caller is not the owner" );
25    _;
26}
27
28function renounceManagement() public virtual override onlyOwner() {
29    emit OwnershipPushed( _owner, address(0) );
30    _owner = address(0);
31}
32
33function pushManagement( address newOwner_ ) public virtual override onlyOwner() {
34    require( newOwner_ != address(0), "Ownable: new owner is the zero address");
35    emit OwnershipPushed( _owner, newOwner_ );
36    _newOwner = newOwner_;
37}
38
39function pullManagement() public virtual override {
40    require( msg.sender == _newOwner, "Ownable: must be new owner to pull");
41    emit OwnershipPulled( _owner, _newOwner );
42    _owner = _newOwner;
43}

Recommendation:

We advise the latter to be utilized as it is the canonical one when an _owner entry is written in pullManagement.

Alleviation:

The OwnershipPulled event is now properly emitted within the renounceOwnership function.

OWN-03M: Potentially Restrictive Functionality

Description:

The pushManagement function validates that the newOwner_ being set is not the zero address, however, in doing so it will prevent a pending owner to be overwritten with a zero entry.

Example:

Copy
33function pushManagement( address newOwner_ ) public virtual override onlyOwner() {
34    require( newOwner_ != address(0), "Ownable: new owner is the zero address");
35    emit OwnershipPushed( _owner, newOwner_ );
36    _newOwner = newOwner_;
37}

Recommendation:

We advise this check to either be omitted entirely or only be evaluated when _newOwner is itself equal to the zero-address as otherwise, an incorrectly set _newOwner will not be able to be deleted.

Alleviation:

The restrictive require check is now omitted from the codebase.