Missing Revocation of SafeLead Role

[hats-bug-reporter[bot]]

Github username: @Jelev123 Twitter username: zhulien_zhelev Submission hash (on-chain): 0x0501b25165916518f6e3cfcadf8c2b8ae723b94f3654750783d73617b57e1fba Severity: medium

Description: Description\ This report addresses a critical issue found in the current implementation of the role management system within our contract. Specifically, it highlights the absence of the revocation logic for the SafeLead role when the conditions are met to revoke the SuperSafe role. Ensuring that both roles are revoked under the specified conditions is crucial for maintaining the integrity and security of the system.

Impact The existing code snippet is designed to revoke the SuperSafe role if certain conditions are met (i.e., the entity has no children). However, the SafeLead role is not revoked, which can lead to potential security vulnerabilities and operational inconsistencies.

1. Proof of Concept (PoC) File

   if (oldSuper.child.length == 0) {
   _authority.setUserRole(
       oldSuper.safe, uint8(DataTypes.Role.SUPER_SAFE), false
   );
   }

   https://github.com/keyper-labs/PalmeraModule/blob/dfd821e2fd7825c66c079c19be9460238f6e045a/src/PalmeraModule.sol#L624

Recommendation

The revised code snippet includes the revocation of both the SuperSafe and SafeLead roles if the entity has no children:

 if (oldSuper.child.length == 0) {
        // Revoke SUPER_SAFE role
        _authority.setUserRole(
            oldSuper.safe, uint8(Role.SUPER_SAFE), false
        );

        // Revoke SAFE_LEAD role
        _authority.setUserRole(
            oldSuper.safe, uint8(Role.SAFE_LEAD), false
        );
    }

[Jelev123]

Also i forgot to add that it have to make another check for is not organisation

 /// Revoke SuperSafe and SafeLead if don't have any child, and is not organisation
 if (oldSuper.child.length == 0 && !isOrgRegistered(org)) 

[alfredolopez80]

Non-Issue @0xRizwan becuase the wrong here is the natSpec, not the funcionality of the Smart contract, when we call updateSuper only can remove SUPER_SAFE, not the SAFE_LEAD, because we don't know if the Root_safe or the Org itself require this save like a SAFE_LEAD of another safe into the tree!! or leaves!!

so is invalid!!

[Jelev123]

why you write wrong natspec everywhere?