StackSwap stackswap-swap-v5k

Location: All functions — get-lp-data/set-lp-data pattern throughout

Description: The swap contract delegates all pair state (balances, shares, fees) to the liquidity token contract passed as a trait parameter. While create-pair is DAO-gated, the fix-or-add-pair function (H-01 related, see M-03) allows the DAO owner to replace a pair's liquidity token at any time. More importantly, the swap contract implicitly trusts that the LP token's get-lp-data returns honest values and that set-lp-data stores them faithfully.

If a malicious or buggy liquidity token is registered (via fix-or-add-pair or a compromised DAO), it could:

• Return inflated balance-y to extract more tokens than exist in a swap
• Return zero shares-total to re-trigger initial liquidity logic and dilute existing LPs
• Ignore set-lp-data calls, making the contract think balances updated when they didn't

The safe-transfer function mitigates some of this by verifying actual token balance changes, but it cannot catch inflated get-lp-data values used in the AMM formula.

;; All pair state comes from the LP token — fully trusted
(let ((pair (try! (contract-call? token-liquidity-trait get-lp-data)))
      (balance-x (get balance-x pair))
      (balance-y (get balance-y pair))
      ;; dy computed using potentially dishonest balances
      (dy (/ (* FEE_1 balance-y dx) (+ (* FEE_2 balance-x) (* FEE_1 dx)))))

Impact: A compromised or malicious LP token contract can manipulate swap outputs, drain pool reserves, or dilute liquidity providers. The trust boundary is effectively at the LP token level, not the swap contract.

Recommendation: This is an architectural risk inherent to the design. Mitigations: (1) ensure LP token contracts are immutable or DAO-governed with timelock, (2) add an allowlist of approved LP token contracts checked at the swap level, (3) consider storing pair state in the swap contract itself rather than delegating to the LP token.

Location: swap-x-for-y, swap-y-for-x

Description: Both swap functions use strict less-than (<) for slippage protection instead of less-than-or-equal (<=). This means if the computed output exactly equals the user's minimum, the swap reverts.

;; swap-x-for-y:
(asserts! (< min-dy dy) ERR_TOO_MUCH_SLIPPAGE)

;; swap-y-for-x:
(asserts! (< min-dx dx) ERR_TOO_MUCH_SLIPPAGE)

Impact: Users who set their minimum output to the exact expected amount (common in UI-generated transactions) will have their swaps rejected. This causes unnecessary transaction failures, especially in low-liquidity pools where the output may exactly match expectations.

Recommendation: Change to <=: (asserts! (<= min-dy dy) ERR_TOO_MUCH_SLIPPAGE). This is the standard convention used by Uniswap and most AMMs.

Location: collect-fees

Description: The fee collection function requires both fee-balance-x > 0 AND fee-balance-y > 0 to succeed. If trading is predominantly one-directional (e.g., everyone buying token-y with token-x), the fee-balance-y remains zero and fees cannot be collected at all — even though fee-balance-x may be substantial.

(asserts! (> fee-x u0) ERR_NO_FEE_X)
(asserts! (> fee-y u0) ERR_NO_FEE_Y)

Impact: Protocol fees can become locked indefinitely in one-directional markets. For tokens that trend strongly in one direction (e.g., a token declining to zero), accumulated fees on one side may never be collectable.

Recommendation: Allow collection when either fee is non-zero: (asserts! (or (> fee-x u0) (> fee-y u0)) ERR_NO_FEE). Use conditional transfers: only transfer fee-x if it's > 0, same for fee-y.

Location: fix-or-add-pair

Description: The DAO owner can call fix-or-add-pair to replace the liquidity token contract for any existing pair. This changes which contract stores pair state and holds the pooled tokens. While the old LP token still holds the actual tokens, the swap contract would now read state from and route transfers through the new LP token — effectively orphaning the old pool's funds and state.

;; Owner can replace LP token for any existing pair
(define-public (fix-or-add-pair (token-x-trait ...) (token-y-trait ...) (token-liquidity-trait ...))
  (let ((contract-owner-check (asserts! (is-eq contract-caller 
         (contract-call? .stackswap-dao-v5k get-dao-owner)) ERR_NOT_OWNER))
        ...)
    (if x-to-y
      ;; Replaces the LP token in the registry
      (map-set pairs-data-map ... {liquidity-token: token-liquidity, pair-id: pair-id})
      ...)))

Impact: DAO owner (single key) can redirect any pair to a malicious LP token, enabling the attacks described in H-01. This is a centralization risk — a single compromised key could affect all pairs.

Recommendation: Add a timelock or multi-sig requirement for LP token replacement. Consider emitting an event when LP tokens are changed so monitoring tools can alert users. Alternatively, make LP token assignments immutable after creation.

Location: add-to-position

Description: The first liquidity provider receives sqrti(x * y) shares with no minimum burned. In Uniswap V2, the first 1000 shares (MINIMUM_LIQUIDITY) are permanently locked to prevent the pool from being fully drained and to mitigate share price manipulation attacks.

(new-shares
  (if (is-eq (get shares-total pair) u0)
    (sqrti (* x y))  ;; No minimum subtracted
    (/ (* x (get shares-total pair)) balance-x)))

Impact: A first depositor can create a pool with dust amounts (e.g., 1 unit of each token), mint 1 LP share, then donate tokens directly to the LP contract to inflate the share price. Subsequent depositors would receive 0 shares due to integer division rounding, losing their deposits. This is the classic "first depositor" or "inflation" attack on AMMs.

Recommendation: Burn a minimum amount of initial shares (e.g., 1000) by sending them to a dead address, similar to Uniswap V2's MINIMUM_LIQUIDITY constant.

Location: add-to-position

Description: When a pool already has liquidity, the y parameter provided by the user is completely ignored. The actual y amount deposited is recalculated as (/ (* x balance-y) balance-x) to maintain the price ratio. While this is mathematically correct, it means the user has no way to specify a maximum y they're willing to deposit.

(new-y
  (if (is-eq (get shares-total pair) u0)
    y  ;; Only used on first deposit
    (/ (* x balance-y) balance-x)  ;; User's y parameter ignored
  ))

Impact: Users cannot set a maximum y deposit. If the pool ratio changes between transaction submission and execution (e.g., due to a large swap), the user may deposit significantly more y tokens than intended. The router presumably handles this, but the core contract provides no protection.

Recommendation: Add a check: (asserts! (<= new-y y) ERR_TOO_MUCH_SLIPPAGE) to use the y parameter as a maximum, similar to Uniswap's amountBDesired/amountBMin pattern.

Description: The contract uses Clarity 1 (pre-Clarity 4). While the swap contract itself doesn't use as-contract (token transfers are done via the LP token's transfer-token), migrating to Clarity 4 would allow the LP token contracts to use as-contract? with explicit asset allowances, significantly reducing the risk described in H-01.

