Crucial Functions addOwnerWithThreshold and removeOwner Do Not Emit Events

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

Description: Description\ The functions addOwnerWithThreshold() and removeOwner() in the PalmeraModule contract are critical for managing the ownership and threshold of Safe Multisig Wallets. However, these functions do not emit any events upon successful execution. Emitting events is crucial for tracking changes and ensuring transparency and traceability of actions performed on the blockchain.

Attack Scenario\ Describe how the vulnerability can be exploited.

Attachments

Proof of Concept (PoC) File

function addOwnerWithThreshold(
    address ownerAdded,
    uint256 threshold,
    address targetSafe,
    bytes32 org
)
    external
    validAddress(ownerAdded)
    SafeRegistered(targetSafe)
    requiresAuth
{
    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);

    // Emit event
    emit Events.OwnerAddedWithThreshold(org, targetSafe, ownerAdded, threshold);
}

and

function removeOwner(
    address prevOwner,
    address ownerRemoved,
    uint256 threshold,
    address targetSafe,
    bytes32 org
) external SafeRegistered(targetSafe) requiresAuth {
    address caller = _msgSender();
    if (
        prevOwner == address(0) || ownerRemoved == address(0)
            || prevOwner == Constants.SENTINEL_ADDRESS
            || ownerRemoved == Constants.SENTINEL_ADDRESS
    ) {
        revert Errors.ZeroAddressProvided();
    }

    if (hasNotPermissionOverTarget(caller, org, targetSafe)) {
        revert Errors.NotAuthorizedRemoveOwner();
    }
    ISafe safeTarget = ISafe(targetSafe);
    /// if Owner Not found
    if (!safeTarget.isOwner(ownerRemoved)) {
        revert Errors.OwnerNotFound();
    }

    bytes memory data = abi.encodeCall(
        ISafe.removeOwner, (prevOwner, ownerRemoved, threshold)
    );

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

    // Emit event
    emit Events.OwnerRemoved(org, targetSafe, prevOwner, ownerRemoved, threshold);
}

Revised Code File (Optional)
- Adding event emissions to these functions will enhance the contract's transparency and allow for better tracking of changes to the Safe Multisig Wallets. This is a crucial improvement for maintaining a reliable and auditable system.

hats-finance / Palmera-0x5fee7541ddcd51ba9f4af606f87b2c42eea655be

Crucial Functions addOwnerWithThreshold and removeOwner Do Not Emit Events #54