[Title]

The OneOf constraint type for address parameters is intended to allow any address from a Merkle-tree-based allowlist. However, the Merkle proof required for verification is included inside the constraints data that gets hashed into the allowed functions Merkle tree, effectively locking the constraint to a single provable address.

In LibPolicyContractInteraction._isFunctionAllowedByPolicy(), the constraints bytes are hashed to produce the constraintsHash, which is then combined with the function selector to form the leaf that must exist in the allowed functions Merkle tree:

bytes32 constraintsHash = keccak256(constraints);
bytes32 funcLeaf = _computeFunctionLeaf(selector, constraintsHash);
return MerkleProof.verify(functionProof, policy.roots.allowedFunctionsRoot, funcLeaf);

The constraints bytes are an ABI-encoded array of ParameterConstraint structs. Each ParameterConstraint contains a paramValueInListProof field, which holds the Merkle proof used by _isAddressParameterAllowedByConstraint() to verify that the actual address value is a member of the allowed addresses tree.

Because paramValueInListProof is part of the ABI-encoded constraints, and the hash of the full constraints is committed in the allowed functions Merkle tree, only one specific proof (and therefore one specific address from the allowlist) can satisfy both the function-level Merkle verification and the address-level Merkle verification at the same time. Submitting a different proof for a different allowed address would change the constraints hash, causing the function-level Merkle verification to fail.

This reduces the OneOf constraint to behave identically to an Exact constraint, defeating its purpose of allowing any address from a predefined set.

Medium. Policies that use OneOf address constraints for contract interaction parameters will silently restrict transactions to a single address instead of the full allowlist. This limits the expressiveness of the policy system and may force administrators to create redundant policies for each allowed address.

Move the paramValueInListProof outside of the data that gets hashed into the allowed functions Merkle tree. One approach is to pass the proofs as a separate parameter alongside the constraints, so that changing the proof does not affect the constraints hash used for function-level verification.

Policies configured for ContractInteractions cannot restrict the native ETH value attached to a call, allowing transactions to payable functions with arbitrary msg.value.

In LibOrganizationPolicy.isTransactionAllowedByPolicy(), the ContractInteractions branch filters out token transfers but does not validate the value field against any policy constraint before delegating to LibPolicyContractInteraction.isContractInteractionAllowedByPolicy():

if (proofs.policy.config.transactionType == TransactionType.ContractInteractions) {
    if (TokenTransferUtils.isTransactionTokenTransfer(data, value)) return false;

    return LibPolicyContractInteraction.isContractInteractionAllowedByPolicy({...});
}

A native ETH transfer (empty calldata with nonzero value) is already classified as a token transfer and filtered out. However, a call with nonempty calldata and a positive value passes the token transfer check and enters the contract interaction path. The policy system validates the function selector, destination, and parameter constraints, but has no mechanism to express a constraint on the attached value. This means a policy that authorizes a call to a payable function implicitly allows any amount of ETH to be sent along with it.

Medium. Organizations cannot limit the ETH value attached to authorized contract interactions. A member authorized to call a specific payable function could drain the account's ETH balance in a single call, even if the policy was only intended to authorize the function's logic without large value transfers.

Add an optional value constraint to the contract interaction policy configuration, allowing administrators to set a maximum or exact ETH value that can be attached to authorized calls.

The Guardian security model relies on BatchedTransaction to execute groups of operations atomically, but the user authorization layer signs each operation independently. This means batch cohesion is operationally enforced by the Guardian path rather than cryptographically enforced across the full stack.

SafeExecutorModule.executeOnBehalf() enables the authorized executor to route calls through BatchedTransaction.execute(), which gives the Guardian Safe an atomic all-or-nothing submission path. The documentation explicitly treats this batching behavior and the associated full-revert semantics as part of the Guardian security model.

At the same time, the organization and wallet layers expose single-operation execution primitives such as OrganizationAccountTransactionBase.executeAccountTransaction() and AccountImplementation.executeTransaction(). The signed payloads for account transactions and admin operations authorize each operation on its own. They do not commit to a shared batch hash, batch identifier, ordered bundle, or other cryptographic linkage that would make the operations inseparable once disclosed.

