hats-finance / Palmera-0x5fee7541ddcd51ba9f4af606f87b2c42eea655be

Palmera hierarchical module
0 stars 1 forks source link

removeWholeTree() will run out of gas #53

Open hats-bug-reporter[bot] opened 1 week ago

hats-bug-reporter[bot] commented 1 week ago

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

Description: Description\ Palmera allows Safes to add children in a hierarchy, but there is no limit on how much children can be added.

A problem will arise then any of the ROOT safes tries to call removeWholeTree(). This will get all safes that are succession of the calling ROOT. But the function will loop through all SAFE which will run out of gas.

Attack Scenario\

    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);
        if (indexSafe[org].length == 0) removeOrg(org);
    }
  1. There is a ROOT with some children
  2. One of the children add 100+ SAFE
  3. Another child add 200 more
  4. When the ROOT safe calls removeWholeTree(), the function will run out of gas.

Attachments

  1. Proof of Concept (PoC) File

  2. Revised Code File (Optional) Like the tree depth, restrict the amount of children that any SAFE can have.

alfredolopez80 commented 1 week ago

Non-Issue, we have a stress-test in we can create a Onchain-Organization with 8100 Safes with any run out gas problem, additional we limit the maxdeepTreelimit to 50, to avoid this situation!!

i invite to check this stress test in the test folder, was writed in foundry