XYK Core v1.2 — AMM DEX Router

Location: get-dx function

Description: The get-dx function computes how many x-tokens a user would receive for a given y-token input. However, it reads x-protocol-fee and x-provider-fee from the pool data, when it should read y-protocol-fee and y-provider-fee — because fees are charged on the input token (y in this case).

The actual swap-y-for-x function correctly uses y-protocol-fee and y-provider-fee. This means the quote function returns different values from what the swap actually produces.

;; get-dx — INCORRECT: reads x-fees for a y-input operation
(protocol-fee (get x-protocol-fee pool-data))
(provider-fee (get x-provider-fee pool-data))

;; swap-y-for-x — CORRECT: reads y-fees for a y-input operation
(protocol-fee (get y-protocol-fee pool-data))
(provider-fee (get y-provider-fee pool-data))

Impact: Any UI or contract relying on get-dx to estimate swap output will get incorrect values. If x-fees ≠ y-fees (which is configurable per pool), users may set inappropriate slippage parameters based on the wrong quote, leading to either failed transactions (if the real output is lower) or accepting worse prices than necessary (if the real output is higher). On a mainnet DEX, this directly affects user trading experience and potentially enables more effective sandwich attacks when the quote overestimates output.

Recommendation: Change get-dx to read y-protocol-fee and y-provider-fee. Since this contract is already deployed, a v1-3 deployment would be needed, or UIs should use swap-y-for-x with a dry-run call instead of get-dx.

Location: add-liquidity function

Description: When adding liquidity, the user specifies x-amount and min-dlp (minimum LP tokens). The y-amount is computed automatically as (x-amount × y-balance) / x-balance. There is no max-y-amount parameter to bound how many y-tokens the user will transfer.

;; y-amount is computed, not bounded by the caller
(y-amount (/ (* x-amount y-balance) x-balance))
;; Only min-dlp is checked, not max-y-amount
(asserts! (>= dlp min-dlp) ERR_MINIMUM_LP_AMOUNT)

Impact: A sandwich attacker can manipulate the pool ratio between a user's transaction submission and execution. By skewing the pool to have a higher y/x ratio, the attacker forces the LP provider to contribute more y-tokens than expected. The min-dlp check doesn't fully protect against this because the LP amount is proportional to x-amount, not y-amount. The user gets the expected LP tokens but overpays in y-token value.

Recommendation: Add a max-y-amount parameter to add-liquidity and assert (<= y-amount max-y-amount). Alternatively, the get-dlp function returns both dlp and y-amount — UIs should display the computed y-amount prominently so users can set post-conditions accordingly.

Location: set-x-fees, set-y-fees, set-fee-address, set-pool-status

Description: Any single admin can instantly change pool fees (up to 99.99% in BPS), redirect fee revenue to a new address, or disable any pool. There is no timelock, multi-sig threshold, or cool-down period. The fee ceiling check only requires (< (+ protocol-fee provider-fee) BPS), meaning fees can be set to 9999 BPS (99.99%).

Impact: A compromised admin key can immediately set fees to near-100%, redirect all protocol fees to an attacker address, or disable pools to grief users. For a mainnet DEX, this represents significant centralization risk. Users have no warning period to exit positions before fee changes take effect.

Recommendation: Implement a timelock (e.g., 144 blocks / ~24h) for fee changes and fee-address updates. Consider requiring 2-of-N admin signatures for critical operations. Cap total fees at a reasonable maximum (e.g., 10% / 1000 BPS).

Location: create-pool, set-public-pool-creation

Description: When public-pool-creation is enabled, anyone can call create-pool with any contract implementing xyk-pool-trait. The core contract validates token matching and fee parameters, but cannot verify the pool contract's implementation is honest. A malicious pool contract could implement pool-transfer to steal tokens, update-pool-balances to report fake balances, or get-pool to return manipulated data.

Impact: If public pool creation is enabled, an attacker can register a trojan pool contract. Users interacting with that pool through the core router (which appears legitimate) would have their tokens routed to the malicious contract. The is-valid-pool check only verifies registration, not implementation safety.

Recommendation: Keep public-pool-creation disabled (it defaults to false). If public creation is desired, maintain an allowlist of approved pool-trait implementations (verified contract hashes). With Clarity 4, contract-hash? could enforce that only known-good pool contracts are registered.

Location: create-pool function

Description: The check (asserts! (>= (- total-shares burn-amount) u0) ERR_MINIMUM_LP_AMOUNT) is vacuous. Since both values are uint, the subtraction would cause a runtime underflow panic if burn-amount > total-shares, aborting the transaction before the assert is ever evaluated. The result of any uint subtraction that doesn't underflow is always ≥ 0.

;; This check can never fail — it either panics (underflow) or is always true
(asserts! (>= (- total-shares burn-amount) u0) ERR_MINIMUM_LP_AMOUNT)

Impact: Minimal — the underflow panic provides equivalent protection (the tx aborts). However, users get an unhelpful runtime panic instead of a clear error code. The pool creator also doesn't receive the meaningful ERR_MINIMUM_LP_AMOUNT error.

Recommendation: Replace with (asserts! (>= total-shares burn-amount) ERR_MINIMUM_LP_AMOUNT) to provide the correct error before any underflow. Consider also asserting a minimum remaining shares (e.g., (> (- total-shares burn-amount) u0)) to prevent the creator from burning 100% of LP tokens.

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

Description: Fees are calculated as (/ (* amount fee) BPS). For small swap amounts where amount × fee < 10000, integer division truncates to zero. For example, with a 30 BPS fee (0.3%), any swap of less than 334 units pays zero fees.

Impact: Low — an attacker could split a large swap into many small sub-swaps to avoid fees entirely. However, each transaction has a base STX fee, so the gas cost of splitting likely exceeds the fee savings for most token denominations. This is primarily a revenue leakage issue for the protocol and liquidity providers.

Recommendation: Use ceiling division for fees: (/ (+ (* amount fee) (- BPS u1)) BPS). This ensures any non-zero fee rate always charges at least 1 unit.

Description: The contract is deployed on Clarity 2 (Epoch 2.5). Pool contracts likely use as-contract for token transfers, which grants blanket asset access. Clarity 4's as-contract? with explicit with-ft/with-stx allowances would provide language-level protection against pool contracts moving unexpected assets.

Recommendation: For future versions, deploy on Clarity 4 and use as-contract? with explicit asset allowances in pool contracts.

Description: get-dy, get-dx, and get-dlp are quote/estimation functions that don't modify state. However, they are define-public because they call contract-call? on the pool trait (which requires public context in Clarity 2). This means callers pay transaction fees for read operations.

Recommendation: UIs should call these via callReadOnlyFunction (Stacks.js) to avoid on-chain execution costs. This is a Clarity 2 limitation — the functions cannot be define-read-only because they make cross-contract calls.

Description: Unlike swap-x-for-y, swap-y-for-x, and add-liquidity, the withdraw-liquidity function does not check pool-status. This means liquidity providers can always exit their positions even when the pool is paused by admins.

Assessment: This is correct and intentional design. Users should always be able to withdraw their funds regardless of admin actions. This is a positive security property.