As a result, if a batch is assembled offchain and later fails or is otherwise revealed, the Guardian can still resubmit only a subset of those already-signed operations, or retry them in a different grouping, while remaining within the current authorization model. This is not an external bypass because only the Guardian can call the state-changing organization entrypoints, but it does mean atomic batch behavior is not enforced in a multi-layer fashion and ultimately depends on Guardian correctness.

Low. Batch atomicity is a trust assumption on the Guardian rather than a cryptographic guarantee, which could lead to partial resubmission of signed operations.

If batch-level atomicity is intended to be a hard security property, introduce a batch commitment into the signed payloads, such as a batch hash or batch identifier that every operation in the bundle must carry, and add an organization or wallet level batch entrypoint that enforces it onchain. Otherwise, document explicitly that preventing partial resubmission of a revealed batch is a trust assumption placed on the Guardian rather than a guarantee enforced by the contracts.

Rate-limit accounting for an executed transaction is handled in LibOrganizationAccountTransaction._validateAndUpdateRateLimitOrRevert(). The branch that decides how much usage to attribute to the current transaction is:

if (policy.config.transactionType == TransactionType.TokenTransfers) {
    usageAmount = TokenTransferUtils.extractTransferAmount(data, params.value);
} else {
    usageAmount = 1; // Count-based limit for non-transfer transactions
}

The selector is purely the policy's transactionType enum — the shape of the calldata is not consulted. Any policy whose transactionType is not exactly TokenTransfers gets count-based accounting (usageAmount = 1) even when the calldata is a real ERC-20 transfer(address,uint256) / transferFrom(address,address,uint256) that the codebase can already recognize via TokenTransferUtils.isTransactionTokenTransfer() (used for routing in LibOrganizationPolicy.isTransactionAllowedByPolicy() and LibOrganizationPolicy.sol:138).

The concrete case this affects is TransactionType.Any. An Any policy authorizes the transaction at LibOrganizationPolicy.sol:153-160 purely on destination matching. The rate-limit surface, however, is a separate layer that runs after the policy check and does have access to data and value. Choosing to ignore calldata shape at this layer is an active decision, not a consequence of "no common field" — it produces a second-order asymmetry:

• The same policy.config.rateLimit.timeIntervalLimit uint256 scalar means "N calls per window" when the operator uses Any and "N units of amount per window" when they use TokenTransfers. The field is semantically overloaded purely by the sibling transactionType field.
• An operator who uses Any because they want to cover both shapes of call behind one policy loses the ability to express amount-based caps on the transfer half of the traffic. The only workaround is to split into one TokenTransfers policy plus one ContractInteractions policy, which forces a different set of tradeoffs (two policy leaves, two separate approval/timelock flows for admins).
• An operator setting timeIntervalLimit = 1000 on an Any policy while mentally translating "1000 USDC per day" is silently given "1000 calls per day, each of unbounded amount". This is a configuration footgun that is not flagged by the type system or by documentation.

Low. Operators who use TransactionType.Any with rate limiting cannot express any amount-based cap for the transfer-shaped traffic that passes through the policy.

This is most likely to affect:

• Operations teams who migrate a policy from TokenTransfers to Any to also allow occasional contract calls, keeping the same timeIntervalLimit value. The intent "amount cap" silently becomes "count cap".
• Operations teams who design a single Any policy for a DeFi workflow that mixes approve / transfer / router calls under one rate cap, expecting a single USD-value budget per window.

Split the single timeIntervalLimit scalar in RateLimitConfig into two independent caps, one per accounting dimension, and accrue each from calldata shape:

struct RateLimitConfig {
    RateLimitType limitType;
    uint16 timeIntervalHours;
    uint256 timeIntervalCountLimit;   // 0 = disabled; bucket always accrues +1 per call
    uint256 timeIntervalAmountLimit;  // 0 = disabled; bucket accrues transfer amount when calldata is transfer-shaped
    uint256 anchorTimestamp;
    RateLimitScope initiatorScope;
    RateLimitScope sourceScope;
    RateLimitScope destinationScope;
}

Accounting in _validateAndUpdateRateLimitOrRevert becomes shape-driven rather than type-driven:

