Potential Protocol insolvency in `removeWholeTree` and `disconnectSafe`

[hats-bug-reporter[bot]]

Github username: @0xmahdirostami Twitter username: 0xmahdirostami Submission hash (on-chain): 0xd77a9b46593e4a7658ab290f9fe2d76a34d5ed7ef1b4e5558a5f286e8c6d8cf2 Severity: high

Description: Description: The _exitSafe function currently includes a check for getPreviewModule(caller) and reverts if it returns address(0). This can lead to a Denial of Service (DoS) issue because users can call disableModule on their own, causing subsequent calls to _exitSafe to fail.

Impact: The DoS vulnerability in _exitSafe affects the removeWholeTree and disconnectSafe functions. If disableModule has been called by the user, it will result in a failure of _exitSafe, thereby preventing the proper execution of these functions, leading to Protocol insolvency.

Proof of Concept (PoC):

Here is the relevant code snippet in _exitSafe:

function _exitSafe(address caller) internal {
    // Some other code...

    address prevModule = getPreviewModule(caller);
    if (prevModule == address(0)) {
        revert Errors.PreviewModuleNotFound(_safe);
    }
    data = abi.encodeCall(ISafe.disableModule, (prevModule, address(this)));
    // Execute transaction from target safe
    _executeModuleTransaction(_safe, data);

    emit Events.SafeDisconnected(org, safeId, address(safeTarget), caller);
}

If a user has previously called disableModule, getPreviewModule(caller) will return address(0), leading to the function reverting.

Suggested Test Case:

please change the following:

