Super safe can restrict ROOT to change him

[hats-bug-reporter[bot]]

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

Description: Description\ Root can set SAFE to be SUPER, and the SUPER can add children.

ROOT can also remove the SUPER of any child with updateSuper(), allowing the ROOT to update the child SUPER (Father).

But SUPER can front run updateSuper() call by adding a lot SAFEs as children since there is no limit. Which will cause updateSuper() to run out of gas and SUPER cannot be removed.

Attack Scenario\

1. ROOT adds SUPER
2. SUPER adds 10 safes
3. SUPER act bad for 1 of the SAFES or SAFE just want to have different SUPER now.
4. ROOT calls updateSuper() with the new SUPER
5. SUPER front-run the tx by adding multiple safes
6. updateSuper() will fails when tries to find the child inside the oldSuper
7. oldSuper will always stay this child's SUPER

function updateSuper(uint256 safeId, uint256 newSuperId)
        external
        IsRootSafe(_msgSender())
        SafeIdRegistered(newSuperId)
        requiresAuth
    {
        bytes32 org = getOrgBySafe(safeId);
        address caller = _msgSender();
        /// RootSafe usecase : Check if the safe is Member of the Tree of the caller (rootSafe)
        if (!isRootSafeOf(caller, safeId)) {
            revert Errors.NotAuthorizedUpdateNonChildrenSafe();
        }
        // Validate are the same org
        if (org != getOrgBySafe(newSuperId)) {
            revert Errors.NotAuthorizedUpdateSafeToOtherOrg();
        }
        /// Check if the new Super Safe is Reached Depth Tree Limit
        if (isLimitLevel(newSuperId)) {
            revert Errors.TreeDepthLimitReached(depthTreeLimit[org]);
        }
        DataTypes.Safe storage _safe = safes[org][safeId];
        /// SuperSafe is either an Org or a Safe
        DataTypes.Safe storage oldSuper = safes[org][_safe.superSafe];

        /// Remove child from superSafe
        for (uint256 i; i < oldSuper.child.length;) {
            if (oldSuper.child[i] == safeId) {
                oldSuper.child[i] = oldSuper.child[oldSuper.child.length - 1];
                oldSuper.child.pop();
                break;
            }
            unchecked {
                ++i;
            }
        }
        RolesAuthority _authority = RolesAuthority(rolesAuthority);
        /// Revoke SuperSafe and SafeLead if don't have any child, and is not organisation
        if (oldSuper.child.length == 0) {
            _authority.setUserRole(
                oldSuper.safe, uint8(DataTypes.Role.SUPER_SAFE), false
            );
        }

        /// Update safe superSafe
        _safe.superSafe = newSuperId;
        DataTypes.Safe storage newSuperSafe = safes[org][newSuperId];
        /// Add safe to new superSafe
        /// Give Role SuperSafe if not have it
        if (
            !_authority.doesUserHaveRole(
                newSuperSafe.safe, uint8(DataTypes.Role.SUPER_SAFE)
            )
        ) {
            _authority.setUserRole(
                newSuperSafe.safe, uint8(DataTypes.Role.SUPER_SAFE), true
            );
        }
        newSuperSafe.child.push(safeId);
        emit Events.SafeSuperUpdated(
            org,
            safeId,
            _safe.lead,
            caller,
            getSafeIdBySafe(org, oldSuper.safe),
            newSuperId
        );
    }

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional) Limit the children count that every SUPER can add, and allow every Child to change its SUPER, that will remove the for loop

[alfredolopez80]

Non-Issue, is the SuperSafe act bad, the RootSafe can updateSuper of all childSafe of this bad actor (SuperSafe), and after removeSafe and avoid any malicious action/behavior over the onChainOrganization.