Hardcoded level inside of `_seekMember` causes discrepancy

[hats-bug-reporter[bot]]

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

Description:

Description

inside function isLimitLevel function _seekMember is called:

    function isLimitLevel(uint256 superSafeId) public view returns (bool) {
        if ((superSafeId == 0) || (superSafeId > indexId)) return false;
        bytes32 org = getOrgBySafe(superSafeId);
->        (, uint256 level,) = _seekMember(indexId + 1, superSafeId);
        return level >= depthTreeLimit[org];
    }

a bool will be returned, essentially validating if the depthTreeLimit has been reached.

    function _seekMember(uint256 superSafeId, uint256 childSafeId)
        private
        view
        returns (bool isMember, uint256 level, uint256 rootSafeId)
    {
        bytes32 org = getOrgBySafe(childSafeId);
        DataTypes.Safe memory _childSafe = safes[org][childSafeId];
        // Check if the Child Safe is was removed or not Exist and Return False
        if (
            (_childSafe.safe == address(0))
                || (_childSafe.tier == DataTypes.Tier.REMOVED)
        ) {
            return (isMember, level, rootSafeId);
        }
        // Storage the Root Safe Address in the next superSafe is Zero
        rootSafeId = _childSafe.superSafe;
        uint256 currentSuperSafe = rootSafeId;
->        level = 2; // Level start in 1
        while (currentSuperSafe != 0) {
            _childSafe = safes[org][currentSuperSafe];
            // Validate if the Current Super Safe is Equal the SuperSafe try to Found, in case is True, storage True in isMember
            isMember =
                !isMember && currentSuperSafe == superSafeId ? true : isMember;
            // Validate if the Current Super Safe of the Child Safe is Equal Zero
            // Return the isMember, level and rootSafeId with actual value
            if (_childSafe.superSafe == 0) return (isMember, level, rootSafeId);
            // else update the Vaule of possible Root Safe
            else rootSafeId = _childSafe.superSafe;
            // Update the Current Super Safe with the Super Safe of the Child Safe
            currentSuperSafe = _childSafe.superSafe;
            // Update the Level for the next iteration
            unchecked {
->                ++level;
            }
        }
    }

Inside this function the level starts counting from 2, it loops for every safe from an [org].

Now lets assume the following scenario:

• depthTreeLimit = 10
• there are 9 safes inside the org
• isLimitLevel is called which calls _seekMember
• _seekMember will iterate safes amount of times
• because level = 2, after 9 iterations this will be 11
• the return statement inside isLimitLevel will be true because: return level >= depthTreeLimit[org];
• Note that this statement will also be true if there are 8 safes, since level will be 10, which will trigger the = inside return level >= depthTreeLimit[org];.

now inside addSafe the following if statement will trigger:

    function addSafe(uint256 superSafeId, string memory name)
        external
        SafeIdRegistered(superSafeId)
        IsSafe(_msgSender())
        returns (uint256 safeId)
    {
        // check the name of safe is not empty
        if (bytes(name).length == 0) revert Errors.EmptyName();
        bytes32 org = getOrgBySafe(superSafeId);
        address caller = _msgSender();
        if (isSafeRegistered(caller)) {
            revert Errors.SafeAlreadyRegistered(caller);
        }
        // check to verify if the caller is already exist in the org
        if (isTreeMember(superSafeId, getSafeIdBySafe(org, caller))) {
            revert Errors.SafeAlreadyRegistered(caller);
        }
        // check if the superSafe Reached Depth Tree Limit
->        if (isLimitLevel(superSafeId)) {
            revert Errors.TreeDepthLimitReached(depthTreeLimit[org]);
        }

The problem is that the limit has been reached because level is set to 2 inside _seekMember, which is checked against depthTreeLimit[org which does not start at 2.

    mapping(bytes32 => uint256) public depthTreeLimit;

Nowhere in the contract is the depthTreeLimit hardcoded to start at 2.

Ultimately addSafe will trigger the TreeDepthLimitReached while in reality it has not.

Tools Used

Manual Review

Recommendation

Keep consistency with the start of the indexes, make sure the variable level is checked against also starts at the same index.

[alfredolopez80]

Non-issue, i guess not clear about how _seekMember will iterate into the leaf of the superSafeId need to evalute, the account you need to verify if this superSafeId reach the depthTreeLimit of the org, and this iterate if from the more down childSafe of the Tree, to the RootSafe, and when you check the superSafeId you check start in the level 1, and before the while you check if you found the RootSafe or not!!, if not your start to check the level to and go on!!

so i guess is not clear how _seekMember worked!!, but @0xRizwan and @0xmahdirostami take a look and let me your comments

Additional i guess is important to clarify the safe id not neccesary to be consecutive, for be part of the same tree!! and the level analysis into this leaves if from the down to the top!!

RootSafe	SuperSafe

         SubSuperSafe
                           |
                       SubSubSuperSfe
                                              |
                                            .....
                                              |
                                          ChildSafe (in your use case the new safe to try to join the OnChain Org)