1. If timeIntervalCountLimit > 0, accrue +1 to the count bucket and revert if the bucket would exceed the limit.
2. If timeIntervalAmountLimit > 0 and TokenTransferUtils.isTransactionTokenTransfer(data, value) is true, accrue TokenTransferUtils.extractTransferAmount(data, value) to the amount bucket and revert if the bucket would exceed the limit.
3. Both checks are independent; a transaction must pass both to execute. The two buckets should have separate storage keys (e.g., a (usageKey, bucketKind, timeWindow) triple) to avoid aliasing.

After this change:

• Any policies can express "max N calls per window AND max M transferred per window" in one policy leaf.
• TokenTransfers policies can additionally cap call count (defense-in-depth against many small transfers).
• ContractInteractions policies can leave timeIntervalAmountLimit = 0 and keep today's count-based semantics unchanged.
• The semantic overload on the old timeIntervalLimit ("meaning depends on transactionType") is eliminated: each field has one, explicit unit.

This is a breaking change to RateLimitConfig, PolicyConfig, and the policy Merkle leaf. If the team prefers to keep the existing layout, the minimum alternative is to document explicitly in docs/MERKLETREE_ARCHITECTURE.md (or the user-facing policy-builder docs) that under TransactionType.Any, timeIntervalLimit is always a call-count cap and is never applied to transfer amounts, so that operators do not rely on the amount-cap semantic they are used to from TransactionType.TokenTransfers.

The SafeExecutorModule only prevents calls to the Safe itself, but does not verify that the target address is a known organization. This allows the authorized executor to call any arbitrary contract through the Safe.

In executeOnBehalf(), the module checks that the target is not the Safe address to prevent ownership and module modifications, but it does not validate that the target is a registered organization:

if (to == SAFE) {
    revert CannotCallSafe(to);
}

The authorized executor can call any contract address, including other modules or entities that may have control over the Safe through alternative paths. The OrganizationFactory could maintain a registry of deployed organizations that could be used to verify that the target is a legitimate organization.

Note that the same restriction would need to be replicated in the BatchedTransaction implementation for consistency.

Low.

Consider adding a check that validates the target address is a registered organization deployed through the OrganizationFactory. This would provide an additional layer of defense by ensuring the Safe can only interact with known organizations.

ImplementationWhitelistProxy only rejects initData.length == 0, but it accepts any other payload. During construction, OpenZeppelin's ERC1967Proxy forwards any non-empty _data to ERC1967Utils.upgradeToAndCall(), which only requires the delegatecall to succeed.

That means initData does not need to call initialize(...). Any non-reverting selector exposed by ImplementationWhitelistImplementation, such as isInitialized() or isImplementationWhitelisted(), can return successfully while leaving owner == address(0) and the initializer still callable at initialize(...).

Informational.

Strengthen the ImplementationWhitelistProxy constructor:

constructor(
    address implementation,
    address initialOwner,
    address[] memory organizationImplementations,
    address[] memory accountImplementations
)
    ERC1967Proxy(
        implementation,
        abi.encodeCall(
            IImplementationWhitelist.initialize,
            (initialOwner, organizationImplementations, accountImplementations)
        )
    )
{}

The Safe module documentation and deployment flow consistently describe AUTHORIZED_EXECUTOR as an EOA. See GUARDIAN_PROTECTION.md, DEPLOYMENT.md, and DeployGuardianSafeModule.s.sol.

Onchain, though, SafeExecutorModule only checks that authorizedExecutor != address(0). It does not enforce authorizedExecutor.code.length == 0 or otherwise constrain the signer type.

That means the module can be deployed with a contract as AUTHORIZED_EXECUTOR. In that configuration:

• executeOnBehalf() authorizes calls based only on msg.sender == AUTHORIZED_EXECUTOR, regardless of whether that sender is an EOA or contract.
• isValidSignature() uses SignatureUtils.tryRecoverSigner(), which supports both EOA and ERC-1271 signers, so the module's signing semantics can effectively be delegated to that configured contract address.

Informational.

Make the design intent explicit in code:

• If the executor must be an EOA, reject contract addresses in the constructor.
• If contract executors are intentionally allowed, update the contract comments and deployment docs to describe the broader signer model accurately.

OrganizationImplementation.upgradeToAndCallWithAuthorization() is the custom upgrade entrypoint. It is protected by onlyGuardian, validates admin authorization, performs whitelist checks, and then forwards into the inherited UUPS upgradeToAndCall.

