Missing `disableSafeLeadRoles` Call for Root in `removeWholeTree` Function**

[hats-bug-reporter[bot]]

Github username: @0xmahdirostami Twitter username: 0xmahdirostami Submission hash (on-chain): 0xaad7afa14366c6b8c37347f008d96027c424ac49bfd1b2a4529532c1c739661f Severity: medium

Description: Description:

In the removeWholeTree function, the disableSafeLeadRoles function is not called for the root safe. The relevant code snippet is:

            disableSafeLeadRoles(safes[org][safe].safe);
            _exitSafe(safe);
            unchecked {
                ++j;
            }
        }
        // After Disconnect Root Safe
        _exitSafe(rootSafe);

This snippet shows that disableSafeLeadRoles is called for all safes but not for the root safe.

Impact:

• The lack of disabling lead roles for the root will cause issues, especially if the root safe becomes a member in another organization. This can result in unexpected behavior and potential security risks, as the root safe will retain roles that should have been disabled.

Mitigation:

To mitigate this issue, ensure that disableSafeLeadRoles is also called for the root safe. The corrected code should look like this:

            disableSafeLeadRoles(safes[org][safe].safe);
            _exitSafe(safe);
            unchecked {
                ++j;
            }
        }
        // After Disconnect Root Safe
        emit Events.WholeTreeRemoved(
            org, rootSafe, caller, safes[org][rootSafe].name
        );
+        disableSafeLeadRoles(rootSafe);
        _exitSafe(rootSafe);
        if (indexSafe[org].length == 0) removeOrg(org);

This ensures that lead roles are disabled for the root safe, preventing any issues or security risks associated with the root safe retaining roles it should no longer have.

[alfredolopez80]

@0xmahdirostami if you check this for loop https://github.com/hats-finance/Palmera-0x5fee7541ddcd51ba9f4af606f87b2c42eea655be/blob/1ac35880b5d45154267788e2db548eaaae0beaa0/src/PalmeraModule.sol#L523, This runs through the organization's entire safeId array that include the RootSafe

So, this issue like you explain is not valid

[0xmahdirostami]

@alfredolopez80, thank you, but the issue is valid.

[0xmahdirostami]

@alfredolopez80 @0xRizwan ,the root safe is not in loop. add the console log to code.

function removeWholeTree() external IsRootSafe(_msgSender()) requiresAuth {
        address caller = _msgSender();
        bytes32 org = getOrgHashBySafe(caller);
        uint256 rootSafe = getSafeIdBySafe(org, caller);
        uint256[] memory _indexSafe = getTreeMember(rootSafe, indexSafe[org]);
        RolesAuthority _authority = RolesAuthority(rolesAuthority);
        for (uint256 j; j < _indexSafe.length;) {
            uint256 safe = _indexSafe[j];
            DataTypes.Safe memory _safe = safes[org][safe];
            // Revoke roles to safe
            _authority.setUserRole(
                _safe.safe, uint8(DataTypes.Role.SUPER_SAFE), false
            );
            // Disable safe lead role
            disableSafeLeadRoles(safes[org][safe].safe);
+            console.log("disconnect", safe);
            _exitSafe(safe);
            unchecked {
                ++j;
            }
        }
        // After Disconnect Root Safe
        emit Events.WholeTreeRemoved(
            org, rootSafe, caller, safes[org][rootSafe].name
        );
        _exitSafe(rootSafe);
        if (indexSafe[org].length == 0) removeOrg(org);
    }

test:

    function testEventWhenRemoveWholeTree() public {
        (
            uint256 rootId,
            uint256 safeIdA1,
            uint256 subSafeIdA1,
            uint256 subSubSafeIdA1
        ) = palmeraSafeBuilder.setupOrgFourTiersTree(
            orgName, safeA1Name, subSafeA1Name, subSubSafeA1Name
        );

        address rootAddr = palmeraModule.getSafeAddress(rootId);
        vm.startPrank(rootAddr);
        vm.expectEmit(true, true, true, true);
        emit WholeTreeRemoved(
            keccak256(abi.encodePacked(orgName)), rootId, rootAddr, orgName
        );
        palmeraModule.removeWholeTree();
    }

Logs:

Ran 1 test for test/EventsChecker.t.sol:EventsChekers
[PASS] testEventWhenRemoveWholeTree() (gas: 2535909)
Logs:
  disconnect 2
  disconnect 3
  disconnect 4

As it shows, root is not in loop, so the issue is valid

[alfredolopez80]

is a valid issue

[AlexAurthur]

@alfredolopez80 @0xRizwan,

This issue is invalid .the root safe role is removed at the end of the removeWholeTree() by calling _exitSafe(rootSafe); function.

