PalmeraGuard can be removed due to wrong check orders

[hats-bug-reporter[bot]]

Github username: @SB-Security Twitter username: SBSecurity_ Submission hash (on-chain): 0x11bc72f4ba0cf84c991dbd29b94c59ea597ef9f60570cff2dfc82c1ec7998c81 Severity: medium

Description: Description\ checkAfterExecution is called after the main transaction execution and if guard has been removed prior to that checkAfterExecution will not even be called.

if (guard != address(0)) {
                Guard(guard).checkAfterExecution(txHash, success);
            }

If safe has executed setGuard function disabling the PalmeraGuard here https://github.com/safe-global/safe-smart-account/blob/v1.4.0/contracts/Safe.sol#L203, the post-execution will not call the PalmeraGuard back to perform the additional validations..

Attack Scenario\

Attachments

 function checkAfterExecution(bytes32, bool) external view {
        address caller = _msgSender();
        // if it does, check if try to disable guard and revert if it does.
        // if it does, check if try to disable the Palmera Module and revert if it does.
        if (palmeraModule.isSafeRegistered(caller)) {
            if (!ISafe(caller).isModuleEnabled(address(palmeraModule))) {
                revert Errors.CannotDisablePalmeraModule(address(palmeraModule));
            }
            if (
                abi.decode(
                    StorageAccessible(caller).getStorageAt(
                        uint256(Constants.GUARD_STORAGE_SLOT), 2
                    ),
                    (address)
                ) != address(this)
            ) {
                revert Errors.CannotDisablePalmeraGuard(address(this));
            }
        } else {
            if (!palmeraModule.isSafe(caller)) {
                bool isSafeLead;
                // Caller is EAO (lead) : check if it has the rights over the target safe
                for (uint256 i = 1; i < palmeraModule.indexId();) {
                    if (palmeraModule.isSafeLead(i, caller)) {
                        isSafeLead = true;
                        break;
                    }
                    unchecked {
                        ++i;
                    }
                }
                if (!isSafeLead) {
                    revert Errors.NotAuthorizedAsNotSafeLead();
                }
            }
        }

1. Proof of Concept (PoC) File

2. Revised Code File (Optional) Perform the checks in checkTransaction

[alfredolopez80]

Non-Issue, because are not clear about how safe send the transactions

when any safe try to call "setGuard" this method pass thorugh a "execTransaction and if you check here: https://github.com/safe-global/safe-smart-account/blob/186a21a74b327f17fc41217a927dea7064f74604/contracts/GnosisSafe.sol#L191

This method call Guard(guard).checkAfterExecution(txHash, success); before to finalize the method, so is the safe not have the autoriy to disconnectSafe, can't setGuard in Zero Address, because pass through PalmeraGuard Before to finalize the execution!!!

Additinal info if you check the lib folder notice we work with version 1.3.0, although regarding the guard we are compatible with version 1.4.0 as well.

Another point you refer the method but missed in the same method call Guard(guard).checkAfterExecution(txHash, success); in the line 218 like you can see here: https://github.com/safe-global/safe-smart-account/blob/e870f514ad34cd9654c72174d6d4a839e3c6639f/contracts/Safe.sol#L218