Unlike the inherited UUPS entrypoint, it does not explicitly enforce proxy context with onlyProxy.

Today this is still fail-closed in practice:

• A direct call on the implementation reaches onlyGuardian.
• onlyGuardian reads the guardian slot from the implementation's own storage.
• The implementation constructor calls _disableInitializers(), so the guardian slot remains unset.
• As a result, direct implementation calls revert before the upgrade flow can proceed.

So the current protection works, but it is state-dependent rather than structural. The proxy-only invariant for this entrypoint currently relies on implementation storage remaining uninitialized, instead of being enforced directly by the function itself.

Informational.

Add onlyProxy to the entrypoint before onlyGuardian so the execution-context check fails first:

function upgradeToAndCallWithAuthorization(
    address newImplementation,
    bytes calldata data,
    AdminAuthParams calldata authParams
) external override onlyProxy onlyGuardian {
    // body unchanged
}

The OperationType enum in CommonTypes.sol defines an AccountTransactionRejection value that is never used anywhere in the codebase. Unused enum values can lead to confusion about intended functionality.

The AccountTransactionRejection value is the last entry in the OperationType enum. No function or library references this value, and no signature digest includes it. Its presence suggests a feature that was removed or never implemented.

Informational.

Remove AccountTransactionRejection from the OperationType enum to avoid confusion.

Several internal state queries tracked by the organization have no corresponding external getter, preventing off-chain integrations and other contracts from accessing this information without relying on events.

1. Deployed accounts: The deployedAccounts mapping in LibOrganizationAccountFactoryStorage tracks which accounts were deployed by the organization, and LibOrganizationAccountFactory.isAccountDeployedByOrganization() exists as an internal library function, but no public wrapper is exposed in OrganizationAccountFactoryBase.

2. Deleted groups: The group system tracks deleted groups internally, but OrganizationGroupsBase only exposes isGroup() (which returns false for deleted groups) and isGroupMember(). There is no way to distinguish between a group that was never created and one that was created and later deleted.

Informational.

Expose isAccountDeployedByOrganization() as a public view function in OrganizationAccountFactoryBase, and add an isDeletedGroup() or getGroupStatus() view function in OrganizationGroupsBase.

The _extractContractInnerSignature() function in SignatureUtils copies signature bytes using 32-byte mstore operations but does not clear potential dirty bytes in the last word when sigLength is not a multiple of 32. This is inconsistent with the approach used in BytesUtils.sliceRange().

The assembly loop in _extractContractInnerSignature() copies data in 32-byte chunks. When sigLength is not aligned to 32 bytes, the final mstore writes a full 32-byte word, potentially leaving dirty bytes beyond the declared length of the contractSig bytes array. While Solidity's high-level operations respect the length field and ignore trailing bytes, low-level consumers or hashing operations that read past the declared length could observe inconsistent data.

Informational.

Clear the trailing bytes after the copy loop, similar to how BytesUtils.sliceRange() handles partial words, or document the assumption that consumers always respect the length prefix.

The OrganizationFactory embeds the implementation address in the OrganizationProxy creation bytecode, which is used as the CREATE2 salt input. This creates a dependency between cross-chain address determinism and the initial implementation address.

_getOrganizationProxyBytecode() encodes the implementation address into the proxy's constructor arguments:

return abi.encodePacked(type(OrganizationProxy).creationCode, abi.encode(implementationAddress, whitelistAddress));

Because CREATE2 addresses are derived from the deployer, salt, and init code hash, the organization's address is tied to the specific implementation address used at deployment time. This has two implications:

1. When the implementation is upgraded on a new chain, new organizations must first be deployed with the original implementation address and then upgraded, in order to produce the same address.
2. The original implementation address cannot be removed from the whitelist, as it must remain deployable to preserve address consistency across chains.

Informational.

Document these constraints explicitly for operators managing cross-chain deployments. Alternatively, consider removing the implementation address from the proxy's constructor arguments and setting it post-deployment, so that the CREATE2 address does not depend on the implementation version.

The getActualDestination() function in LibPolicyDestination uses two sequential checks (empty calldata, then token transfer detection) where the logic could be expressed more directly as a single ternary condition.

