Off-by-One Error in Loop Condition in `PalmeraGuard` Contract

[hats-bug-reporter[bot]]

Github username: -- Twitter username: 4gontuk Submission hash (on-chain): 0x99b273a17a8321e48d0f700907ba5c912765a4b081f1d7fb8df3a0d7f0c5c5ec Severity: high

Description:

Description:

An off-by-one error exists in the checkAfterExecution function of the PalmeraGuard contract. The loop iterates from i = 1 to i < palmeraModule.indexId(), excluding the last index value. This bug can lead to incorrect authorization checks, potentially allowing unauthorized users to bypass security checks under certain conditions.

Attack Scenario:

An attacker can exploit this off-by-one error by ensuring that their critical index is exactly palmeraModule.indexId(). Since this index is not checked due to the loop condition, the attacker can manipulate the system to their advantage, bypassing authorization checks and gaining unauthorized access or permissions.

Proof of Concept (PoC)

Example Scenario:

1. Assume palmeraModule.indexId() returns 5.
2. The loop will iterate for i = 1 to i = 4, excluding i = 5.
3. If the attacker’s critical check is at index 5, it will be skipped.

Code Snippet Demonstrating the Issue:

for (uint256 i = 1; i < palmeraModule.indexId();) {
    if (palmeraModule.isSafeLead(i, caller)) {
        isSafeLead = true;
        break;
    }
    unchecked {
        ++i;
    }
}

Example User Execution:

// Suppose palmeraModule.indexId() returns 5 and the caller is safe lead at index 5
// The current code will not check index 5, bypassing the critical authorization check
address caller = someAddress;
for (uint256 i = 1; i < palmeraModule.indexId();) {
    if (palmeraModule.isSafeLead(i, caller)) {
        // Critical code that should not be executed for unauthorized users
        executeCriticalFunction();
    }
    unchecked {
        ++i;
    }
}

Mitigation:

To fix the off-by-one error, the loop condition should be updated to include the last index value. Change the loop condition from i < palmeraModule.indexId() to i <= palmeraModule.indexId().

Corrected Code Snippet:

for (uint256 i = 1; i <= palmeraModule.indexId();) {
    if (palmeraModule.isSafeLead(i, caller)) {
        isSafeLead = true;
        break;
    }
    unchecked {
        ++i;
    }
}

This change ensures that the loop will iterate through all indices from 1 to palmeraModule.indexId(), including the last index, thus preventing unauthorized access and ensuring proper security checks are performed.

[alfredolopez80]

take the snippet of function _createOrgOrRoot you can notice that always in this function of th function addSafe, the contract take the value of index and after increment!!, soo, the "real" last indexId is not the value if the before the last one, and this is the reason we use in the for loop the operator > and not the >=

    function _createOrgOrRoot(
        string memory name,
        address caller,
        address newRootSafe
    ) private returns (uint256 safeId) {
        if (bytes(name).length == 0) {
            revert Errors.EmptyName();
        }
        bytes32 org = caller == newRootSafe
            ? bytes32(keccak256(abi.encodePacked(name)))
            : getOrgHashBySafe(caller);
        if (isOrgRegistered(org) && caller == newRootSafe) {
            revert Errors.OrgAlreadyRegistered(org);
        }
        if (isSafeRegistered(newRootSafe)) {
            revert Errors.SafeAlreadyRegistered(newRootSafe);
        }
        safeId = indexId++;
        safes[org][safeId] = DataTypes.Safe({
            tier: DataTypes.Tier.ROOT,
            name: name,
            lead: address(0),
            safe: newRootSafe,
            child: new uint256[](0),
            superSafe: 0
        });
        indexSafe[org].push(safeId);

        /// Assign SUPER_SAFE Role + SAFE_ROOT Role
        RolesAuthority _authority = RolesAuthority(rolesAuthority);
        _authority.setUserRole(
            newRootSafe, uint8(DataTypes.Role.ROOT_SAFE), true
        );
        _authority.setUserRole(
            newRootSafe, uint8(DataTypes.Role.SUPER_SAFE), true
        );
    }

i hope more clear with this answer!!, and pls let me know anny another doubt!!