tpiliposian - Insecure Role Checks in MidasAccessControl

[sherlock-admin2]

tpiliposian

medium

Insecure Role Checks in MidasAccessControl

Summary

The MidasAccessControl.sol contract currently suffers from an insecure role management mechanism, which allows potential unauthorized modifications of role assignments. Specifically, the flaw arises from the fact that a role owner can front-run role revocations by assigning their role to another address, thereby circumventing intended role revocation actions.

Vulnerability Detail

The vulnerability stems from the flawed role management functions grantRoleMult() and revokeRoleMult() in the MidasAccessControl.sol contract. While these functions are designed to grant or revoke multiple roles, they lack adequate access controls to prevent unauthorized role modifications. Although the role owner should ideally be the only entity capable of granting or revoking their respective role, the current implementation allows for abuse. For instance, if the deployer gives address X the role of A, intending to revoke it later, X can potentially front-run this action by assigning the A role to address Y before the deployer can execute the intended revocation:

        for (uint256 i = 0; i < roles.length; i++) {
            _checkRole(getRoleAdmin(roles[i]), sender);
            _grantRole(roles[i], addresses[i]);
        }

        for (uint256 i = 0; i < roles.length; i++) {
            _checkRole(getRoleAdmin(roles[i]), sender);
            _revokeRole(roles[i], addresses[i]);
        }

Impact

Role owners can exploit the system to elevate the privileges of unauthorized addresses by granting them roles they should not possess. The vulnerability allows to bypass of intended role revocation actions, thereby maintaining access to privileged functionalities even after they were supposed to be revoked.

Code Snippet

https://github.com/sherlock-audit/2024-05-midas/blob/main/midas-contracts/contracts/access/MidasAccessControl.sol#L34-L44

https://github.com/sherlock-audit/2024-05-midas/blob/main/midas-contracts/contracts/access/MidasAccessControl.sol#L53-L63

Tool used

Manual Review

Recommendation

Introduce a designated general admin role responsible for all role management actions. Only this admin should have the authority to grant or revoke roles. Modify the grantRoleMult() and revokeRoleMult() functions to include a modifier that restricts access to the general admin role and finally add functionality to change general admin if needed, ensuring flexibility and scalability in the role management system.

[tpiliposian]

Hey there, Can I have an explanation as to why this issue is excluded? Which part of it is not correct?

[tpiliposian]

Imo, this can be considered as a duplicate of https://github.com/sherlock-audit/2024-05-midas-judging/issues/160 as this report covers the issue described in 160, and even more cases, e.g. Alice is greenlisted, then admin wants to remove greenlist role from Alice, and Alice gives her role to Bob before it.

[pronobis4]

This doesn't look like a duplicate, it's specifically about the renounceRole function which you didn't mention once.

[tpiliposian]

@pronobis4 Thanks again for the quick response, I agree with you in terms of renounceRole, I just meant that this all can be a general issue about Role management. But let's then review my issue once more. Can I have an explanation my this is not valid?

[MxAxM]

@pronobis4 Thanks again for the quick response, I agree with you in terms of renounceRole, I just meant that this all can be a general issue about Role management. But let's then review my issue once more. Can I have an explanation my this is not valid?

Root cause of #160 is renounce blacklisting role which isn't mentioned in this report, also this report is invalid because admins are trusted

[tpiliposian]

Maybe my example with X, Y, and A was confusing, but let's examine this scenario: The admin initially gives a M_TBILL_PAUSE_OPERATOR_ROLE or GREENLIST_OPERATOR_ROLE or whatever role to the Alice, then Alice can grant her role to Bob, here is the issue, that, if then the admin wants to revoke role from Alice she can give her role to the Bob, and it will give a Bob a window to an unauthorised function calls as M_TBILL_PAUSE_OPERATOR_ROLE or GREENLIST_OPERATOR_ROLE role owner. I suggested to not give all role owners the grantRoleMult and revokeRoleMult functionality.

[MxAxM]

Maybe my example with X, Y, and A was confusing, but let's examine this scenario: The admin initially gives a M_TBILL_PAUSE_OPERATOR_ROLE or GREENLIST_OPERATOR_ROLE or whatever role to the Alice, then Alice can grant her role to Bob, here is the issue, that, if then the admin wants to revoke role from Alice she can give her role to the Bob, and it will give a Bob a window to an unauthorised function calls as M_TBILL_PAUSE_OPERATOR_ROLE or GREENLIST_OPERATOR_ROLE role owner. I suggested to not give all role owners the grantRoleMult and revokeRoleMult functionality.

They're trusted

[tpiliposian]