The current implementation first checks for empty calldata (native ETH transfer), then checks whether the transaction is a token transfer:

// Case: The transaction is a native token transfer
if (data.length == 0) return to;

// Case: The transaction is a contract interaction
if (!TokenTransferUtils.isTransactionTokenTransfer(data, value)) return to;

// Case: The transaction is an ERC-20 token transfer
// Extract the recipient address from the transfer function call
return TokenTransferUtils.extractERC20TransferRecipient(data);

Since isTransactionTokenTransfer() already handles both native and ERC-20 transfers, this could be simplified to a single check that returns the ERC-20 recipient only when the transaction is a token transfer, and to otherwise. The current version is functionally correct but adds an unnecessary branching step.

Informational.

Consider simplifying the function to a single expression for readability.

return isTransactionERC20TokenTransfer(data, value) ? TokenTransferUtils.extractERC20TransferRecipient(data) : to;

Multiple contracts use custom isInitialized() functions based on domain-specific state, alongside OpenZeppelin's Initializable library which tracks initialization independently. Having two separate mechanisms could lead to confusion about the contract's state.

In LibOrganizationInitialization, the initialize() function checks isInitialized() (which returns adminCount > 0) before proceeding. The organization also inherits from OpenZeppelin's Initializable, which provides its own _initialized flag and initializer modifier. These two guards operate independently: one is domain-specific (admin count) and the other is a generic reentrancy-style guard from the upgradeable proxy pattern.

A similar pattern exists in ImplementationWhitelistImplementation, where isInitialized() returns owner() != address(0). This relies on the assumption that the owner is always set during initialization, but would incorrectly report the contract as uninitialized if the owner ever renounces ownership via renounceOwnership(), even though the contract was properly initialized.

Informational.

Document the relationship between the two initialization mechanisms and clarify which one serves as the authoritative check. Alternatively, consolidate them to use a single source of truth. For the whitelist contract, consider using OpenZeppelin's initialization state directly instead of relying on ownership as a proxy for initialization status.

Rate limits and per-transaction amount thresholds operate on raw token amounts without accounting for token identity or decimal precision. When combined with anyToken = true policies, these controls become meaningless because they mix values from tokens with different decimals and economic worth into a single comparison or usage bucket.

Token transfer policies can set anyToken = true to authorize transfers of any token. Amount controls in LibPolicyTokenTransfer._isTokenAmountAllowedByPolicy() compare the raw extracted amount against a configured threshold, while LibPolicyRateLimits tracks cumulative usage per time window using the same raw amounts. The rate limit usage key computed by computeUsageKey() does not include the token contract address, so all tokens share the same bucket.

This creates two problems when multiple tokens with different decimals are used under the same policy:

1. A threshold calibrated for an 18-decimal token (e.g., 1e18 for "1 token") would permit 1e18 raw units of a 6-decimal token like USDC, which represents 1e12 USDC in economic terms.
2. Time-interval rate limits accumulate raw amounts across different tokens in the same bucket, making the cumulative cap arbitrary when tokens have different decimal scales or economic values.

Informational.

Avoid combining anyToken policies with amount thresholds or time-interval rate limits. If multi-token spending caps are needed, use explicit per-token policies with thresholds calibrated to each token's decimals, or include the token contract address in the rate limit usage key and maintain per-token buckets.

Policy updates replace the organization-wide Merkle root in OrganizationPolicyBase.setPolicies() / LibOrganizationPolicy.setPolicies(). However, the EIP-712 digests used for transaction and ERC-1271 approvals do not bind signers to that root, to the policy leaf hash, or to any policy version/epoch.

For account transactions, the initiator and reviewer digests include policyId but nothing derived from proofs.policy or policiesRoot; see LibOrganizationAccountTransaction._computeInitiatorHashFromParams() and _computeReviewHashFromParams().

For ERC-1271, the same pattern exists in LibOrganizationAccountSignature._getInitiatorSignatureHash() and _getReviewSignatureHash(). During validation, the contract then checks proofs.policy against the current Merkle root in _validatePolicyBasedSignature() and LibOrganizationPolicy.isPolicyInOrg().