/// @notice Remove whole tree of a RootSafe
    /// @dev Remove whole tree of a RootSafe
    function removeWholeTree() external IsRootSafe(_msgSender()) requiresAuth {
        address caller = _msgSender();
        bytes32 org = getOrgHashBySafe(caller);
        uint256 rootSafe = getSafeIdBySafe(org, caller);
        uint256[] memory _indexSafe = getTreeMember(rootSafe, indexSafe[org]);
        RolesAuthority _authority = RolesAuthority(rolesAuthority);
        for (uint256 j; j < _indexSafe.length;) {
            uint256 safe = _indexSafe[j];
            DataTypes.Safe memory _safe = safes[org][safe];
            // Revoke roles to safe
            _authority.setUserRole(
                _safe.safe, uint8(DataTypes.Role.SUPER_SAFE), false
            );
            // Disable safe lead role
            disableSafeLeadRoles(safes[org][safe].safe);
            _exitSafe(safe);
            unchecked {
                ++j;
            }
        }
        // After Disconnect Root Safe
        emit Events.WholeTreeRemoved(
            org, rootSafe, caller, safes[org][rootSafe].name
        );
        _exitSafe(rootSafe);<-----------------------------------------------------here exitsafe is called to remove rootsafe
        if (indexSafe[org].length == 0) removeOrg(org);
    }

the _exitSafe(rootSafe) function removes root safe by

removeIndexSafe(org, safeId);
        delete safes[org][safeId];

below is the _exitSafe() function

    /// @dev Function for refactoring DisconnectSafe Method, and RemoveWholeTree in one Method
    /// @param safeId ID's of the organisation
    function _exitSafe(uint256 safeId) private {
        bytes32 org = getOrgBySafe(safeId);
        address _safe = safes[org][safeId].safe;
        address caller = _msgSender();
        ISafe safeTarget = ISafe(_safe);
        removeIndexSafe(org, safeId);
        delete safes[org][safeId];<--------------------------------------by here the root safe will be removed

        /// Disable Guard
        bytes memory data = abi.encodeCall(ISafe.setGuard, (address(0)));
        /// Execute transaction from target safe
        _executeModuleTransaction(_safe, data);

        /// Disable Module
        address prevModule = getPreviewModule(caller);
        if (prevModule == address(0)) {
            revert Errors.PreviewModuleNotFound(_safe);
        }
        data = abi.encodeCall(ISafe.disableModule, (prevModule, address(this)));
        /// Execute transaction from target safe
        _executeModuleTransaction(_safe, data);

        emit Events.SafeDisconnected(org, safeId, address(safeTarget), caller);
    }

as mentioned by the @0xmahdirostami ,

the disableSafeLeadRoles function is not called for the root safe

Root safe cannot be removed by calling disableSafeLeadRoles.as the disableSafeLeadRoles is designed to remove SAFE_LEAD || SAFE_LEAD_EXEC_ON_BEHALF_ONLY || SAFE_LEAD_MODIFY_OWNERS_ONLY not the Root Safe

 /// @notice disable safe lead roles
    /// @dev Associated roles: SAFE_LEAD || SAFE_LEAD_EXEC_ON_BEHALF_ONLY || SAFE_LEAD_MODIFY_OWNERS_ONLY
    /// @param user Address of the user to disable roles
    function disableSafeLeadRoles(address user) private {
        RolesAuthority _authority = RolesAuthority(rolesAuthority);
        if (_authority.doesUserHaveRole(user, uint8(DataTypes.Role.SAFE_LEAD)))
        {
            _authority.setUserRole(user, uint8(DataTypes.Role.SAFE_LEAD), false);
        } else if (
            _authority.doesUserHaveRole(
                user, uint8(DataTypes.Role.SAFE_LEAD_EXEC_ON_BEHALF_ONLY)
            )
        ) {
            _authority.setUserRole(
                user, uint8(DataTypes.Role.SAFE_LEAD_EXEC_ON_BEHALF_ONLY), false
            );
        } else if (
            _authority.doesUserHaveRole(
                user, uint8(DataTypes.Role.SAFE_LEAD_MODIFY_OWNERS_ONLY)
            )
        ) {
            _authority.setUserRole(
                user, uint8(DataTypes.Role.SAFE_LEAD_MODIFY_OWNERS_ONLY), false
            );
        }
    }

as you can see in the above function there is no check for root safe.here this function is used to remove only the Associated roles: SAFE_LEAD || SAFE_LEAD_EXEC_ON_BEHALF_ONLY || SAFE_LEAD_MODIFY_OWNERS_ONLY and root safe is removed by calling _exitSafe(rootSafe);

[AlexAurthur]

@alfredolopez80 @0xRizwan @fonstack ,please verify the above comment.

[AlexAurthur]

This issue is invalid

[AlexAurthur]

@fonstack @jellegerbrandy ,this issue is invalid as i explained in the above comments.can u please verify this issue ,as not verifying it affects other auditors payout too.

please comment your thought on this issue.and also my comment