A safe will still have SUPER_SAFE ROLE after exit

[hats-bug-reporter[bot]]

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

Description: Description\ By design, afer a safe is removed from an org, it should not have any role and this was implemented partialy in the removeWholeTree(), all roles were revoked before making a call to _exitSafe() to delete the safe struct and remove the safe from the org safe array.

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
            ); //revoke super_safe roles
            // Disable safe lead role
   @>         disableSafeLeadRoles(safes[org][safe].safe); //revoke all lead roles
            _exitSafe(safe);
            unchecked {
                ++j;
            }
        }

After all children of the root were removed, the root safe was then removed

// After Disconnect Root Safe
        emit Events.WholeTreeRemoved(
            org, rootSafe, caller, safes[org][rootSafe].name
        );
@>      _exitSafe(rootSafe);
        if (indexSafe[org].length == 0) removeOrg(org);
    }

The problem here is that super safe role of the root was not revoked with the assupmtion that root does not have a super safe role, however, it is possible for a root safe to have a super safe root.

Attack Scenario\

1. safe A is a super safe under root A
2. root A then promoted safe A to root but still has super safe role
3. The entire tree was then removed but the root B safe address still has the super safe role.

Attachments

1. Proof of Concept (PoC) File The promoteRoot() is a method used by a root to promote a superSafe to become a root safe.

It was first confirmed that the safe to be promoted is a super safe by checking that it's child array is greater than zero, and whenever the child of a safe is greater than zero, such safe is granted super safe role.

This function does not revoke the super safe role of the safe to be promoted before granting it root role

 function promoteRoot(uint256 safeId)
        external
        IsRootSafe(_msgSender())
        SafeIdRegistered(safeId)
        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();
        }
        DataTypes.Safe storage newRootSafe = safes[org][safeId];
        /// Check if the safe is a Super Safe, and an Direct Children of thr Root Safe
        if (
            (newRootSafe.child.length <= 0)
                || (!isSuperSafe(getSafeIdBySafe(org, caller), safeId))
        ) {
            revert Errors.NotAuthorizedUpdateNonSuperSafe();
        }
        /// Give Role RootSafe if not have it
        RolesAuthority _authority = RolesAuthority(rolesAuthority);
        _authority.setUserRole(
            newRootSafe.safe, uint8(DataTypes.Role.ROOT_SAFE), true
        );
        // Update Tier
        newRootSafe.tier = DataTypes.Tier.ROOT;
        // Update Root Safe
        newRootSafe.lead = address(0);
        newRootSafe.superSafe = 0;
        emit Events.RootSafePromoted(
            org, safeId, caller, newRootSafe.safe, newRootSafe.name
        );
    }

No where in the snippet above was the super safe role revoked, and when the removeWholeTree() was called, the root safe of a promoted safe will still have a super safe role after removal.

2. Revised Code File (Optional) Revole the super safe role of a safe before granting it the root safe role

[alfredolopez80]

This issue is invalid, because three things to clarify:

1. All RootSafe by default need to have the SuperSafe Role, this is an expected behavior!!
2. if you check the code of disconnectSafe() always need to removeSafe() before, soo, in removeSafe() always remore SuperSafe Role!!
3. The scenario of attack is invalid because the point 1.

@0xmahdirostami and @0xRizwan i wait your comments

[0xmahdirostami]

invalid

[lanrebayode]

@0xmahdirostami @0xRizwan @alfredolopez80 I think this issue should be re-evaluated, considering the fact that after the exit of a safe, it can be added to another org, where it has an instant superSafe role.

This issue is about a safe having super-safe role after exit. This is similar issue to #72

thanks

[lanrebayode]

The problem here is that super safe role of the root was not revoked with the assupmtion that root does not have a super safe role, however, it is possible for a root safe to have a super safe root.

I stated a root could have a super safe role, and that the super safe role was not revoked despite exiting the org, so the safe retains a super safe role if added to a new org without any child!

[lanrebayode]

However, the correct fix will be to revoke the super-safe role before exiting.

[lanrebayode]

All RootSafe by default need to have the SuperSafe Role, this is an expected behavior!!

Agreed!

if you check the code of disconnectSafe() always need to removeSafe() before, soo, in removeSafe() always remore SuperSafe Role!!

No where in the removeWholeTree() was disconnectSafe() called on the rootSafe!

 /// @notice Remove whole tree of a RootSafe
    /// @dev Remove whole tree of a RootSafe
    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);
    }

The entire tree was then removed but the root B safe address still has the super safe role.

this still holds!

Safe is added to a new org as a child, but has super safe role with zero child!

[0xmahdirostami]

After a discussion with @alfredolopez80, we decided this issue is a VALID ISSUE.