Skip to content

fix(F-9): add reentrancy locks to the v3 successor pools (Option B) #88

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
unixwhisperer wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(F-9): add reentrancy locks to the v3 successor pools (Option B) #88

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions Clarinet.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,16 @@ path = "contracts/test/flashstack-stx-pool-v3.clar"
clarity_version = 3
epoch = "3.0"

[contracts.test-stx-pool-v3-receiver-reentrant]
path = "contracts/test/test-stx-pool-v3-receiver-reentrant.clar"
clarity_version = 3
epoch = "3.0"

[contracts.test-sbtc-pool-v3-receiver-reentrant]
path = "contracts/test/test-sbtc-pool-v3-receiver-reentrant.clar"
clarity_version = 3
epoch = "3.0"

[contracts.flashstack-sbtc-core-v2]
path = "contracts/test/flashstack-sbtc-core-v2.clar"
clarity_version = 3
Expand Down
33 changes: 30 additions & 3 deletions contracts/flashstack-sbtc-pool-v3.clar
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@
;; Here the update only PROPOSES; the new admin must call accept-admin. NOT DEPLOYED.
;; F-8 FIX -- deposit is also gated by pause now (v2's deposit was not; flash-loan was).
;; See docs/security/FINDINGS_REGISTER.md. withdraw stays ungated so LPs can always exit.
;; F-9 FIX -- reentrancy lock on deposit/withdraw/flash-loan, ported from
;; flashstack-pool-v3's pv3-F1 guard. Without it, a flash-loan receiver can
;; "repay" by calling deposit mid-callback and mint shares at the loan-depressed
;; price, diluting every LP (proven on the live v2 pools, ajv.4.8).
;; ============================================================================
;; FlashStack sBTC Pool v2 (HARDENED)
;;
Expand Down Expand Up @@ -52,6 +56,7 @@
(define-constant ERR-NO-SHARES (err u708))
(define-constant ERR-INSUFFICIENT-SHARES (err u709))
(define-constant ERR-TRANSFER-FAILED (err u710))
(define-constant ERR-REENTRANT (err u712))

;; =============================================
;; State
Expand All @@ -68,6 +73,12 @@
(define-data-var total-volume uint u0)
(define-data-var total-fees uint u0)

;; F-9 fix: one lock shared by deposit/withdraw/flash-loan. This pool holds a
;; single asset, so a bool is the per-asset lock (pool-v3 needs a map keyed by
;; asset). Set first in each entry point, cleared just before its (ok ...); a
;; failed assert aborts the whole call, so a reverted call never leaves it set.
(define-data-var reentrancy-locked bool false)

(define-constant SHARE-PRECISION u100000000) ;; 1e8 - matches sBTC sat precision

;; F-1 fix (v2): virtual shares + virtual assets (OpenZeppelin ERC-4626
Expand All @@ -94,17 +105,23 @@
;; shares = amount * (total_shares + VIRTUAL-SHARES) / (pool_balance + VIRTUAL-ASSETS)
(new-shares (/ (* amount (+ current-shares VIRTUAL-SHARES)) (+ pool-balance VIRTUAL-ASSETS)))
)
;; F-9 fix: reentrancy guard, checked first.
(asserts! (not (var-get reentrancy-locked)) ERR-REENTRANT)
(var-set reentrancy-locked true)
;; F-8 fix: deposit is gated by pause, matching flash-loan and pool-v3's pv3-F3 fix.
;; withdraw is deliberately never gated, so LPs can always still exit.
(asserts! (not (var-get paused)) ERR-PAUSED)
(asserts! (> amount u0) ERR-ZERO-AMOUNT)
;; Effects before interaction, matching pool-v3; if the transfer below
;; fails the whole call reverts, these writes included.
(map-set lp-shares depositor
(+ (default-to u0 (map-get? lp-shares depositor)) new-shares))
(var-set total-shares (+ current-shares new-shares))
(unwrap!
(contract-call? 'SM3VDXK3WZZSA84XXFKAFAF15NNZX32CTSG82JFQ4.sbtc-token
transfer amount depositor (as-contract tx-sender) none)
ERR-TRANSFER-FAILED)
(map-set lp-shares depositor
(+ (default-to u0 (map-get? lp-shares depositor)) new-shares))
(var-set total-shares (+ current-shares new-shares))
(var-set reentrancy-locked false)
(ok new-shares)
)
)
Expand All @@ -118,6 +135,10 @@
get-balance (as-contract tx-sender)) ERR-TRANSFER-FAILED))
(sats-amount (/ (* shares (+ pool-balance VIRTUAL-ASSETS)) (+ current-shares VIRTUAL-SHARES)))
)
;; F-9 fix: reentrancy guard. Withdraw has no callback surface of its own;
;; this blocks a flash-loan callback from reentering withdraw (pool-v3 parity).
(asserts! (not (var-get reentrancy-locked)) ERR-REENTRANT)
(var-set reentrancy-locked true)
(asserts! (> shares u0) ERR-ZERO-AMOUNT)
(asserts! (>= depositor-shares shares) ERR-INSUFFICIENT-SHARES)
(asserts! (> sats-amount u0) ERR-ZERO-AMOUNT)
Expand All @@ -127,6 +148,7 @@
(as-contract (contract-call? 'SM3VDXK3WZZSA84XXFKAFAF15NNZX32CTSG82JFQ4.sbtc-token
transfer sats-amount tx-sender withdrawer none))
ERR-TRANSFER-FAILED)
(var-set reentrancy-locked false)
(ok sats-amount)
)
)
Expand All @@ -148,6 +170,10 @@
get-balance (as-contract tx-sender))
ERR-REPAY-FAILED))
)
;; F-9 fix: reentrancy guard. A receiver reentering deposit/withdraw during
;; the callback below now hits ERR-REENTRANT, so that call changes nothing.
(asserts! (not (var-get reentrancy-locked)) ERR-REENTRANT)
(var-set reentrancy-locked true)
(asserts! (not (var-get paused)) ERR-PAUSED)
(asserts! (> amount u0) ERR-ZERO-AMOUNT)
(asserts! (<= amount (var-get max-single-loan)) ERR-EXCEEDS-LIMIT)
Expand All @@ -173,6 +199,7 @@
(var-set total-loans (+ (var-get total-loans) u1))
(var-set total-volume (+ (var-get total-volume) amount))
(var-set total-fees (+ (var-get total-fees) (- reserve-after reserve-before)))
(var-set reentrancy-locked false)
(ok true)
)
)
Expand Down
34 changes: 30 additions & 4 deletions contracts/flashstack-stx-pool-v3.clar
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@
;; Here the update only PROPOSES; the new admin must call accept-admin. NOT DEPLOYED.
;; F-8 FIX -- deposit is also gated by pause now (v2's deposit was not; flash-loan was).
;; See docs/security/FINDINGS_REGISTER.md. withdraw stays ungated so LPs can always exit.
;; F-9 FIX -- reentrancy lock on deposit/withdraw/flash-loan, ported from
;; flashstack-pool-v3's pv3-F1 guard. Without it, a flash-loan receiver can
;; "repay" by calling deposit mid-callback and mint shares at the loan-depressed
;; price, diluting every LP (proven on the live v2 pools, ajv.4.8).
;; ============================================================================
;; FlashStack STX Pool v2 -- External Liquidity Provider Model (HARDENED)
;; Anyone can deposit STX and earn yield from flash loan fees.
Expand Down Expand Up @@ -51,6 +55,7 @@
(define-constant ERR-INVALID-FEE (err u407))
(define-constant ERR-NO-SHARES (err u408))
(define-constant ERR-INSUFFICIENT-SHARES (err u409))
(define-constant ERR-REENTRANT (err u411))

;; =============================================
;; Data vars
Expand All @@ -67,6 +72,12 @@
(define-data-var total-volume uint u0)
(define-data-var total-fees uint u0)

;; F-9 fix: one lock shared by deposit/withdraw/flash-loan. This pool holds a
;; single asset, so a bool is the per-asset lock (pool-v3 needs a map keyed by
;; asset). Set first in each entry point, cleared just before its (ok ...); a
;; failed assert aborts the whole call, so a reverted call never leaves it set.
(define-data-var reentrancy-locked bool false)

;; Precision multiplier for share calculations (avoids integer rounding)
(define-constant SHARE-PRECISION u1000000)

Expand Down Expand Up @@ -102,20 +113,25 @@
;; shares = amount * (total_shares + VIRTUAL-SHARES) / (pool_balance + VIRTUAL-ASSETS)
(new-shares (/ (* amount (+ current-shares VIRTUAL-SHARES)) (+ pool-balance VIRTUAL-ASSETS)))
)
;; F-9 fix: reentrancy guard, checked first.
(asserts! (not (var-get reentrancy-locked)) ERR-REENTRANT)
(var-set reentrancy-locked true)
;; F-8 fix: deposit is gated by pause, matching flash-loan and pool-v3's pv3-F3 fix.
;; withdraw is deliberately never gated, so LPs can always still exit.
(asserts! (not (var-get paused)) ERR-PAUSED)
(asserts! (> amount u0) ERR-ZERO-AMOUNT)

