Guardable Manual Review Findings

GUA-01M: Improper Guardian Renouncation

Description:

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

Example:

Copy
31function renounceGuardian() public virtual override onlyGuardian() {
32    emit GuardianPushed( _guardian, address(0) );
33    _guardian = address(0);
34}

Recommendation:

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

Alleviation:

The new guardian is properly deleted when guardianship is renounced.

GUA-02M: Incorrect Event Emitted

Description:

The renounceGuardian function emits the GuardianPushed event when the one in place is renounced, however, it should emit the GuardianPulled event instead.

Example:

Copy
16constructor () {
17        _guardian = msg.sender;
18        emit GuardianPulled( address(0), _guardian );
19    }
20
21
22    function guardian() public view override returns (address) {
23        return _guardian;
24    }
25
26    modifier onlyGuardian() {
27        require( _guardian == msg.sender, "Guardable: caller is not the guardian" );
28        _;
29    }
30
31    function renounceGuardian() public virtual override onlyGuardian() {
32        emit GuardianPushed( _guardian, address(0) );
33        _guardian = address(0);
34    }
35
36    function pushGuardian( address newGuardian_ ) public virtual override onlyGuardian() {
37        require( newGuardian_ != address(0), "Guardable: new guardian is the zero address");
38        emit GuardianPushed( _guardian, newGuardian_ );
39        _newGuardian = newGuardian_;
40    }
41    
42    function pullGuardian() public virtual override {
43        require( msg.sender == _newGuardian, "Guardable: must be new guardian to pull");
44        emit GuardianPulled( _guardian, _newGuardian );
45        _guardian = _newGuardian;
46    }
47}

Recommendation:

We advise it to do so as the latter event is utilized in functions like pullGuardian and the constructor.

Alleviation:

The GuardianPulled event is now properly emitted within the renounceGuardian function.

GUA-03M: Potentially Restrictive Functionality

Description:

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

Example:

Copy
36function pushGuardian( address newGuardian_ ) public virtual override onlyGuardian() {
37    require( newGuardian_ != address(0), "Guardable: new guardian is the zero address");
38    emit GuardianPushed( _guardian, newGuardian_ );
39    _newGuardian = newGuardian_;
40}

Recommendation:

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

Alleviation:

The restrictive require check is now omitted from the codebase.