Location: deposit, init-withdraw, cancel-withdraw, withdraw

Description: All user-facing functions accept trait parameters (reserve, commission, staking, direct-helpers) that are validated only via .dao check-is-protocol. The contract trusts the DAO's protocol allowlist completely. If a malicious contract is ever added to the allowlist — through compromised governance, a malicious proposal, or an allowlist bypass — it can execute arbitrary logic under as-contract authority, potentially draining all funds.

;; Each function validates trait params against DAO allowlist
(try! (contract-call? .dao check-is-protocol (contract-of reserve)))
(try! (contract-call? .dao check-is-protocol (contract-of commission-contract)))
(try! (contract-call? .dao check-is-protocol (contract-of staking-contract)))
(try! (contract-call? .dao check-is-protocol (contract-of direct-helpers)))

;; Then calls these contracts with as-contract authority
(try! (as-contract (contract-call? commission-contract add-commission staking-contract stx-fee-amount)))

Impact: A compromised or overly permissive DAO allowlist enables an attacker to pass a malicious contract implementing any of the four traits. Under as-contract, this malicious contract executes with the core contract's authority — it could drain the reserve, mint arbitrary stSTX, or steal fees. The blast radius is the entire protocol's TVL.

Recommendation: Consider hardcoding known-good contract principals for critical trait implementations (reserve, commission) rather than accepting them as dynamic parameters. Alternatively, migrate to Clarity 4's as-contract? with explicit with-stx/with-ft allowances to limit what called contracts can do. At minimum, the DAO should have a timelock on allowlist additions.

Location: deposit (line: stx-ststx binding), init-withdraw (line: stx-ststx binding)

Description: The STX:stSTX exchange rate is fetched from .data-core-v1.get-stx-per-ststx which takes the reserve contract as a parameter. This rate determines how many stSTX tokens a depositor receives and how much STX a withdrawer gets. If the exchange rate can be manipulated within a single transaction (e.g., by temporarily inflating/deflating the reserve balance), an attacker could mint excess stSTX or withdraw excess STX.

;; Deposit: rate determines stSTX minted
(stx-ststx (try! (contract-call? .data-core-v1 get-stx-per-ststx reserve)))
(ststx-amount (/ (* stx-user-amount u1000000) stx-ststx))

;; Init-withdraw: rate determines STX owed
(stx-ststx (try! (contract-call? .data-core-v1 get-stx-per-ststx reserve)))
(stx-amount (/ (* ststx-amount stx-ststx) u1000000))

Impact: If an attacker can manipulate the reserve's reported balance within a single transaction, they can deposit at a deflated rate (getting more stSTX) and withdraw at an inflated rate (getting more STX), extracting value from other stakers. The severity depends on how data-core-v1.get-stx-per-ststx computes the rate — if it's purely based on reserve STX balance vs stSTX supply, it may be vulnerable to donation attacks.

Recommendation: Review data-core-v1.get-stx-per-ststx implementation for manipulation resistance. Consider using a TWAP or rate-smoothing mechanism. Add minimum output amount parameters so users can set slippage protection.

Location: withdraw

Description: The withdrawal function uses strict greater-than (>) to compare burn block height against the unlock height. This means users must wait one block beyond the specified unlock height before they can withdraw. The unlock height from get-withdraw-unlock-burn-height represents the start of the next reward cycle — users should be able to withdraw at that exact block, not one block after.

(asserts! (> burn-block-height unlock-burn-height) (err ERR_WITHDRAW_LOCKED))

Impact: Users are forced to wait one extra block (~10 minutes) beyond the advertised unlock height. While not a fund loss, this creates UX friction and may cause confusion when the UI shows "unlocked" but the withdrawal transaction fails.

Recommendation: Change to (>= burn-block-height unlock-burn-height) to allow withdrawal at the exact unlock block.

Location: set-stack-fee, set-unstack-fee

Description: The fee-setting functions accept any uint value without bounds checking. Fees are in basis points (bps), so a value of 10000 = 100%. A compromised governance or malicious proposal could set fees to 100%, taking the entire deposit/withdrawal amount.

(define-public (set-stack-fee (fee uint))
  (begin
    (try! (contract-call? .dao check-is-protocol contract-caller))
    (var-set stack-fee fee)  ;; No upper bound check
    (ok true)
  )
)

Impact: Governance can silently extract 100% of user deposits or withdrawals through fees. Even with honest governance, the absence of a hard cap removes a safety net against governance attacks.