;; Transfer STX from depositor to pool
(unwrap! (stx-transfer? amount depositor (as-contract tx-sender)) ERR-REPAY-FAILED)

;; Credit shares
;; Credit shares (effects before interaction, matching pool-v3; if the
;; transfer below fails the whole call reverts, these writes included)
(map-set lp-shares depositor
(+ (default-to u0 (map-get? lp-shares depositor)) new-shares)
)
(var-set total-shares (+ current-shares new-shares))

;; Transfer STX from depositor to pool
(unwrap! (stx-transfer? amount depositor (as-contract tx-sender)) ERR-REPAY-FAILED)

(var-set reentrancy-locked false)
(ok new-shares)
)
)
Expand All @@ -131,6 +147,10 @@
;; STX owed = shares * (pool_balance + VIRTUAL-ASSETS) / (total_shares + VIRTUAL-SHARES)
(stx-amount (/ (* shares (+ pool-balance VIRTUAL-ASSETS)) (+ current-shares VIRTUAL-SHARES)))
)
;; F-9 fix: reentrancy guard. Withdraw has no callback surface of its own;
;; this blocks a flash-loan callback from reentering withdraw (pool-v3 parity).
(asserts! (not (var-get reentrancy-locked)) ERR-REENTRANT)
(var-set reentrancy-locked true)
(asserts! (> shares u0) ERR-ZERO-AMOUNT)
(asserts! (>= depositor-shares shares) ERR-INSUFFICIENT-SHARES)
(asserts! (> stx-amount u0) ERR-ZERO-AMOUNT)
Expand All @@ -142,6 +162,7 @@
;; Send STX back
(unwrap! (as-contract (stx-transfer? stx-amount tx-sender withdrawer)) ERR-REPAY-FAILED)

