STX Yield Staking — Security Audit

C-01 tx-self Does Not Exist — Contract Won't Compile

Location: unstake, claim, deposit-rewards, emergency-withdraw — 7 occurrences

Description: The contract uses tx-self in multiple functions as if it were a keyword that returns the contract's own principal address. This keyword does not exist in any version of Clarity (1 through 4). The correct way to reference the contract's principal is (as-contract tx-sender).

;; unstake (line ~201): tx-self is not a Clarity keyword
(try! (stx-transfer? pending tx-self caller))
...
(try! (stx-transfer? amount tx-self caller))

;; claim (line ~225):
(try! (stx-transfer? pending tx-self caller))

;; deposit-rewards (line ~238):
(try! (stx-transfer? amount tx-sender tx-self))

;; emergency-withdraw (line ~270):
(let ((bal (stx-get-balance tx-self)))
  (try! (stx-transfer? bal tx-self (var-get owner))))

Impact: The contract will fail at compilation time. None of the core functions (unstake, claim, deposit-rewards, emergency-withdraw) can execute. The contract is completely undeployable.

Recommendation: Replace all tx-self references with (as-contract tx-sender). For transfers FROM the contract, wrap the transfer in (as-contract ...):

;; Instead of: (stx-transfer? amount tx-self caller)
;; Use:
(as-contract (stx-transfer? amount tx-sender caller))

C-02 update-rewards Returns Bool, Used as Uint — Type Mismatch

Location: update-rewards (private fn), stake, unstake, claim

Description: The update-rewards function returns bool (true/false). However, every public function binds its return value to current-rps and uses it as a uint in arithmetic operations — multiplication with staked amounts and comparison in calc-pending.

;; update-rewards returns bool:
(define-private (update-rewards)
  (let (...)
    (var-set last-reward-block current-block)
    (if (and (> ts u0) (> current-block last-block) (> rate-bps u0))
      (let (...)
        (var-set acc-reward-per-share new-rps)
        true    ;; <-- returns bool
      )
      false     ;; <-- returns bool
    )
  )
)

;; But callers use it as uint:
(define-public (stake (amount uint))
  (let (
    (current-rps (update-rewards))  ;; bool, NOT uint!
    ...
    (new-debt (* new-staked current-rps))  ;; uint * bool = TYPE ERROR
  )
    (calc-pending user-data current-rps)   ;; expects uint, gets bool
    ...
  )
)

Impact: Clarity is strongly typed — uint * bool and passing bool where uint is expected will fail at compilation time. Combined with C-01, no function in this contract can compile.

Recommendation: update-rewards should return the current acc-reward-per-share:

(define-private (update-rewards)
  (let (...)
    (var-set last-reward-block current-block)
    (if (and (> ts u0) (> current-block last-block) (> rate-bps u0))
      (let (...)
        (var-set acc-reward-per-share new-rps)
        new-rps  ;; return the updated value
      )
      rps  ;; return unchanged value
    )
  )
)

C-03 stake Transfers STX From User to User — Funds Never Reach Contract

Location: stake function, line ~170

Description: In the stake function, the user's STX deposit is "transferred" with:

(let (
  (caller tx-sender)
  ...
)
  ;; Transfer stake from user to contract (user-authorized transfer)
  (try! (stx-transfer? amount caller tx-sender))
  ;; caller == tx-sender, so this transfers from user to... user
)

Since caller is bound to tx-sender at the start of the let block, this is (stx-transfer? amount tx-sender tx-sender) — a transfer from the user to themselves. The contract's balance never increases, but total-staked is incremented.

Impact: Users appear to have staked funds (state is updated), but no STX actually moves to the contract. When users attempt to unstake or claim, the contract has no STX to pay them. This creates a phantom accounting system where the contract promises STX it doesn't hold.

Recommendation: Transfer to the contract using (as-contract tx-sender):

;; Correct: transfer from user to contract
(try! (stx-transfer? amount tx-sender (as-contract tx-sender)))

H-01 emergency-withdraw Drains User Stakes, Not Just Reward Pool

Location: emergency-withdraw function

Description: The emergency withdrawal transfers the contract's entire STX balance to the owner. This includes both the reward pool AND all user-staked principal. The comment says "admin drains entire contract STX balance" and acknowledges "total-staked is NOT zeroed here; users must unstake after." But users cannot unstake — the contract has no STX left.

(define-public (emergency-withdraw)
  (begin
    (asserts! (is-owner) ERR-OWNER-ONLY)
    (let ((bal (stx-get-balance tx-self)))
      (asserts! (> bal u0) ERR-NO-EMERGENCY)
      (try! (stx-transfer? bal tx-self (var-get owner)))
      ;; total-staked NOT zeroed — users can't recover
      (var-set reward-pool-balance u0)
      (ok bal)
    )
  )
)

