CRITICAL C-01: Stale Threshold on Pending Transactions

Location: safe.clar, add function (line ~217) and confirm function (line ~262)

Description: When a transaction is submitted, the current threshold is snapshot into the transaction record. If the safe's threshold is later increased (e.g., from 2 to 3), all pending transactions retain the old threshold value. This means pending transactions can be executed with fewer confirmations than the safe currently requires.

;; In add():
(map-insert transactions tx-id {
    executor: (contract-of executor),
    threshold: (var-get threshold),  ;; snapshot at submission time
    ...
})

;; In confirm():
(confirmed (>= (len new-confirmations) (get threshold tx)))
;; Uses tx's stale threshold, not current threshold

Impact: If a safe increases its threshold from 2-of-3 to 3-of-3 for increased security, any previously submitted (but unconfirmed) transaction still only needs 2 confirmations to execute. An attacker who submitted a malicious transaction before the threshold increase can still execute it with fewer approvals.

Recommendation: Use the current threshold at confirmation time, not the stored one:

(confirmed (>= (len new-confirmations) (var-get threshold)))

Alternatively, invalidate all pending transactions when the threshold changes.

Exploit Test

;; Exploit test for C-01: Stale threshold bypass
(define-public (test-exploit-c01-stale-threshold)
  (let
    (
      ;; Setup: Safe has 3 owners, threshold = 2
      ;; Owner A submits a malicious tx (tx-id 0) — threshold stored as 2
      ;; Safe increases threshold to 3 via governance
      ;; Owner A confirms tx-id 0 (1 confirmation)
      ;; Owner B confirms tx-id 0 (2 confirmations >= stored threshold of 2)
      ;; Tx executes despite current threshold being 3!
      (result true)
    )
    ;; Verify: tx executes with 2/3 confirmations when threshold is now 3
    (ok result)
  )
)

HIGH H-01: Owner Removal Allows Safe Capture by Colluding Minority

Location: safe.clar, remove-owner function (line ~96)

Description: The remove-owner function checks that the remaining owner count stays at or above the threshold: (>= (- (len owners-list) u1) (var-get threshold)). However, a colluding group of owners equal to the current threshold can systematically remove all other owners until only they remain, effectively capturing the safe.

;; With 5 owners and threshold=3:
;; 3 colluding owners submit remove-owner for owner4 → passes (4 >= 3)
;; 3 colluding owners submit remove-owner for owner5 → passes (3 >= 3)
;; Now only 3 owners remain, all colluding

Impact: A quorum of colluding owners can eject all non-colluding owners, gaining exclusive control of the safe and all its assets. This defeats the purpose of a multi-sig with diverse ownership.

Recommendation: Consider requiring a supermajority (e.g., N-1 confirmations) for owner removal, or implementing a time-locked removal process that gives affected owners time to respond.

HIGH H-02: as-contract Blanket Authority to Executors

Location: safe.clar, confirm function (line ~271)

Description: When a transaction is confirmed, the executor runs under as-contract with blanket authority over all the safe's assets. While the executor's principal must match what was stored at submission, the executor code itself could do anything — including transferring assets not specified in the tx parameters.

(and confirmed (try! (as-contract (contract-call? executor execute safe param-ft param-nft (get param-p tx) (get param-u tx) (get param-b tx)))))

Impact: A malicious executor (deployed by any owner, then submitted as a transaction) could drain all STX, tokens, and NFTs from the safe in a single execution — even if the transaction's parameters suggest a small transfer. Owners must manually verify executor source code before confirming.

Recommendation: Implement an executor allowlist that must be populated through multi-sig governance. Only pre-approved executor contracts should be executable. In Clarity 4, use as-contract? with explicit asset allowances (with-stx, with-ft, with-nft) to limit what each execution can move.

MEDIUM M-01: is-allowed-caller Bypass via Direct Calls

Location: safe.clar, is-allowed-caller function (line ~168)