(var-set reentrancy-locked false)
(ok stx-amount)
)
)
Expand All @@ -160,6 +181,10 @@
(fee (if (> raw-fee u0) raw-fee u1))
(reserve-before (stx-get-balance (as-contract tx-sender)))
)
;; F-9 fix: reentrancy guard. A receiver reentering deposit/withdraw during
;; the callback below now hits ERR-REENTRANT, so that call changes nothing.
(asserts! (not (var-get reentrancy-locked)) ERR-REENTRANT)
(var-set reentrancy-locked true)
(asserts! (not (var-get paused)) ERR-PAUSED)
(asserts! (> amount u0) ERR-ZERO-AMOUNT)
(asserts! (<= amount (var-get max-single-loan)) ERR-EXCEEDS-LIMIT)
Expand All @@ -183,6 +208,7 @@

;; Fee stays in pool -- automatically increases share value for all LPs

(var-set reentrancy-locked false)
(ok true)
)
)
Expand Down
33 changes: 30 additions & 3 deletions contracts/test/flashstack-sbtc-pool-v3.clar
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@
;; Here the update only PROPOSES; the new admin must call accept-admin. NOT DEPLOYED.
;; F-8 FIX -- deposit is also gated by pause now (v2's deposit was not; flash-loan was).
;; See docs/security/FINDINGS_REGISTER.md. withdraw stays ungated so LPs can always exit.
;; F-9 FIX -- reentrancy lock on deposit/withdraw/flash-loan, ported from
;; flashstack-pool-v3's pv3-F1 guard. Without it, a flash-loan receiver can
;; "repay" by calling deposit mid-callback and mint shares at the loan-depressed
;; price, diluting every LP (proven on the live v2 pools, ajv.4.8).
;; ============================================================================
;; FlashStack sBTC Pool v2 (HARDENED)
;;
Expand Down Expand Up @@ -52,6 +56,7 @@
(define-constant ERR-NO-SHARES (err u708))
(define-constant ERR-INSUFFICIENT-SHARES (err u709))
(define-constant ERR-TRANSFER-FAILED (err u710))
(define-constant ERR-REENTRANT (err u712))

;; =============================================
;; State
Expand All @@ -68,6 +73,12 @@
(define-data-var total-volume uint u0)
(define-data-var total-fees uint u0)

;; F-9 fix: one lock shared by deposit/withdraw/flash-loan. This pool holds a
;; single asset, so a bool is the per-asset lock (pool-v3 needs a map keyed by
;; asset). Set first in each entry point, cleared just before its (ok ...); a
;; failed assert aborts the whole call, so a reverted call never leaves it set.
(define-data-var reentrancy-locked bool false)

(define-constant SHARE-PRECISION u100000000) ;; 1e8 - matches sBTC sat precision

;; F-1 fix (v2): virtual shares + virtual assets (OpenZeppelin ERC-4626
Expand All @@ -94,17 +105,23 @@
;; shares = amount * (total_shares + VIRTUAL-SHARES) / (pool_balance + VIRTUAL-ASSETS)
(new-shares (/ (* amount (+ current-shares VIRTUAL-SHARES)) (+ pool-balance VIRTUAL-ASSETS)))
)
;; F-9 fix: reentrancy guard, checked first.
(asserts! (not (var-get reentrancy-locked)) ERR-REENTRANT)
(var-set reentrancy-locked true)
;; F-8 fix: deposit is gated by pause, matching flash-loan and pool-v3's pv3-F3 fix.
;; withdraw is deliberately never gated, so LPs can always still exit.
(asserts! (not (var-get paused)) ERR-PAUSED)
(asserts! (> amount u0) ERR-ZERO-AMOUNT)
;; Effects before interaction, matching pool-v3; if the transfer below
;; fails the whole call reverts, these writes included.
(map-set lp-shares depositor
(+ (default-to u0 (map-get? lp-shares depositor)) new-shares))
(var-set total-shares (+ current-shares new-shares))
(unwrap!
(contract-call? .sbtc-token
transfer amount depositor (as-contract tx-sender) none)
ERR-TRANSFER-FAILED)
(map-set lp-shares depositor
(+ (default-to u0 (map-get? lp-shares depositor)) new-shares))
(var-set total-shares (+ current-shares new-shares))
(var-set reentrancy-locked false)
(ok new-shares)
)
)
Expand All @@ -118,6 +135,10 @@
get-balance (as-contract tx-sender)) ERR-TRANSFER-FAILED))
(sats-amount (/ (* shares (+ pool-balance VIRTUAL-ASSETS)) (+ current-shares VIRTUAL-SHARES)))
)
;; F-9 fix: reentrancy guard. Withdraw has no callback surface of its own;
;; this blocks a flash-loan callback from reentering withdraw (pool-v3 parity).
(asserts! (not (var-get reentrancy-locked)) ERR-REENTRANT)
(var-set reentrancy-locked true)
(asserts! (> shares u0) ERR-ZERO-AMOUNT)
(asserts! (>= depositor-shares shares) ERR-INSUFFICIENT-SHARES)
(asserts! (> sats-amount u0) ERR-ZERO-AMOUNT)
Expand All @@ -127,6 +148,7 @@
(as-contract (contract-call? .sbtc-token
transfer sats-amount tx-sender withdrawer none))
ERR-TRANSFER-FAILED)
(var-set reentrancy-locked false)
(ok sats-amount)
)
)
Expand All @@ -148,6 +170,10 @@
get-balance (as-contract tx-sender))
ERR-REPAY-FAILED))
)
;; F-9 fix: reentrancy guard. A receiver reentering deposit/withdraw during
;; the callback below now hits ERR-REENTRANT, so that call changes nothing.
(asserts! (not (var-get reentrancy-locked)) ERR-REENTRANT)
(var-set reentrancy-locked true)
(asserts! (not (var-get paused)) ERR-PAUSED)
(asserts! (> amount u0) ERR-ZERO-AMOUNT)
(asserts! (<= amount (var-get max-single-loan)) ERR-EXCEEDS-LIMIT)
Expand All @@ -173,6 +199,7 @@
(var-set total-loans (+ (var-get total-loans) u1))
(var-set total-volume (+ (var-get total-volume) amount))
(var-set total-fees (+ (var-get total-fees) (- reserve-after reserve-before)))
(var-set reentrancy-locked false)
(ok true)
)
)
Expand Down
Loading
Loading