hats-finance / Palmera-0x5fee7541ddcd51ba9f4af606f87b2c42eea655be

Palmera hierarchical module
0 stars 1 forks source link

`setupRoles()` in `PalmeraRoles.sol` can not transfer ownership due to non-existent function calling #26

Open hats-bug-reporter[bot] opened 1 week ago

hats-bug-reporter[bot] commented 1 week ago

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

Description: Description\

PalmeraRoles.sol contract has inherited the solmate's RolesAuthority contract.

In constructor, RolesAuthority is also initialized.

    constructor(address palmeraModule)
        RolesAuthority(_msgSender(), Authority(address(0)))
    {
        setupRoles(palmeraModule);
    }

Here, the owner of authority is set to contract deployer i.e msg.sender.

RolesAuthority constructor:

   constructor(address _owner, Authority _authority) Auth(_owner, _authority) {}

PalmeraRoles.sol contract's constructors also calls setupRoles(palmeraModule) which is implemented as:

    function setupRoles(address palmeraModule)
        internal
        validAddress(palmeraModule)
    {

       . . . some code . . .

        setRoleCapability(
            uint8(DataTypes.Role.ROOT_SAFE),
            palmeraModule,
            Constants.REMOVE_WHOLE_TREE,
            true
        );

        /// Transfer ownership of authority to palmera module
        setOwner(palmeraModule);
        emit Events.PalmeraModuleSetup(palmeraModule, _msgSender());
    }

As it can be seen, the ownership of authority is transferred to palmeraModule but the issue is that setOwner() is not exist in contract.

RolesAuthority.sol also inherits Auth.sol which has transferOwnership() function to transfer the ownership.

    function transferOwnership(address newOwner) public virtual requiresAuth {
        owner = newOwner;

        emit OwnershipTransferred(msg.sender, newOwner);
    }

Recommendations\

Use transferOwnership() from inherited Auth than non-existent setOwner()

    function setupRoles(address palmeraModule)
        internal
        validAddress(palmeraModule)
    {

       . . . some code . . .

        setRoleCapability(
            uint8(DataTypes.Role.ROOT_SAFE),
            palmeraModule,
            Constants.REMOVE_WHOLE_TREE,
            true
        );

        /// Transfer ownership of authority to palmera module
-        setOwner(palmeraModule);
+        transferOwnership(palmeraModule);
        emit Events.PalmeraModuleSetup(palmeraModule, _msgSender());
    }
alfredolopez80 commented 1 week ago

Hi, i guess you refer a different version because we use in out implementation the version 6.6.2, an in this version the method as you mention transferOwnership doesn't exist, only ha SetOwner, you can check here: https://github.com/transmissions11/solmate/blob/bff24e835192470ed38bf15dbed6084c2d723ace/src/auth/Auth.sol#L48

so, taking into account this Non-issue!!! and is invalid!! @0xRizwan pls check!!

0xRizwan commented 1 week ago

transferOwnership() is in latest version of solmate, its renamed from old version. @alfredolopez80 is correct here.