-    function getPreviewModule(address safe) internal view returns (address) {
+    function getPreviewModule(address safe) public view returns (address) {

To demonstrate the issue, add the following test to PalmeraGuardTest:

function test_test() public {
    (uint256 rootId, uint256 safeA1Id) = palmeraSafeBuilder.setupRootOrgAndOneSafe(orgName, safeA1Name);

    address rootAddr = palmeraModule.getSafeAddress(rootId);
    address safeAddr = palmeraModule.getSafeAddress(safeA1Id);

    // Remove Safe A1
    safeHelper.updateSafeInterface(rootAddr);
    bool result = safeHelper.createDisconnectSafeTx(safeA1Id);
    assertEq(result, true);

    address prevModule = palmeraModule.getPreviewModule(safeAddr);
    assertEq(prevModule, address(0));
}

Expected Log:

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 28.34ms (10.57ms CPU time)

Mitigation:

Update the _exitSafe function to handle cases where prevModule is address(0):

function _exitSafe(address caller) internal {
    // Some other code...

    address prevModule = getPreviewModule(caller);
    if (prevModule != address(0)) {
        data = abi.encodeCall(ISafe.disableModule, (prevModule, address(this)));
        // Execute transaction from target safe
        _executeModuleTransaction(_safe, data);
    }

    emit Events.SafeDisconnected(org, safeId, address(safeTarget), caller);
}

By checking if prevModule is not address(0), the function avoids reverting unnecessarily and ensures that _exitSafe can proceed even if the module has already been disabled.

[0xmahdirostami]

In test_test, I want to describe a scenario where the user disables the module on his safe, so calls to removeWholeTree and disconnectSafe will fail.

Better test case:

    function test_test() public {
        (uint256 rootId, uint256 safeA1Id) =
            palmeraSafeBuilder.setupRootOrgAndOneSafe(orgName, safeA1Name);

        address rootAddr = palmeraModule.getSafeAddress(rootId);
        address safeAddress = palmeraModule.getSafeAddress(safeA1Id);

        vm.startPrank(rootAddr);
        palmeraModule.setRole(
            DataTypes.Role.SAFE_LEAD_EXEC_ON_BEHALF_ONLY,
            rootAddr,
            safeA1Id,
            true
        );

        /// Disable
        address prevModule = palmeraModule.getPreviewModule(safeAddress);
        bytes memory data = abi.encodeCall(
            ISafe.disableModule, (prevModule, address(palmeraModule))
        );
        bool result = palmeraModule.execTransactionOnBehalf(
            orgHash,
            rootAddr,
            safeAddress,
            safeAddress,
            uint256(0),
            data,
            Enum.Operation.Call,
            "0x"
        );

        // try to disconnect
        vm.expectRevert();
        palmeraModule.disconnectSafe(safeA1Id);
    }

[alfredolopez80]

Non-Issue @0xmahdirostami , we have a Unit-test for that @0xmahdirostami

• avoid replay attack to removeSafe: https://github.com/hats-finance/Palmera-0x5fee7541ddcd51ba9f4af606f87b2c42eea655be/blob/1ac35880b5d45154267788e2db548eaaae0beaa0/test/PalmeraGuardTest.t.sol#L50

• avoid replay attack to disconnectSafe: https://github.com/hats-finance/Palmera-0x5fee7541ddcd51ba9f4af606f87b2c42eea655be/blob/1ac35880b5d45154267788e2db548eaaae0beaa0/test/PalmeraGuardTest.t.sol#L50

in the end _exitSafe is a private function called by removeWholeTree and disconnectSafe but verify the status fo the safe before to call _exitSafe additonally, the Palmera Guard avoid to the Safe disconnectSafe if not have the permissions to execute that methods

can you check the logic of the Palmera Guard and verify that!!

[0xmahdirostami]

@alfredolopez80 thank you.

if you check the test case in comments, you will see that it’s different from tests that you mentioned., it’s not replay attack, if safe disable module from its own, the disconnect safe and removeWholeTree will revert. It means that root couldn’t delete that safe.

It's not about calling remove safe or disconnect safe twice; it's about first disabling the module, so any attempt to disconnect the safe will be reverted.

[alfredolopez80]

@alfredolopez80 thank you.

if you check the test case in comments, you will see that it’s different from tests that you mentioned., it’s not replay attack, if safe disable module from its own, the disconnect safe and removeWholeTree will revert. It means that root couldn’t delete that safe.

It's not about calling remove safe or disconnect safe twice; it's about first disabling the module, so any attempt to disconnect the safe will be reverted.

ok, @0xmahdirostami retake it this, and let me explain the details about how removeSafe and disconnectSafe work, and how to connect with the Palmera Guard

1. RootSafe, SuperSafe and Child Safe can't disable module in their own (using the native methods of safe disableModule(module address) and setGuard(zero address)) becuase the Palmera Guard prevent if any Safe of Organization try to disable module or guard without autority, only the rootSafe have the Role and Autority to call DisconnectSafe or removeWholeTree, and eliminate de safe of the array safes[org][id's] in this scenario and pass this if in the PalmeraGuard: https://github.com/hats-finance/Palmera-0x5fee7541ddcd51ba9f4af606f87b2c42eea655be/blob/1ac35880b5d45154267788e2db548eaaae0beaa0/src/PalmeraGuard.sol#L62, so is impossible to any superSafe or Child Safe execute in their own any disable module or disable guard!!

2. The Root Safe can call remoteSafe, Disconnect Safe or remoteWholeTree if they want but of course the have the more high level in all organization, and we asume have a deep knowledge of the consecuence into their organization, but in any case not produce the error that you mentions!!

3. I run your test have several point in your PoC:

it working that way:

    function test_test() public {
        (uint256 rootId, uint256 safeA1Id) =
            palmeraSafeBuilder.setupRootOrgAndOneSafe(orgName, safeA1Name);

        address rootAddr = palmeraModule.getSafeAddress(rootId);
        address safeAddress = palmeraModule.getSafeAddress(safeA1Id);

        vm.startPrank(rootAddr);
        palmeraModule.setRole(
            DataTypes.Role.SAFE_LEAD_EXEC_ON_BEHALF_ONLY,
            rootAddr,
            safeA1Id,
            true
        );

        palmeraHelper.setSafe(rootAddr);
        bytes memory data = abi.encodeCall(
            ISafe.disableModule, (Constants.SENTINEL_ADDRESS, address(palmeraModule))
        ); 
        /// try to disable module through execTransactionOnBehalf
        bool result = palmeraModule.execTransactionOnBehalf(
            orgHash,
            rootAddr,
            safeAddress,
            safeAddress,
            uint256(0),
            data,
            Enum.Operation.Call,
            "0x"
        );

        // try to disconnect
        vm.expectRevert();
        palmeraModule.disconnectSafe(safeA1Id);
    }

But this is works only with the express colaboration of the rootSafe, the question is the RootSafe execute this several steps to fail the correct way to disconnectSafe?? i'm not sure is a issue, Because it is like intentionally placing a wrong address in the receive address in a cross-chain transfer, that is, RootSafe, which is the only one that has the authorization levels to execute that series of steps, executes them with the intention of making the methods fail. disconnectSafe() and removeWholeTree()