sherlock-audit / 2024-05-elfi-protocol-judging

0 stars 0 forks source link

engineer - `Public` access control functions such as `grantRole`, `revokeRole` and `renounceRole` will fail as `ADMIN_ROLE` role is not it's own admin #268

Closed sherlock-admin2 closed 1 week ago

sherlock-admin2 commented 2 weeks ago

engineer

Medium

Public access control functions such as grantRole, revokeRole and renounceRole will fail as ADMIN_ROLE role is not it's own admin

Summary

In contracts/vault/Vault.sol, a new role called ADMIN_ROLE is getting used for admin tasks. Unlike DEFAULT_ADMIN_ROLE the ADMIN_ROLE has no admin to manage it. Even though the contract defines specialised methods grantAdmin and revokeAdmin these provided methods from OpenZeppelin's AccessControl will fail:

  1. grantRole
  2. revokeRole
  3. renounceRole

Vulnerability Detail

This happens due to the modifier present in each of these functions. For example consider the OZ's code for grantRole as described in Code Snippet section. Here this call, even though publicly available to be called will fail due to onlyRole(getRoleAdmin(role)). Here getRoleAdmin(ADMIN_ROLE) will return 0x0 and the msg.sender won't have this role assigned.

Impact

OpenZeppelin's public facing functions will fail.

Code Snippet

    function grantRole(bytes32 role, address account) public virtual override onlyRole(getRoleAdmin(role)) {
        _grantRole(role, account);
    }

Tool used

Manual Review

Recommendation

Update Vault's constructor

    constructor(address admin) {
        _grantRole(ADMIN_ROLE, admin);
+       _setRoleAdmin(ADMIN_ROLE, ADMIN_ROLE);
    }

Duplicate of #220