If a new root reuses the same policyId, already-collected signatures remain cryptographically valid and are reinterpreted under the new policy definition. The result could be a semantic mismatch between what signers approved offchain and what the contract enforces onchain at execution time.

Neither the natspec of setPolicies() / LibOrganizationPolicy.setPolicies() nor any file under docs/ currently states that updating policiesRoot does not invalidate outstanding approvals and that reusing a policyId preserves pending signatures until they expire (the existing notes on signature invalidation in docs/SIGNATURES.md cover only nonce burning on execute/reject).

Informational.

Document this behavior clearly.

OrganizationPolicyBase.setPolicies() commits administrators to a 32-byte newPoliciesRoot and a hash of the IPFS CID, but the contract has no way to verify that:

1. newPoliciesRoot is the merkle root of any specific dump.
2. The IPFS payload at ipfsCid resolves to bytes whose merkle root equals newPoliciesRoot.
3. Administrators reviewed the policies that newPoliciesRoot actually commits to.

This is intrinsic to the design: the IPFS CID is hashed using the IPFS multihash scheme over raw bytes; newPoliciesRoot is the keccak256 merkle root of the parsed (policyId, Policy) leaves. The two cannot be cross-checked on-chain, and the contract cannot fetch IPFS content. NatSpec at IOrganizationPolicy.sol:22-24 already declares the CID purpose as "for disaster recovery", consistent with the CID not being a signing-time trust primitive.

The setPolicies flow consequently depends on the off-chain layers of the project's three-layer security model to perform the consistency verification. If layers 1 and 2 perform their independent verifications correctly, the realistic attacks against setPolicies (an admin proposing (R_A, CID_of_B) where merkleRoot(B) ≠ R_A to inject a hidden permissive policy set) fail. The on-chain layer is intentionally not the line of defense for this consistency check.

The gap this finding identifies is documentation, not code.

Informational.

Add to docs a section that:

1. Specifies the canonical dump format. Exact serialization (ABI vs JSON), field ordering, leaf ordering when constructing the tree from the dump, and tie-breaking rules (e.g., sort by policyId ascending). The format must be deterministic so any compliant implementation produces the same merkleRoot.
2. States the off-chain verification responsibilities explicitly:
   • The signing client MUST recompute merkleRoot(dump) == newPoliciesRoot from a dump the admin can review before presenting the EIP-712 payload for signature.
   • The Guardian service MUST repeat the same verification independently before relaying on-chain.
   • The CID is a disaster-recovery anchor, not a signing-time trust primitive; admins SHOULD NOT use IPFS-fetched content as the primary review source unless they also verify the recomputed root.
3. Calls out that on-chain enforcement is intentionally absent for this check so readers do not mistake the absence for a bug.
4. Provides or links to a reference implementation (open-source CLI / library) that any party can use to compute merkleRoot(dump) from a canonical dump. Reproducible builds preferred. Without this, the canonical-format specification is only paper, and admins are still forced to trust a closed-binary client.

The implementation() function in OrganizationAccountFactoryBase validates that the account implementation address is nonzero and has code. These checks should already be enforced by LibOrganizationAccountFactory.setAccountImplementation() at the time the implementation is set.

implementation() performs two checks every time it is called:

if (impl == address(0)) {
    revert IOrganization.AccountImplementationNotSet();
}
if (impl.code.length == 0) {
    revert ERC1967Utils.ERC1967InvalidImplementation(impl);
}

If setAccountImplementation() already validates these conditions when the implementation address is stored, repeating them on every read adds unnecessary gas cost.

Gas Savings.

Remove the redundant checks from implementation() if setAccountImplementation() guarantees the stored address is always valid and nonzero.

In LibPolicyInitiator, the isGroup() check before isGroupMember() is redundant because isGroupMember() already verifies that the given group ID corresponds to a current group in the organization.

The initiator validation for group-based policies performs two sequential checks:

if (!LibOrganizationGroups.isGroup(initiatorGroupId)) {
    return false;
}
return LibOrganizationGroups.isGroupMember(initiatorGroupId, initiatorAddress);

Since isGroupMember() internally validates that the group exists before checking membership, the preceding isGroup() call duplicates work and wastes gas.

Gas Savings.

Remove the standalone isGroup() check and rely on isGroupMember() to handle both group existence and membership verification.