_revokeRole may cause access control problem .

Summary

ERC20Token#_revokeRole may cause access control problems due to the improper design of access.

Vulnerability Detail

In ERC20Token.sol, there are access control functions like setXXX and revokeXXX, setXXX is to change role to another address, and revokeXXX is to revoke one's role. The issue is according to this function, only one user can have one role, take the guardian role for example:

 function setGuardian(address newGuardian, address oldGuardian) external onlyGuardian {
        require(newGuardian != address(0), "newGuardian cannot be the zero address");
        _revokeRole(GUARDIAN_ROLE, oldGuardian);
        _grantRole(GUARDIAN_ROLE, newGuardian);
    }

once revoke function is called, no one will have the guardian role


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

revokeGuardian remove the only one guardian role , and no one has the guardian role may cause the function can never be accessed.

  function pause() external only guardian {
        _pause();
    }

For example, function pause() can never be called because no one has the guardian role.

Impact

Methods cannot be accessed.

Code Snippet

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

Tool used

Manual Review

Recommendation

revokeGuardian() is not necessary because this Contract must have a role to access pause() or other function, revokeGuardian() can be removed.

sherlock-audit / 2023-04-unitasprotocol-judging

twcctop - _revokeRole may cause access control problem . #126