Recommendation: Add a maximum fee constant (e.g., u500 for 5%) and assert the fee is within bounds: (asserts! (<= fee MAX-FEE) (err ERR_FEE_TOO_HIGH))

Location: get-withdraw-unlock-burn-height, get-reward-cycle-length

Description: The helper function get-reward-cycle-length uses unwrap-panic on the result of pox-4 get-pox-info. Similarly, init-withdraw uses unwrap-panic on both get-withdraw-unlock-burn-height and get-last-token-id. If any of these return none unexpectedly, the entire transaction aborts with an unrecoverable panic.

(define-read-only (get-reward-cycle-length)
  (get reward-cycle-length (unwrap-panic (contract-call? 'SP000000000000000000002Q6VF78.pox-4 get-pox-info)))
)

Impact: While pox-4 is unlikely to return none, using unwrap-panic provides no error information and could brick init-withdraw if assumptions about external contracts break.

Recommendation: Replace unwrap-panic with unwrap! and descriptive error codes for debuggability.

Location: init-withdraw

Description: The NFT ID is read via get-last-token-id in the let block, then used to set withdrawal data in data-core-v1, before the actual NFT mint occurs later. While Clarity's single-threaded execution within a block prevents true race conditions, if get-last-token-id returns the last minted ID (not the next ID), the withdrawal data could be mapped to the wrong NFT.

(let (
    ;; ...
    (nft-id (unwrap-panic (contract-call? .ststx-withdraw-nft get-last-token-id)))
  )
    ;; Sets withdrawal data using nft-id BEFORE minting
    (try! (contract-call? .data-core-v1 set-withdrawals-by-nft nft-id stx-amount ststx-amount unlock-burn-height))
    ;; ... 
    ;; Mint happens later
    (try! (as-contract (contract-call? .ststx-withdraw-nft mint-for-protocol sender)))
)

Impact: If get-last-token-id returns the current (already minted) ID rather than the next-to-be-minted ID, withdrawal data is stored under the wrong key. This depends on the NFT contract's implementation — if it returns next-id, this is correct. Severity depends on the NFT contract.

Recommendation: Verify that get-last-token-id returns the next-to-mint ID (not the last minted). Better yet, have mint-for-protocol return the minted ID and use that to set withdrawal data.

Location: cancel-withdraw

Description: When cancelling a withdrawal, the contract calls reserve.request-stx-for-withdrawal (which sends STX to the contract) and then immediately sends it back to the reserve. This creates an unnecessary two-hop transfer that wastes gas.

;; Request STX from reserve to this contract
(try! (as-contract (contract-call? reserve request-stx-for-withdrawal stx-amount tx-sender)))
;; Immediately send it back
(try! (as-contract (stx-transfer? stx-amount tx-sender (contract-of reserve))))

Impact: Extra gas cost for cancel operations. Also creates a transient state where STX is held by the core contract, which could interact poorly with any invariant checks on reserve balance.

Recommendation: Add a dedicated cancel-stx-for-withdrawal function to the reserve trait that decrements the withdrawal counter without moving STX.

Description: The contract uses as-contract (pre-Clarity 4) which gives blanket authority over all contract-held assets when calling external contracts. Clarity 4 introduced as-contract? with explicit asset allowances that restrict which tokens/assets the called contract can move.

This is particularly relevant here because the contract passes as-contract authority to dynamically-provided trait implementations (reserve, commission, staking, direct-helpers). A malicious implementation could exploit the blanket authority to move assets beyond what the function intends.

Recommendation: On redeployment, migrate to Clarity 4 and use as-contract? with explicit allowances: (as-contract? (with-stx stx-amount) (with-ft .ststx-token ststx-amount) (contract-call? ...)). This would eliminate H-01's blast radius entirely.

Description: The migrate-ststx function burns stSTX from v1 and mints to v2. It's protocol-gated but can be called multiple times. After the first call, v1's balance is 0, so subsequent calls are no-ops (burn 0, mint 0). However, defense-in-depth suggests a one-time execution flag.

(define-public (migrate-ststx)
  (let (
    (balance-v1 (unwrap-panic (contract-call? .ststx-token get-balance .stacking-dao-core-v1)))
  )
    (try! (contract-call? .dao check-is-protocol contract-caller))
    (try! (contract-call? .ststx-token burn-for-protocol balance-v1 .stacking-dao-core-v1))
    (try! (contract-call? .ststx-token mint-for-protocol balance-v1 (as-contract tx-sender)))
    (ok true)
  )
)

Recommendation: Add a (define-data-var migrated bool false) guard. Low priority since re-execution is harmless.