hats-finance / Palmera-0x5fee7541ddcd51ba9f4af606f87b2c42eea655be

Palmera hierarchical module
0 stars 4 forks source link

Safe of higher hierarchy than a safe's super_Safe cannot remove safe #93

Open hats-bug-reporter[bot] opened 3 months ago

hats-bug-reporter[bot] commented 3 months ago

Github username: @lanrebayode Twitter username: lanrebayode1 Submission hash (on-chain): 0xef148c5ae22406067becec4d500dbb0436cf1d19138284868b16050bf96b95f7 Severity: medium

Description: Description\ Palmera allows organization to run on chain based on preset hierarchycal structure.

So there is a rootSafe(boss safe) and children that are super safes to many others which could be super safe for other safes too.

ROOT super_Safe
sub-superSafe sub-sub-supersafe

child

Actions that can be perfomoed in the org by each safe is dependednt of the safe's role.

removeSafe() is a method that allows the root safe or super safe of a safe to remove a safe from the org.

What happens after the removal of the safe is that, if the removed safe has children, it's children is transferred to the current super safe

        // Handle child from removed safe
        for (uint256 i; i < _safe.child.length;) {
            // Add removed safe child to superSafe
            superSafe.child.push(_safe.child[i]);
   @>      DataTypes.Safe storage childrenSafe = safes[org][_safe.child[i]]; // @audit changing the super safe of the child to the super safe of the safe being removed 
            // Update children safe superSafe reference
            childrenSafe.superSafe = _safe.superSafe;
            unchecked {
                ++i;
            }
        }

The promblem with this method is that it does not allow a safe higher than the super safe of the safe in the tree to call this method.

Attack Scenario\ If A is the super safe of B, and B is the super safe of C, only B and root safe(Let call it Z) can remove C directly by calling removeSafe() with C. For A to remove C, it must need to first remove B(an action that disrupts org structure), when all the children of B is then added to A directly, that is when it can now remove C.

The above scenerio breaks a fundamental functionality that should be permitted.

Attachments

  1. Proof of Concept (PoC) File
    // SuperSafe usecase : Check caller is superSafe of the safe
        if (
            (!isRootSafeOf(caller, safeId))
                && (!isSuperSafe(callerSafe, safeId))
        ) { //@audit this check is not robust enogh to permit super safe of highter order 
            revert Errors.NotAuthorizedAsNotRootOrSuperSafe();
        }

    The check above will not permit a higher order super safe, until the direct super safe of the safe is first removed.

  2. Revised Code File (Optional) Check that the caller is either the root, direct super safe or in higher rank than the super safe in the same tree.
alfredolopez80 commented 2 months ago

is not issue, is a Expected Behavior, only the RootSafe can do that, the Super_Safe, only can remove their direct child's and let me clarify why:

  1. Any SuperSafe can't remove if not before updateSuper(), of childer of this SuperSafe, this is the reason we create the method updateSuper(), so after you update the supersafe you can remove this safe safety!!, without any revert!!

tha exception of this is removeWholeTree but only the rootSafe can call this method!! @0xmahdirostami and @0xRizwan pls check and let me know any comments