Revoke for Governor and Guardian are dangerous

Summary

In the ERC20Token contract there are Governor and Guardian roles. As part of administration, the contract exposes set and revoke functions for both the roles. These functions due to human error can result in non functioning roles for both governor and guardian.

Vulnerability Detail

The revoke functions are dangerous as in order to setGovernor or setGuardian, the caller of the function should be in that role. If the current governor calls revokeGovernor, it will revoke the governor role of the user, but after this, the ability to set governor is lost.

Impact

As a consequence of this lost of entitlements, the minter roles can no more be managed by the governor. There will be no way to set and revoke Minter after that.

The existing minters can continue to mint the ERC20 tokens for ever. Governor will have no control over them.

Similar issue exists with Guardian role as well. Once the Guardian role is lost, the ability to pause and unpause the contract will also be permanently lost. This applies also the ability to manage backlist accounts.

Code Snippet

https://github.com/sherlock-audit/2023-04-unitasprotocol/blob/main/Unitas-Protocol/src/ERC20Token.sol#L103-L106

Revoke function for Governor

function revokeGovernor(address oldGovernor) external onlyGovernor { //  @audit dangerous
        require(oldGovernor != address(0), "oldGovernor cannot be the zero address");
        _revokeRole(GOVERNOR_ROLE, oldGovernor);
    }

https://github.com/sherlock-audit/2023-04-unitasprotocol/blob/main/Unitas-Protocol/src/ERC20Token.sol#L123-L126 Revoke function for Guardian

 function revokeGuardian(address oldGuardian) external onlyGuardian { // @audit dangerous
        require(oldGuardian != address(0), "oldGuardian cannot be the zero address");
        _revokeRole(GUARDIAN_ROLE, oldGuardian);
    }

//the below two functions will not function will governor role is revoked
function setMinter(address newMinter, address oldMinter) external onlyGovernor {
....
}
function revokeMinter(address oldMinter) external onlyGovernor {
....
}

//the below two functions will not function will guardian role is revoked
 function pause() external onlyGuardian {
....
}

function unpause() external onlyGuardian {
....
}

function addBlackList(address evilUser) public onlyGuardian { .... }

function removeBlackList(address clearedUser) public onlyGuardian { .... }

function getBlacklist(address addr) public onlyGuardian view returns(bool) { .... }

Tool used

Manual Review

Recommendation

Remove the external functions for revokeGuardian and revokeGovernor. Let the set functions take care of revoking old and setting now.

sherlock-audit / 2023-04-unitasprotocol-judging

ravikiran.web3 - Revoke for Governor and Guardian are dangerous #18