Any contract can bypass `isSafe` restrictions

[hats-bug-reporter[bot]]

Github username: @0xfuje Twitter username: 0xfuje Submission hash (on-chain): 0xe58592223e20a1148ea47089a11616fe4ccd1063acef32e9956bf64700de509a Severity: medium

Description:

Description

User can create a non-safe custom contract that returns 1 on getThreshold() calls to use PalmeraModule functionality.

Non-safe contracts can registers organization, create a root safe, and be added as a safe in addSafe(), basically bypass the isSafe() restrictions.

src/Helpers.sol - isSafe()

     function isSafe(address safe) public view returns (bool) { // @audit m - any contract can bypass isSafe calls
        /// Check if the address is a Safe Smart Account Wallet
        if (safe.isContract()) {
            /// Check if the address is a Safe Multisig Wallet
            bytes memory payload = abi.encodeCall(ISafe.getThreshold, ());
            (bool success, bytes memory returnData) = safe.staticcall(payload);
            if (!success) return false;
            /// Check if the address is a Safe Smart Account Wallet
            uint256 threshold = abi.decode(returnData, (uint256));
            if (threshold == 0) return false;
            return true;
        } else {
            return false;
        }
    }

Recommendation

Consider to implement stricter checks, for example: ERC165 supportsInterface() checks instead of only checking safe is a contract and threshold in isSafe().

[0xfuje]

Such a safe can prevent execTransactionOnBehalf() calls plus ownership management with addOwnerWithThreshold() and removeOwner() calls from higher permissions safe or roles.

Can also do any arbitrary execution (or revert) in _executeModuleTransaction() calls. Which can make it impossible to remove or disconnect such a safe because in disconnectSafe() -> _exitSafe() it can revert the setGuard(address(0)) and disableModule() calls.