Don't want to argue, and with all due respect, but if they are trusted why having revoke role functionality? Okay, the general admin is Trusted, so only let the admin to grant and revoke roles, not all role owners, how many times there were cases that role owners did malicious actions. However, I am here for a security of the space, not for arguing. Thank you.

[MxAxM]

Don't want to argue, and with all due respect, but if they are trusted why having revoke role functionality? Okay, the general admin is Trusted, so only let the admin to grant and revoke roles, not all role owners, how many times there were cases that role owners did malicious actions. However, I am here for a security of the space, not for arguing. Thank you.

I'm not here for arguing sir, but it's a general role: when admins are trusted we consider they won't perform malicious actions

[tpiliposian]

Dear friend,

I was aware about the role, but having a revoke role, means that there can be a situation when Trusted Alice should be revoked from Pausing role, right? So I am saying that yes, the general ADMIN is TRUSTED, only let him to grant and revoke roles.

[MxAxM]

Dear friend,

I was aware about the role, but having a revoke role, means that there can be a situation when Trusted Alice should be revoked from Pausing role, right? So I am saying that yes, the general ADMIN is TRUSTED, only let him to grant and revoke roles.

Trusted means admins know what they're doing and how protocol works and they don't perform malicious actions

[tpiliposian]

Ok, as you said.

[MxAxM]

Ok, as you said.

If you think my decision is wrong feel free to escalate it, but as i said it's not considered a valid issue

[tpiliposian]

I just decided to participate in the contest, but I don't have enough points to escalate the issue. I am a full-time employee at Hexens, and I believe that if there is any potential for manipulation, it should be reported. I have experience working in security, and I recognize this as a security issue. Once again, I understand that the admin is trusted, but that's why I am suggesting giving that functionality to the admin. It's not guaranteed that the admin can never make a mistake, and it's also uncertain whether those whom the admin grants roles to are always trustworthy. Therefore, I believe this is a valid issue and not against any rules ser.

[tpiliposian]

Ok, as you said.

If you think my decision is wrong feel free to escalate it, but as i said it's not considered a valid issue

If you could escalate this issue for me so the project team can review it, I would be very grateful.

[MxAxM]

Escalate

Please review the conversion

[sherlock-admin3]

Escalate

Please review the conversion

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[Afriaudit]

According to code implementation , Its impossible for anyone except default admin to grant or rekove role to someone else. Here is why;

