Unauthorized Addition of any Safe to Organization

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x502694a977b0b4659d5341adbcaabf4be8294cbd97fd5baa9abf6df97349c7a5 Severity: high

Description: Description\ The addSafe function in the PalmeraModule contract allows any safe to call this function and add itself to an organization, provided it has a valid superSafeId. This lack of restriction can lead to unauthorized safes being added to an organization, potentially compromising the security and integrity of the organization. Attack Scenario\ An attacker can exploit this vulnerability by calling the addSafe function with a valid superSafeId and adding their own safe to the organization. This can be done without any checks on the caller's permissions, allowing the attacker to become part of the organization without proper authorization.

Attachments

1. Proof of Concept (PoC) File

• Deploy the PalmeraModule contract.
• Assume superSafeId is a valid ID(like this can be done using frontrunning the addSafe function)
• An unauthorized safe (attacker's safe) calls the addSafe function with the superSafeId and a name for the new safe.
• The attacker's safe is now added to the organization without any authorization checks.

 function addSafe(uint256 superSafeId, string memory name)
        external
        SafeIdRegistered(superSafeId)
        IsSafe(_msgSender())
        returns (uint256 safeId)
    {//@audit-no authorization
        // check the name of safe is not empty
        if (bytes(name).length == 0) revert Errors.EmptyName();
        bytes32 org = getOrgBySafe(superSafeId);
        address caller = _msgSender();
        if (isSafeRegistered(caller)) {
            revert Errors.SafeAlreadyRegistered(caller);
        }
        // check to verify if the caller is already exist in the org
        if (isTreeMember(superSafeId, getSafeIdBySafe(org, caller))) {
            revert Errors.SafeAlreadyRegistered(caller);
        }
        // check if the superSafe Reached Depth Tree Limit
        if (isLimitLevel(superSafeId)) {
            revert Errors.TreeDepthLimitReached(depthTreeLimit[org]);
        }
        /// Create a new safe
        DataTypes.Safe storage newSafe = safes[org][indexId];
        /// Add to org root/safe
        DataTypes.Safe storage superSafeOrgSafe = safes[org][superSafeId];
        /// Add child to superSafe
        safeId = indexId++;
        superSafeOrgSafe.child.push(safeId);

        newSafe.safe = caller;
        newSafe.name = name;
        newSafe.superSafe = superSafeId;
        indexSafe[org].push(safeId);
        /// Give Role SuperSafe
        RolesAuthority _authority = RolesAuthority(rolesAuthority);
        if (
            (
                !_authority.doesUserHaveRole(
                    superSafeOrgSafe.safe, uint8(DataTypes.Role.SUPER_SAFE)
                )
            ) && (superSafeOrgSafe.child.length > 0)
        ) {
            _authority.setUserRole(
                superSafeOrgSafe.safe, uint8(DataTypes.Role.SUPER_SAFE), true
            );
        }

        emit Events.SafeCreated(
            org, safeId, newSafe.lead, caller, superSafeId, name
        );
    }

2. Revised Code File (Optional)

   To fix this issue, add a check to ensure that only authorized roles (e.g., root safe or super safe) can call the addSafe function

[alfredolopez80]

duplicate #24 @0xmahdirostami @0xRizwan

[AresAudits]

I would like to clarify that issue #87 is not a duplicate of issue #24.

• Issue #87 addresses the lack of authorization checks in the addSafe function, allowing any valid safe to add itself to any organization without proper permissions.
  • Issue #24 focuses on bypassing isSafe checks by creating a custom contract that mimics a safe, allowing unauthorized contracts to exploit the system.

These issues are distinct and address different security vulnerabilities

[0xmahdirostami]

thank you i think it's duplicate of issue #46. the root casue of this issue: addresses the lack of authorization checks in the addSafe function, allowing any valid safe to add itself to any organization without proper permissions.

issue #46, The function allows the caller (a safe) to add it to an organisation.

Given this, anyone can stuff any given org to the TreeDepthLimit with Safes they don't want.

There is no issue with this, the new added safe does not have any control over other safes, and the root and super safe do have control over it.