Missing Threshold Sanitization in addOwnerWithThreshold Function

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0xbbbac1110d919caf87aeacc6b6aa61691806e6fc4b5ab177743914f26cd046aa Severity: low

Description: Description\

The addOwnerWithThreshold function in the PalmeraModule contract allows the caller to add a new owner to a Safe and set a threshold for the Safe's multisig wallet. However, the function does not currently validate that the threshold parameter is greater than zero. This oversight can lead to the threshold being set to zero, which may cause unexpected behavior or security vulnerabilities.

Impact

If the threshold is set to zero, it can lead to the following issues:

• Security Risk: A zero threshold could potentially allow transactions to be executed without any approvals, compromising the security of the Safe.
• Unexpected Behavior: The Safe's multisig logic may not function as intended, leading to operational issues and potential loss of funds.
• Inconsistent State: The Safe's configuration may become inconsistent, making it difficult to manage.

Attack Scenario\ NA

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

Recommedation

validation check in the addOwnerWithThreshold function to ensure that the threshold parameter is greater than zero. This can be achieved by adding a simple require statement at the beginning of the function.

    function addOwnerWithThreshold(
        address ownerAdded,
        uint256 threshold,
        address targetSafe,
        bytes32 org
    )
        external
        validAddress(ownerAdded)
        SafeRegistered(targetSafe)
        requiresAuth
    {

++        require(threshold > 0, "Threshold must be greater than zero");
        address caller = _msgSender();
        if (hasNotPermissionOverTarget(caller, org, targetSafe)) {
            revert Errors.NotAuthorizedAddOwnerWithThreshold();
        }

        ISafe safeTarget = ISafe(targetSafe);
        /// If the owner is already an owner
        if (safeTarget.isOwner(ownerAdded)) {
            revert Errors.OwnerAlreadyExists();
        }

        bytes memory data =
            abi.encodeCall(ISafe.addOwnerWithThreshold, (ownerAdded, threshold));

        /// Execute transaction from target safe
        _executeModuleTransaction(targetSafe, data);
    }

[0xRizwan]

Centralized issue, should be invalid.

[Atharv181]

@0xRizwan

In my opinion, this issue should be considered as low severity issue. According to the docs of hats finance, it is mentioned that

Minor deviations from best practices that don't lead to security risks consider as a low severity.

And according to the description of the issue, it is not the best practice to not check the threshold. Hence should be considered as a valid low.

[alfredolopez80]

Non-Issue, i agree with @0xRizwan, because safe check that when Palmera Module send the tx with _executeModuleTransaction, in the args data called the same way addOwnerWithThreshold, If we add this require, it would be a redundant verification which is absolutely inefficient at the gas level and bytesize code.

Verification into Safe-Contracts: https://github.com/safe-global/safe-smart-account/blob/186a21a74b327f17fc41217a927dea7064f74604/contracts/base/OwnerManager.sol#L121