• To grant and revoke roles the modifier onlyRole(getRoleAdmin(role) is checked: which basically checks the caller must have role's admin role.
• The role's admin of all admin role was not set which by default sets it to (0x00)
• In withmidasaccesscontrol.sol, bytes32 public constant DEFAULT_ADMIN_ROLE = 0x00 was declared
• During contracts initialization the _setuproles function implements _setupRole(DEFAULT_ADMIN_ROLE, admin) where admin is the address of default admin
• These sequence of logic implementaion makes it so that only address of the default admin can grant and revoke roles

[tpiliposian]

Thanks for the answer, but the MidasAccessControl.sol::grantRoleMult() is calling _revokeRole() not revokeRole() which has modifier: https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/4c078ae2ef7cb53fc48abb79da21eb1925f52dd0/contracts/access/AccessControlUpgradeable.sol#L159

here is the _revokeRole() internal function https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/4c078ae2ef7cb53fc48abb79da21eb1925f52dd0/contracts/access/AccessControlUpgradeable.sol#L223-L232

So what you described is actually what I am recommending! I am saying that ONLY that general admin should be able to grant and revoke roles, but in the implementation, besides general admin, the roles can be revoked by the role owners as well, which is not good!:

    function revokeRoleMult(bytes32[] memory roles, address[] memory addresses)
        external
    {
        require(roles.length == addresses.length, "MAC: mismatch arrays");
        address sender = msg.sender;

        for (uint256 i = 0; i < roles.length; i++) {
            _checkRole(getRoleAdmin(roles[i]), sender);
            _revokeRole(roles[i], addresses[i]);
        }
    }

[Afriaudit]

_checkRole(getRoleAdmin(roles[i]), sender); does same check onlyRole(getRoleAdmin(role) modifier. It ensures msg.sender is default admin else it reverts.

[tpiliposian]

No ser, it calls: https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/4c078ae2ef7cb53fc48abb79da21eb1925f52dd0/contracts/access/AccessControlUpgradeable.sol#L114-L118 which is again an internal function without a modifier, it checks whether the caller has that role or not. So if Alice has greenlisted role, she can grant her role to Bob.

[Afriaudit]

_checkRole(getRoleAdmin(roles[i]), sender) doesnt check if the caller has the role or not. It does the following;

• Firstly it does getRoleAdmin(roles[i]) which retrieves the admin role associated with the i-th role. This function is part of the AccessControlUpgradeable contract and returns the admin role that can manage (grant/revoke) the specified role. see here: https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/4c078ae2ef7cb53fc48abb79da21eb1925f52dd0/contracts/access/AccessControlUpgradeable.sol#L126
• Because role's admin of all admin role was not set it by default is (0x00).
• So _checkRole actually checks if the msg.sender has the admin role for the i-th role.
• So because DEFAULT_ADMIN_ROLE is set to 0x00 only default admin can actually grant or revoke the roles. I dnt know If my explanation was clear ser. Have a closer look especially at the getRoleAdmin(roles[i]) function am sure u will see what am trying to describe.

[tpiliposian]

But he admins for BLACKLISTED_ROLE and GREENLISTED_ROLE is not our default admin, it is being set during deployment, and I read in one of the valid issues that the admin role is trusted except for mentioned two roles:

        _setRoleAdmin(BLACKLISTED_ROLE, BLACKLIST_OPERATOR_ROLE);
        _setRoleAdmin(GREENLISTED_ROLE, GREENLIST_OPERATOR_ROLE);

[Afriaudit]

okay.. So in OpenZeppelin's AccessControlUpgradeable, Each role is represented by a unique bytes32 identifier. Roles and their associated data are stored in mappings within the contract.

 mapping(bytes32 => RoleData) private _roles;

// where RoleData is;
struct RoleData {
        mapping(address => bool) members;
        bytes32 adminRole;
    }

members: A mapping of addresses to boolean values indicating whether an address has the role. adminRole: The bytes32 identifier of the admin role that can manage this role.

_setRoleAdmin(BLACKLISTED_ROLE, BLACKLIST_OPERATOR_ROLE);
 _setRoleAdmin(GREENLISTED_ROLE, GREENLIST_OPERATOR_ROLE);

what the function above does is set adminRole for BLACKLISTED_ROLE to be BLACKLIST_OPERATOR_ROLE(same for GREENLISTED). The BLACKLIST_OPERATOR_ROLE can now grant and revoke the BLACKLISTED_ROLE to/from other addresses.

However remember that the role BLACKLIST_OPERATOR_ROLE never had its own adminRole set which makes it default to (0x00) and be cause DEFAULT_ADMIN_ROLE is set to 0x00 only default admin can actually grant or revoke that role.

so a BLACKLISTED_ROLE is a user that has been backlisted the adminRole for that is BLACKLIST_OPERATOR_ROLE (which is one of the admins) so only him can grant and revoke the role BLACKLISTED_ROLE. However BLACKLIST_OPERATOR_ROLE has adminRole of (0x00) so only DEFAULT_ADMIN_ROLE can grant and revoke that role.

This is how best I can explain It ser. and this is how best I understand it. will leave it here for HOJ to throw more light. Good luck ser!!

[tpiliposian]

Thank you for the detailed response. What I was saying is that, yes, GREENLIST_OPERATOR_ROLE can grant the GREENLISTED_ROLE to others. Essentially, I was highlighting that all role admins should be our trusted default admin.

Best of luck to you too, ser. Let's wait for the HOJ's input.

[Afriaudit]

All adminRole at the moment though is entrusted to default admin(0x00) with exception to GREENLISTED_ROLE and BLACKLISTED_ROLE which are user roles and not admin roles and if their adminRole is not set to their respective GREENLIST_OPERATOR_ROLE and BLACKLIST_OPERATOR_ROLE they wnt be able to greenlist and blacklist users respectively

[WangSecurity]

Firstly, thank you very much for such discussion without arguing. Glad to see our community to be professional, mature and calm, thank you very much.

Secondly, clarification, GREENLISTED_ROLE and BLACKLISTED_ROLE cannot call revokeRoleMulti themselves, correct? It checks if the msg.sender is an admin for these roles, and if not, then the transaction reverts, correct?

[tpiliposian]

Hey, thank you for the reply.

Yeah, exactly, it can be called by GREENLIST_OPERATOR_ROLE and BLACKLIST_OPERATOR_ROLE role owners.

[WangSecurity]

Then, as I understand the system works as intended and correctly, please correct me if wrong.

Planning to reject the escalation and leave the issue as it is.

[tpiliposian]

If GREENLIST_OPERATOR_ROLE and BLACKLIST_OPERATOR_ROLE role owners are also trusted, then I have no other concerns.

[WangSecurity]

Yes, these two roles are considered trusted based on the README.

[WangSecurity]

Result: Invalid Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• MxAxM: rejected