Governable Manual Review Findings

GOV-01M: Improper Governor Renouncation

Description:

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

Example:

Copy
32function renounceGovernor() public virtual override onlyGovernor() {
33    emit GovernorPushed( _governor, address(0) );
34    _governor = address(0);
35}

Recommendation:

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

Alleviation:

The new governor is properly deleted when governorship is renounced.

GOV-02M: Incorrect Event Emitted

Description:

The renounceGovernor function emits the GovernorPushed event when the one in place is renounced, however, it should emit the GovernorPulled event instead.

Example:

Copy
16constructor () {
17    _governor = msg.sender;
18    emit GovernorPulled( address(0), _governor );
19}
20
21/* ========== GOVERNOR ========== */
22
23function governor() public view override returns (address) {
24    return _governor;
25}
26
27modifier onlyGovernor() {
28    require( _governor == msg.sender, "Governable: caller is not the governor" );
29    _;
30}
31
32function renounceGovernor() public virtual override onlyGovernor() {
33    emit GovernorPushed( _governor, address(0) );
34    _governor = address(0);
35}
36
37function pushGovernor( address newGovernor_ ) public virtual override onlyGovernor() {
38    require( newGovernor_ != address(0), "Governable: new governor is the zero address");
39    emit GovernorPushed( _governor, newGovernor_ );
40    _newGovernor = newGovernor_;
41}
42
43function pullGovernor() public virtual override {
44    require( msg.sender == _newGovernor, "Governable: must be new governor to pull");
45    emit GovernorPulled( _governor, _newGovernor );
46    _governor = _newGovernor;
47}

Recommendation:

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

Alleviation:

The GovernorPulled event is now properly emitted within the renounceGovernor function.

GOV-03M: Potentially Restrictive Functionality

Description:

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

Example:

Copy
37function pushGovernor( address newGovernor_ ) public virtual override onlyGovernor() {
38    require( newGovernor_ != address(0), "Governable: new governor is the zero address");
39    emit GovernorPushed( _governor, newGovernor_ );
40    _newGovernor = newGovernor_;
41}

Recommendation:

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

Alleviation:

The restrictive require check is now omitted from the codebase.