Pool signer contract — manages STX stacking delegation to PoX signers for the StackingDAO protocol
| Severity | Count |
|---|---|
| MEDIUM | 2 |
| LOW | 2 |
| INFO | 3 |
This contract is StackingDAO's pool signer — it manages the delegation of STX to PoX-4 signers on behalf of the stacking pool. The contract:
Access control model: Three-way OR — pool-owner (mutable), any DAO-active contract via .dao, or the contract itself (via as-contract tx-sender). All admin and PoX wrapper functions use this same guard.
Key flow: prepare-stacking-dao → fetches delegate list from .data-pools-v1 → for each delegate: delegate-stack-stx (new) or extend+increase (existing) → aggregate-commit or aggregate-increase to the signer.
Location: set-pool-owner, set-pox-reward-address
Description: The pool owner can instantly change both the pool owner address and the PoX reward address. A compromised pool-owner key allows an attacker to immediately redirect all future stacking rewards to their own address and lock out the legitimate owner.
(define-public (set-pox-reward-address (new-address { version: (buff 1), hashbytes: (buff 32) }))
(begin
(asserts! (or
(is-eq contract-caller (var-get pool-owner))
(is-eq true (contract-call? .dao get-contract-active contract-caller))
(is-eq contract-caller (as-contract tx-sender)))
(err ERR_UNAUTHORISED))
(var-set pox-reward-address new-address) ;; Immediate effect, no timelock
(ok true)))Impact: If the pool-owner private key is compromised, the attacker can redirect BTC stacking rewards and transfer ownership in a single transaction. Delegators' STX remains locked in stacking but rewards flow to the attacker for the duration of the cycle.
Recommendation: Implement a two-step ownership transfer pattern (propose + accept) and a timelock on reward address changes, giving delegators time to react. Alternatively, route all admin changes through the DAO governance exclusively.
as-contractLocation: All as-contract calls (lines throughout delegation, aggregation, PoX wrappers)
Description: The contract uses as-contract (pre-Clarity 4) which grants unrestricted asset transfer authority for the enclosed expression. While the contract currently only uses this authority for PoX stacking operations, any future code change or composability path could inadvertently transfer STX, FTs, or NFTs held by the contract.
;; Example: delegation function
(try! (as-contract (delegate-stack-stx delegate delegation-amount
(get-pox-reward-address) burn-block-height u1)))
;; Clarity 4 equivalent (recommended):
(try! (as-contract? (delegate-stack-stx delegate delegation-amount
(get-pox-reward-address) burn-block-height u1)
with-stacking))Impact: Low immediate risk (contract doesn't hold transferable assets), but increases the attack surface if the contract is upgraded or composed with other contracts that deposit assets.
Recommendation: Migrate to Clarity 4 and use as-contract? with explicit with-stacking allowance. This restricts the contract's authority to only stacking operations at the language level.
Location: prepare-delegate-many
Description: The aggregation step silently ignores ERR_STACKING_THRESHOLD_NOT_MET (error 11). While this is intentional (small pools may not meet the minimum), it masks any other error with code 11 and provides no observability into how often the threshold is missed.
(match (aggregation)
success true
error (begin
(asserts! (is-eq error u11) (err error)) ;; Only error 11 is swallowed
true))Impact: If the threshold is persistently not met, delegators' STX sits idle without earning rewards, and there's no on-chain signal to alert operators.
Recommendation: Emit a print event when the threshold is not met so off-chain monitoring can detect and alert on the condition: (begin (print { action: "threshold-not-met", cycle: next-cycle }) true)
set-cycle-signer-infoLocation: set-cycle-signer-info
Description: The admin function accepts any reward cycle value, including past cycles. Setting signer info for an already-completed cycle wastes gas and creates confusing map state.
Impact: Low — only the admin can call this, and past-cycle signer info is never read by the aggregation logic (which always uses next-cycle). However, it could lead to operator confusion.
Recommendation: Add a check: (asserts! (>= reward-cycle (+ (current-pox-reward-cycle) u1)) (err ERR_INVALID_CYCLE))
Location: set-pool-owner, set-pox-reward-address, set-cycle-to-index
Description: Admin functions that modify critical state (pool-owner, pox-reward-address, cycle-to-index) do not emit print events. This makes off-chain monitoring and audit trails harder to maintain.
Recommendation: Add print statements to all admin functions, e.g.: (print { action: "set-pool-owner", old-owner: (var-get pool-owner), new-owner: owner })
can-prepare has no upper boundLocation: can-prepare
Description: The timing check (> burn-block-height (- start-block-next-cycle withdraw-offset)) only enforces a lower bound — it doesn't prevent preparation after the next cycle has already started. In practice, PoX-4 will reject stale operations, but an explicit upper bound would fail faster with a clearer error.
(define-read-only (can-prepare)
(let (
(current-cycle (current-pox-reward-cycle))
(start-block-next-cycle (reward-cycle-to-burn-height (+ current-cycle u1)))
(withdraw-offset (contract-call? .data-core-v1 get-cycle-withdraw-offset))
)
(> burn-block-height (- start-block-next-cycle withdraw-offset))))Recommendation: Add: (and (> burn-block-height (- start-block-next-cycle withdraw-offset)) (< burn-block-height start-block-next-cycle))
unwrap-panic usage in aggregationLocation: aggregation function
Description: The aggregation function calls (unwrap-panic (get pox-addr signer-info)) and similar patterns on fields of signer-info that is already validated as (is-some). While safe (the unwrap will never panic given the prior assert), using unwrap-panic is a code smell — if the logic is ever refactored and the assert is moved, the contract could abort entire transactions.
Recommendation: Use unwrap! with explicit error codes for defense-in-depth: (unwrap! (get pox-addr signer-info) (err ERR_MISSING_SIGNER_INFO))
to-uint), making the contract easy to audit and upgrade.delegation function correctly handles the case where a delegate has no active delegation, avoiding unnecessary PoX calls.stack-aggregation-commit-indexed for first-time commits and stack-aggregation-increase for subsequent additions, tracking the reward index per cycle..dao — DAO governance contract determines which contracts are "active" and authorized. Compromise of the DAO grants full control over this contract..data-pools-v1 — Provides the delegate list. A corrupted delegate list could cause delegation to wrong principals..data-core-v1 — Provides the withdraw offset timing parameter. An incorrect offset could prevent or premature-trigger preparation.pool-owner — Single key with full admin access. Key compromise = full control.Independent audit by cocoa007.btc · Full audit portfolio · Not a professional security audit — see repo for methodology