eeshenggoh - Telcoin uses RBAC method for access control and did not add roles to admin roles

[sherlock-admin2]

eeshenggoh

medium

Telcoin uses RBAC method for access control and did not add roles to admin roles

Summary

The protocol uses Role-Based Access Control in contract StakingRewardsManager and CouncilMember.sol . The implementation does not follow OpenZeppelin's documentation and leads to problems.

Vulnerability Detail

In CouncilMember.sol::initialize(), the DEFAULT_ADMIN_ROLE does not have the admin roles set up, which means the admin roles in initialize:

    bytes32 public constant GOVERNANCE_COUNCIL_ROLE =
        keccak256("GOVERNANCE_COUNCIL_ROLE");
    // Support role for additional functionality
    bytes32 public constant SUPPORT_ROLE = keccak256("SUPPORT_ROLE");

In StakingRewardsManager.sol::initialize(), the DEFAULT_ADMIN_ROLE does not have the admin roles set up in initialize:

    bytes32 public constant BUILDER_ROLE = keccak256("BUILDER_ROLE");
    bytes32 public constant MAINTAINER_ROLE = keccak256("MAINTAINER_ROLE");
    bytes32 public constant SUPPORT_ROLE = keccak256("SUPPORT_ROLE");
    bytes32 public constant ADMIN_ROLE = keccak256("ADMIN_ROLE");
    bytes32 public constant EXECUTOR_ROLE = keccak256("EXECUTOR_ROLE");

Impact

Admins are not able to perform tasks as intended, function calls will revert causing a denial of service.

Code Snippet

https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/sablier/core/CouncilMember.sol#L64

https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/telx/core/StakingRewardsManager.sol#L80

Tool used

Manual Review

Recommendation

You have to put _setRoleAdmin is a internal function, thus input them in initialize function. In CouncilMember.sol:

function initialize(
     IERC20 telcoin,
     string memory name_,
     string memory symbol_,
     IPRBProxy stream_,
     address target_,
     uint256 id_
 ) external initializer {
     _grantRole(DEFAULT_ADMIN_ROLE, _msgSender()); 
++   _setRoleAdmin(GOVERNANCE_COUNCIL_ROLE, DEFAULT_ADMIN_ROLE);
++   _setRoleAdmin(SUPPORT_ROLE, DEFAULT_ADMIN_ROLE);
     __ERC721_init(name_, symbol_);
     TELCOIN = telcoin;
     _stream = stream_;
     _target = target_;
     _id = id_;
 }

In StakingRewardsManager.sol:

 function initialize(
     IERC20 reward,
     StakingRewardsFactory factory
 ) external initializer { //@audit potential issue UUPS
     //check for zero values
     require(
         address(factory) != address(0) && address(reward) != address(0),
         "StakingRewardsManager: cannot intialize to zero"
     );

     // set up default
     _grantRole(DEFAULT_ADMIN_ROLE, _msgSender());
+    _setRoleAdmin(BUILDER_ROLE, DEFAULT_ADMIN_ROLE);
+    _setRoleAdmin(MAINTAINER_ROLE, DEFAULT_ADMIN_ROLE);
+    _setRoleAdmin(SUPPORT_ROLE, DEFAULT_ADMIN_ROLE);
+    _setRoleAdmin(ADMIN_ROLE, DEFAULT_ADMIN_ROLE);

     // set values
     rewardToken = reward;
     stakingRewardsFactory = factory;
     emit StakingRewardsFactoryChanged(factory);
 }

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

invalid because { what watson recommend here is same as whats actually there; grantRole is same as the setRole}

[goheesheng]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

invalid because { what watson recommend here is same as whats actually there; grantRole is same as the setRole}

But it isn't added in the constructor? It is missing.

[nevillehuang]

Invalid, admin can simply grant the necessary roles via grantRole() as seen here