Incorrect access control on removeSafe()

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x23a6cabda11ab9239e0276fc86820ace1efb5cb48b94811ea20f35345fe1e528 Severity: high

Description: Description\

removeSafe() is used to remove safe and reasign all child to the superSafe. This is implemented as:

    function removeSafe(uint256 safeId)
        public
        SafeRegistered(_msgSender())
        requiresAuth
    {

    . . . some code . .. 
   }

The shared Palmera documentation specifically states:

 Remove a Safe
 Function: removeSafe()
 Description: Removes a safe. This must be called by the root safe.

The documentation states, removeSafe() MUST be called by root safe, however the current implementation does not implement it and unsafe from intended protocol design for removeSafe(). This would not ensure, the strict access control on removeSafe() as intended by Palmera protocol as the access to such critical function is given in wrong hands and no where it mentions to allow SafeRegistered() to access removeSafe().

Recommendation\ only allow root safe to access the removeSafe() function as intended in documentation.

For example: Consider below changes:

    function removeSafe(uint256 safeId)
        public
-        SafeRegistered(_msgSender())
+        IsRootSafe(_msgSender())
+        SafeIdRegistered(safeId)
        requiresAuth
    {

    . . . some code . .. 
   }

[alfredolopez80]

Sorry is a missed some text in an part of the Description of the Diagram, but into the diagram itself can check that method removeSafe, is possible to call from Root Safe and Super Safe, inclusive we check internally that: https://github.com/hats-finance/Palmera-0x5fee7541ddcd51ba9f4af606f87b2c42eea655be/blob/1ac35880b5d45154267788e2db548eaaae0beaa0/src/PalmeraModule.sol#L416, https://github.com/hats-finance/Palmera-0x5fee7541ddcd51ba9f4af606f87b2c42eea655be/blob/1ac35880b5d45154267788e2db548eaaae0beaa0/src/PalmeraModule.sol#L417, https://github.com/hats-finance/Palmera-0x5fee7541ddcd51ba9f4af606f87b2c42eea655be/blob/1ac35880b5d45154267788e2db548eaaae0beaa0/src/PalmeraModule.sol#L418

soo is Non-Issue, @0xRizwan @0xmahdirostami let me know your comments

[0xRizwan]

@alfredolopez80 Upon further research on this issue, It seems the issue is valid. Below are my few points for your consideration:

IsRootSafe() implementation can be checked here. This IsRootSafe() modifier is used to Validate if the address is a Safe Smart Account Wallet and Root Safe.

and isRootSafeOf() is implementation can be checked here and isRootSafeOf() is used to check if the address, is a rootSafe of the safe within an organisation.

1) First point, All critical functions in PalmeraModule contract like

1. setRole() -Can only be accessed by root safe i.e IsRootSafe() as well as it checks for isRootSafeOf() in function.
2. promoteRoot() -Can only be accessed by root safe i.e IsRootSafe() as well as it checks for isRootSafeOf() in function.
3. updateSuper() -Can only be accessed by root safe i.e IsRootSafe() as well as it checks for isRootSafeOf() in function.
4. disconnectSafe() -Can only be accessed by root safe i.e IsRootSafe() as well as it checks for isRootSafeOf() in function.
5. createRootSafe(), -Can only be accessed by root safe i.e IsRootSafe()
6. removeWholeTree() -Can only be accessed by root safe i.e IsRootSafe()
7. updateDepthTreeLimit() -Can only be accessed by root safe i.e IsRootSafe()
8. addToList() -Can only be accessed by root safe i.e IsRootSafe()
9. dropFromList() -Can only be accessed by root safe i.e IsRootSafe()
10. enableAllowlist() -Can only be accessed by root safe i.e IsRootSafe()
11. enableDenylist() -Can only be accessed by root safe i.e IsRootSafe()

As you can see all of these critical functions can only be accessed by root safe and has nothing to do with superSafe access. If you observe carefully, in functions like disconnectSafe() which also checks for isRootSafeOf() as well as IsRootSafe(). So similarly, removeSafe() should use both isRootSafeOf() as well as IsRootSafe(), as highlighted by you removeSafe() only checks for isRootSafeOf() but it does not check the msg.sender is a safe root of safe smart wallet address so it must use IsRootSafe() modifer as shown in above recommendation.

2) Second point, Based on Palmera documentation- superSafe can not add or remove safe which is explicitely written in it can not section.

3) Third point, the documentation is correct and stresses on ONLY use of safeRoot

Based on above points, its clear only root safe must be given access to removeSafe() function.

[alfredolopez80]

OK, @0xRizwan letme clarify pount by point, why is Non-Issue and is only a expected behavior!!

1. The removeSafe() is a method not only for RootSafe, and I explained why in the role analysis the weakness was that in case a SuperSafe detected a ChildSafe that should not belong to its branches, it should wait for a RootSafe remove or disconnect them, which at an operational level can be very hierarchical, that is why it was decided (and this was a protocol design decision), to allow a SuperSafe to remove the ChildSafe that are below it in the first level Only, I mean and clarify this detail, the SuperSafe can only remove the Safe that has been connected to it at the level immediately below itself!!!. It is because of this that we cannot use the IsRootSafe() modifier because if we set this modifer, then only RootSafe would be able to execute this method. Additionally, it is important to clarify that the remove method produces a condition that has an additional Tier Type called REMOVED, which basically leaves the safe disabled below the RootSafe of its branch, and waiting for it to decide when is the right time to disconnect it ( For example, if that Safe has assets from the Onchain Organization, the RootSafe can transfer it to another Safe of the Organization or to itself and then distribute it appropriately).
2. The image you mention in point 2 refers to DenyHelpers, where SuperSafe does not have the ability to add or drop addresses within the list, but it has nothing to do with the AddSafe command. Where do I additionally clarify that the add safe capability can only be executed by Safe itself, not by a third party? The only thing that the RootSafe can create for another is when it creates a new RootSafe, which for security has to be created by another Safe with the RootSafe hierarchy.
3. the capture only see the descripcion, but if you see the diagram notice is mention it that the RootSafe and SuperSafe can removeSafe: https://github.com/hats-finance/Palmera-0x5fee7541ddcd51ba9f4af606f87b2c42eea655be/blob/main/doc/diagram/General%20Overview%20of%20Principal%20Work%20Flow%20Of%20Palmera%20Module.md#remove-a-safe