hats-finance / Palmera-0x5fee7541ddcd51ba9f4af606f87b2c42eea655be

Palmera hierarchical module
0 stars 4 forks source link

depthtreelimit can not be updated because different key is used #94

Open hats-bug-reporter[bot] opened 2 months ago

hats-bug-reporter[bot] commented 2 months ago

Github username: @whoismxuse Twitter username: -- Submission hash (on-chain): 0x45a712b149b87a6ef2194349dc213ab14a02bdaed9385f6d2a4f979d0d19bf34 Severity: medium

Description: inside PalmeraModule.sol depthTreeLimit[org] can be updated by calling the following function:

    function updateDepthTreeLimit(uint256 newLimit)
        external
        IsRootSafe(_msgSender())
        requiresAuth
    {
        address caller = _msgSender();
        bytes32 org = getOrgHashBySafe(caller);
        uint256 rootSafe = getSafeIdBySafe(org, caller);
        if ((newLimit > maxDepthTreeLimit) || (newLimit <= depthTreeLimit[org]))
        {
            revert Errors.InvalidLimit();
        }
        emit Events.NewLimitLevel(
            org, rootSafe, caller, depthTreeLimit[org], newLimit
        );
->        depthTreeLimit[org] = newLimit;
    }

note that depthTreeLimit[org] specifically, is updated.

In another place inside the contract depthTreeLimit[name] exists:

    function registerOrg(string calldata orgName)
        external
        IsSafe(_msgSender())
        returns (uint256 safeId)
    {
        bytes32 name = keccak256(abi.encodePacked(orgName));
        address caller = _msgSender();
        safeId = _createOrgOrRoot(orgName, caller, caller);
        orgHash.push(name);
        // Setting level by Default
->        depthTreeLimit[name] = 8;

        emit Events.OrganisationCreated(caller, name, orgName);
    }

a different key called [name] is used instead of [org] for providing logical code, the problem however is that if depthTreelimit were to be updated it should update both the [name] & the [org], but since updateDepthTreeLimit specifically only updates depthTreelimit[org], [name] will not be updated.

This will lead to inconsistencies inside the contract and possible errors whenever depths are checked

Recommendation

- depthTreeLimit[name] = 8;
+ depthTreeLimit[org] = 8;

use the same key values

alfredolopez80 commented 2 months ago

for my side is informational!! not a issue!! @0xmahdirostami and @0xRizwan what do you think?

whoismxuse commented 2 months ago

sponsor is right, oversight on my end

0xmahdirostami commented 2 months ago

invalid