Impact: The admin can rug-pull all staked funds at any time. There is no timelock, no multi-sig, and no separation between reward pool and staked principal. This is a unilateral fund extraction vector.

Recommendation: Emergency withdraw should only drain the reward pool balance, not user stakes. Or implement a two-phase emergency: (1) pause all operations, (2) allow each user to individually withdraw their stake.

H-02 Dual Auth Check Weakens Ownership Guard

Location: is-owner function

Description: The ownership check accepts EITHER contract-caller or tx-sender matching the owner:

(define-private (is-owner)
  (or (is-eq contract-caller (var-get owner))
      (is-eq tx-sender       (var-get owner))))

The comment says "never rely solely on tx-sender for auth" (per CertiK), but the fix is wrong. Using or with both is weaker than using either alone. If the owner calls through an intermediary contract, tx-sender is still the owner even though contract-caller is the intermediary. Conversely, if a malicious contract has contract-caller set to the owner, tx-sender might be an attacker.

Impact: An intermediary contract called by the owner can pass the auth check via tx-sender, even if the intermediary shouldn't have admin privileges. This opens a vector for confused deputy attacks through contract composition.

Recommendation: Use only contract-caller for auth, which represents the immediate caller:

(define-private (is-owner)
  (is-eq contract-caller (var-get owner)))

M-01 No Minimum Stake or Cooldown — Reward Sniping Possible

Location: stake and unstake functions

Description: There is no minimum staking duration or cooldown period. A user can stake in one block and immediately unstake in the next, capturing any accumulated acc-reward-per-share increment. With large flash-loaned STX, an attacker can stake a massive amount for one block, diluting all other stakers' rewards for that period, then unstake.

Impact: Flash-loan-style reward sniping. An attacker with large capital can periodically spike the total-staked to harvest a disproportionate share of rewards, at the expense of long-term stakers.

Recommendation: Add a minimum staking duration (e.g., 144 blocks / ~1 day) before unstaking is allowed. Track stake-block per user and enforce cooldown.

M-02 Reward Rate Change Can Be Front-Run

Location: set-reward-rate function

Description: When the admin increases the reward rate, the transaction is visible in the mempool. An attacker can front-run by staking a large amount before the rate increase takes effect, capturing the higher rate from block one.

Impact: Sophisticated stakers can extract disproportionate rewards around rate changes, diluting returns for honest stakers.

Recommendation: Implement a time-delayed rate change (e.g., announce the new rate, apply it after N blocks). This gives all stakers equal opportunity to adjust positions.

L-01 No Ownership Transfer Confirmation (Two-Step)

Location: set-owner function

Description: Ownership is transferred in a single step. If the admin accidentally sets the owner to a wrong address, the contract becomes permanently unmanageable — no one can change the rate, deposit rewards, or emergency withdraw.

Recommendation: Implement a two-step transfer: propose-owner + accept-ownership.

L-02 burn-block-height vs stacks-block-height Inconsistency

Location: update-rewards, print statements

Description: The reward accumulator uses burn-block-height (Bitcoin blocks) for delta calculation, but the constant BLOCKS-PER-YEAR u52560 assumes ~10-minute Stacks blocks. Post-Nakamoto, Stacks blocks are ~5 seconds while Bitcoin blocks remain ~10 minutes. Using burn-block-height with a Stacks-block-based constant means rewards accrue ~60x slower than intended.

Recommendation: Use stacks-block-height consistently, or adjust BLOCKS-PER-YEAR to match Bitcoin block times (~52,560 Bitcoin blocks/year is actually correct for Bitcoin, but then the APY math needs to account for the much slower block production).

I-01 No Pause Mechanism

Location: Global

Description: Unlike the emergency-withdraw function, there is no way to pause staking/unstaking operations. If a vulnerability is discovered, the admin's only option is to drain all funds (including user stakes) via emergency-withdraw.

Recommendation: Add a paused data-var and guard all public functions with an (asserts! (not (var-get paused)) ERR-PAUSED) check.

I-02 Missing SIP-010 Token — Staked Positions Not Composable

Location: Global

Description: Users' staked positions are tracked in a map but not represented as a fungible token. This means staked positions cannot be used as collateral, traded, or composed with other DeFi protocols. Most production staking protocols issue a receipt token (e.g., stSTX).

Recommendation: Implement a SIP-010 receipt token that is minted on stake and burned on unstake, representing the user's pro-rata share of the pool.