Safe owner/s can prevent being removed from organization by indefinitely increasing their child array

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x8f22a5576088f17cc9d92e7f2f7708511899d90445a9ccaa2db87c91912da884 Severity: medium

Description: Description\ Palmera module allow organizations to orchestrate different safes in hierarchy, where super safe can remove it's children. One problem here is that any arbitrary safe can become child of any superSafeId by calling addSafe. This will increment superSafeOrgSafe.child array. There is no limit on how many child a super safe can have (the limit is only for depth), but a child may have an unlimited amount of siblings. Now lets see how a safe is removed from an organization:

 function removeSafe(uint256 safeId)
        public
        SafeRegistered(_msgSender())
        requiresAuth
    {
        address caller = _msgSender();
        bytes32 org = getOrgHashBySafe(caller);
        uint256 callerSafe = getSafeIdBySafe(org, caller);
        uint256 rootSafe = getRootSafe(safeId);
         ...
        /// Remove child from superSafe
        for (uint256 i; i < superSafe.child.length;) {
            if (superSafe.child[i] == safeId) {
                superSafe.child[i] = superSafe.child[superSafe.child.length - 1];
                superSafe.child.pop();
                break;
            }
            unchecked {
                ++i;
            }
        }
        // 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]];
            // Update children safe superSafe reference
            childrenSafe.superSafe = _safe.superSafe;
            unchecked {
                ++i;
            }
        }
     ...

We can notice that we have to find corresponding safeIdin child array of the father and also change superSafe property of all children of the removed safe. Now as this is very gas intensive operation, because it is reading/writing from/to storage in a loop, which an arbitrary user can increase indefinitely, it is a problem and potential DoS of removing a safe, because of OOG error. Attack Scenario\

1. Organization A creates a root safe and adds Safe B as a chield.
2. Some times passes and A has created some siblings of B and want to remove B, because it has become malicious.
3. But B has already called addSafe with it's Id from 1_000_000 addresses and when removeSafe transaction hits execution, it will try to make those 1_000_000 addresses children of A, but this would be a very large loop with guaranteed OOG error.
4. As a result organization A is compromised

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional) Implement a limit of safe.child array and check it when new safe is integrated.

[alfredolopez80]

Can you clarify that, because the safe.child Array storage the id's of safe, so if B is already called addSafe method and add the new safe with id 1_000_000, in the loop of removeSafe, is only a unique iteration additionally not 1 million?? so i don't understand the PoC?

[NicolaMirchev]

Can you clarify that, because the safe.child Array storage the id's of safe, so if B is already called addSafe method and add the new safe with id 1_000_000, in the loop of removeSafe, is only a unique iteration additionally not 1 million?? so i don't understand the PoC?

Hey, @alfredolopez80, sure I will explain

I have described that the exploiter could add 1_000_000 children by calling addSafe 1_000_000 times from different safes. with IT'S ID FROM 1_000_000 addresses That would make the iteration inside removeSafe indefinitely large.

[batmanBinary]

the addSafe function includes a check with isLimitLevel to ensure the super safe hasn't reached its depth tree limit:

 // check if the superSafe Reached Depth Tree Limit
        if (isLimitLevel(superSafeId)) {
            revert Errors.TreeDepthLimitReached(depthTreeLimit[org]);
        }

This prevents adding an excessive number of safes. ??

[NicolaMirchev]

Hey, @batmanBinary

This check only ensures that the depth limit is not reached. Here the problem is that we add saves by width. Currently, one can add indefinitely siblings to safe with depth 1 for example.