Recommendation: If redeploying, use Clarity 4 with as-contract? and explicit with-ft allowances in the LP token contracts.

Description: The fee structure applies two separate fees:

1. LP fee (0.3%): Built into the AMM formula via FEE_1=997 / FEE_2=1000. This fee stays in the pool, benefiting LPs.
2. Protocol fee (0.05%): Calculated separately as FEE_3=5 / FEE_4=10000. Deducted from the input token and accumulated in fee-balance-x/fee-balance-y.

The protocol fee is deducted from the input after the AMM formula uses the full input for pricing. This means the pool's tracked balance-x increases by (dx - fee) but the formula computed dy using the full dx. Over many swaps, the tracked balances will slightly undercount the actual reserves, creating a small persistent surplus in the LP token contract. This is benign — it slightly benefits LPs — but means the internal accounting doesn't perfectly reflect reality.

Recommendation: This is a known and acceptable pattern. The surplus acts as an additional safety margin. No action needed, but documenting the effective total fee as ~0.35% would help users.

Location: fix-or-add-pair

Description: When replacing an LP token for an existing pair, the old LP token's entry in pairs-token-map is not removed. This means the old LP token is still registered as a valid pair token even though it's no longer associated with any pair in the registry.

;; Old LP token stays in pairs-token-map as `true`
;; Only the new one is added
(map-set pairs-token-map token-liquidity true)

Impact: Stale entries in pairs-token-map. Currently this map is only checked during create-pair to prevent reuse, so the impact is that the old LP token can never be used for a new pair. Minimal practical impact.

Recommendation: Add (map-delete pairs-token-map old-liquidity-token) when replacing.