Description: The allowed-caller check includes a fallback: (is-eq tx-sender caller) where caller is contract-caller. For direct user calls (no intermediary contract), tx-sender == contract-caller, so this always returns true. The allowlist is only effective when calls come through an intermediary contract.

(define-read-only (is-allowed-caller (caller principal))
  (or
    (match (map-get? allowed-callers caller)
      value true
      false
    )
    (is-eq tx-sender caller)  ;; Always true for direct calls
  )
)

Impact: The intent seems to be that the allowlist restricts which contracts can interact with the safe on behalf of users. But non-owners can call revoke for any tx-id (it will fail at the sender check, but the ERR-ONLY-END-USER guard doesn't actually gate non-owners). The real protection is the owner check in submit/confirm, but the allowed-caller system provides a false sense of security for the revoke path.

Recommendation: Clarify the intent of the allowed-caller system. If it's meant to restrict contract interactions, consider checking contract-caller separately from tx-sender.

MEDIUM M-02: No Transaction Expiration or Cancellation

Location: safe.clar, transaction lifecycle

Description: Once submitted, transactions persist indefinitely. There is no expiration mechanism and no way to cancel a transaction (only individual confirmations can be revoked). A transaction submitted months ago with one confirmation still only needs threshold-1 more to execute.

Impact: Stale transactions accumulate and may be unexpectedly executed if ownership changes or new owners unknowingly confirm old transactions. Combined with C-01 (stale threshold), this significantly increases the attack surface.

Recommendation: Add a block-height-based expiration (e.g., transactions expire after N blocks). Alternatively, add a cancel function that requires threshold confirmations to void a pending transaction.

LOW L-01: No Zero-Amount Guard on Transfers

Location: transfer-stx.clar, transfer-sip-010.clar

Description: The transfer executors don't validate that the amount (param-u) is greater than zero. Zero-value transfers waste gas and create misleading transaction records.

Recommendation: Add (asserts! (> amount u0) (err u9998)) before the transfer call.

LOW L-02: Hardcoded Testnet Addresses in Init

Location: safe.clar, bottom of file (line ~477)

Description: The init call at the bottom contains hardcoded testnet addresses (ST1SJ3..., ST2CY5..., ST2JHG...). These are standard Clarinet devnet addresses. For deployment, these must be replaced.

(init (list 
    'ST1SJ3DTE5DN7X54YDH5D64R3BCB6A2AG2ZQ8YPD5 
    'ST2CY5V39NHDPWSXMW9QDT3HC3GD6Q6XX4CFRK9AG 
    'ST2JHG361ZXG51QTKY2NQCVBPPRRE2KZB1HR05NNC
) u2)

Recommendation: This is a known deployment template pattern. Document clearly that these must be changed before mainnet deployment. Consider parameterizing via a deployment script.

INFO I-01: Clarity 1 — No as-contract? Support

Description: The contract uses Clarity 1 (Epoch 2.0). Clarity 4 introduced as-contract? with explicit asset allowances, which would significantly mitigate H-02 by restricting what assets executors can move. Migrating to Clarity 4 would allow:

;; Instead of blanket as-contract:
(as-contract? (contract-call? executor execute ...) 
  (with-stx u1000000)  ;; limit STX to 1 STX
  (with-ft token-contract u500)  ;; limit specific FT
)

Recommendation: Migrate to Clarity 4 when upgrading the contract to leverage language-level asset restrictions.

INFO I-02: Magic Bridge Tight Coupling

Description: The safe includes Magic Bridge-specific functions (mb-initialize-swapper, mb-escrow-swap) directly in the core contract. These functions bypass the multi-sig transaction flow — any single owner can call them directly. While Magic Bridge integration is useful, embedding protocol-specific logic in the core safe reduces modularity.

Recommendation: Consider moving Magic Bridge interactions into an executor contract to keep the core safe protocol-agnostic and ensure all operations go through the multi-sig flow.