Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
eco-tests changed — indexer review requiredThis PR modifies files under Changed files
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| fn on_runtime_upgrade() -> Weight { | ||
| let _ = | ||
| T::Pool::register_pallet_hotkey(&Self::pallet_account(), &T::PalletHotkey::get()); |
There was a problem hiding this comment.
[CRITICAL] Custody hotkey can be preclaimed before the upgrade
This migration ignores the registration result and never verifies that PalletHotkey belongs to the pallet account. The hotkey address is public and deterministic, while create_account_if_non_existent is a no-op when it already exists. An attacker can therefore claim it before this runtime upgrade; subsequent derivative alpha is staked under an attacker-owned hotkey, which the owner can migrate through swap_hotkey. Abort the upgrade on an ownership collision or use a custody identity that cannot be externally claimed, and verify ownership before accepting positions.
There was a problem hiding this comment.
Fixed in 98f0797. PalletHotkey is no longer a compile-time constant. on_runtime_upgrade now calls claim_hotkey, which hashes (PalletId, "hotkey", parent_hash, nonce) into an address, skips any address that already exists, registers it to the pallet account, and only stores it after pallet_hotkey_registered confirms ownership. The address depends on the parent hash of the upgrade block, so it cannot be known before that block, and hooks run before any extrinsic in it. Until the storage is set every open fails with PalletHotkeyUnset. Tests: upgrade_claims_a_fresh_hotkey_for_the_pallet_account, claim_skips_a_hotkey_someone_registered_first, nothing_opens_until_the_hotkey_is_claimed.
| match &deposit { | ||
| Deposit::Tao(amount) => T::Pool::transfer_tao(&owner, &pallet_account, *amount)?, | ||
| Deposit::Alpha { hotkey, amount } => T::Pool::transfer_staked_alpha( | ||
| &owner, | ||
| hotkey, | ||
| &pallet_account, | ||
| &pallet_hotkey, | ||
| netuid, | ||
| *amount, | ||
| true, | ||
| false, | ||
| )?, | ||
| } | ||
|
|
||
| let (lifted_tao, lifted_alpha) = | ||
| T::Pool::lift_liquidity(netuid, phi, &pallet_account, &pallet_hotkey)?; | ||
| let legs = match side { | ||
| Side::Short => { | ||
| let proceeds = T::Pool::sell_alpha_internal( | ||
| &pallet_account, | ||
| &pallet_hotkey, | ||
| netuid, | ||
| lifted_alpha, | ||
| )?; | ||
| ensure!(!proceeds.is_zero(), Error::<T>::SwapReturnedZero); | ||
| Legs::Short { | ||
| proceeds, | ||
| debt: lifted_alpha, | ||
| escrow: lifted_tao, | ||
| } | ||
| } | ||
| Side::Long => { | ||
| let proceeds = T::Pool::buy_alpha_internal( | ||
| &pallet_account, | ||
| &pallet_hotkey, | ||
| netuid, | ||
| lifted_tao, | ||
| )?; | ||
| ensure!(!proceeds.is_zero(), Error::<T>::SwapReturnedZero); | ||
| Legs::Long { | ||
| proceeds, | ||
| debt: lifted_tao, | ||
| escrow: lifted_alpha, | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| let now = frame_system::Pallet::<T>::block_number(); | ||
| let expires_at = Self::schedule_expiry( |
There was a problem hiding this comment.
[HIGH] Failed opens commit partial transfers and pool mutations
do_open transfers the cushion, lifts liquidity, and executes a swap before this fallible expiry-queue insertion, but neither open nor do_open establishes an outer storage transaction. If this or another later check fails, the extrinsic returns an error while those earlier mutations remain and no Position is recorded. Queue saturation makes this failure adversarially reachable. The same atomicity gap affects roll: settlement can commit before reopening fails. Wrap each complete open and roll operation in one transaction and add regression tests asserting all balances, reserves, stake, queues, and position state are unchanged on every late failure.
There was a problem hiding this comment.
Fixed in 98f0797. do_open now wraps its body in with_storage_layer, the same way do_settle already did, so the cushion transfer, the lift, and the opening swap roll back if the expiry-queue insert (or anything else) fails, regardless of caller. The open and roll extrinsics were already transactional as #[pallet::call] dispatchables, but the guarantee is now local to the function.
🛡️ AI Review — Skeptic (security review)VERDICT: VULNERABLE LOW contributor risk / baseline scrutiny: repository write access, substantive merged contributions, disclosed co-authorship, no listed Gittensor association; feat/derivatives → main. All four prior findings remain unresolved. Financial paths become reachable after governance enables derivatives. No AI-review trust-boundary files changed. Static checks passed: Findings
Prior-comment reconciliation
ConclusionPricing and swap-accounting flaws can lose pool or user funds after activation; parking scans exceed their weight accounting, and CLI quote failures disable requested slippage protection. No evidence of malicious intent was found. 📜 Previous run (superseded)
🔍 AI Review — Auditor (domain review)VERDICT: 👎 Gittensor association UNKNOWN; established high-volume contributor with repository write access. No substantive duplicate PR identified. The implementation and substantive PR description agree, and
Findings
Prior-comment reconciliation
ConclusionBlock merge until benchmark-generated weights replace the placeholders. Accurate accounting is required for dispatchables and automatic settlement processing. 📜 Previous run (superseded)
|
|
🔄 AI review updated — Skeptic: VULNERABLE |
| .cushion | ||
| .alpha_hotkey() | ||
| .cloned(); | ||
| let (tao_back, alpha_back) = Self::do_settle(&owner, netuid, side, Closer::Roll)?; |
There was a problem hiding this comment.
[HIGH] Failed rolls permanently settle the old position
do_settle commits its own storage layer before the top-up is validated or do_open runs. Dispatch errors do not automatically roll back earlier writes, so TopUpMismatch, a disabled side, a full expiry queue, a changed pool cap, or another reopen failure returns an error after closing the user's position and executing its settlement swaps. Wrap the entire settle-and-reopen sequence in one outer storage layer so any later error restores the original position and financial state.
|
🔄 AI review updated — Skeptic: VULNERABLE |
| .cushion | ||
| .alpha_hotkey() | ||
| .cloned(); | ||
| let (tao_back, alpha_back) = Self::do_settle(&owner, netuid, side, Closer::Roll)?; |
There was a problem hiding this comment.
[HIGH] Failed rolls permanently settle the old position
do_settle commits its own storage layer before do_open runs. If deposit construction or reopening subsequently fails—for example with TopUpMismatch, DepositTooLow, PoolCapExceeded, or ExpiryQueueFull—roll returns an error but the old position has already been removed and paid out. Wrap the entire settle-and-reopen sequence in one outer storage layer so every later error rolls back settlement.
There was a problem hiding this comment.
Fixed in 0111d05. do_roll now runs settle and reopen inside one with_storage_layer, so any error from deposit construction or do_open rolls the settlement back too. failed_roll_leaves_the_old_position_untouched asserts the position, footprint, owner balance, and pool reserves are unchanged after a TopUpMismatch roll; roll_settles_and_reopens_with_the_payout covers the happy path.
|
🔄 AI review updated — Skeptic: VULNERABLE |
| //! These are hand-written placeholders sized from storage reads and writes so the pallet can be | ||
| //! wired up. CI's reference benchmark run replaces them with measured values. |
There was a problem hiding this comment.
[HIGH] Replace placeholder weights before merge
These unmeasured constants govern three swap-heavy dispatchables and are also used to meter on_idle expiry settlement and dissolution cleanup. If close() underestimates execution, automatic processing can consume more block resources than charged or metered. Run the reference benchmarks and commit their generated WeightInfo output before enabling this pallet; a future CI benchmark run is not sufficient protection for the code merged here.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
| //! These are hand-written placeholders sized from storage reads and writes so the pallet can be | ||
| //! wired up. CI's reference benchmark run replaces them with measured values. |
There was a problem hiding this comment.
[HIGH] Replace placeholder weights before merge
These estimates are used by open, close, roll, and the on_idle weight meter despite covering multi-swap and extensive storage paths. Unmeasured undercharging can admit more execution than a block's weight limit and let expiry processing overrun its budget. Run the pallet benchmarks on reference hardware and commit the generated WeightInfo implementation before enabling this pallet.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
| //! These are hand-written placeholders sized from storage reads and writes so the pallet can be | ||
| //! wired up. CI's reference benchmark run replaces them with measured values. |
There was a problem hiding this comment.
[HIGH] Replace placeholder weights before merge
These weights govern three stateful dispatchables and automatic expiry settlement, but are explicitly hand-written estimates. Underestimated execution or proof-size costs can let blocks exceed their resource limits. Run the pallet benchmarks on the reference hardware and commit the generated WeightInfo before merging; a future CI benchmark patch is not sufficient for release-ready runtime code.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
bd247db to
f487fbb
Compare
| //! These are hand-written placeholders sized from storage reads and writes so the pallet can be | ||
| //! wired up. CI's reference benchmark run replaces them with measured values. |
There was a problem hiding this comment.
[HIGH] Replace placeholder weights before merge
This remains explicitly placeholder resource accounting for new economic dispatchables and automatic on_idle/dissolution work. Storage read/write estimates do not establish execution time or proof size, and underestimated weights can admit excessive block work. Run the reference benchmarks and commit their generated WeightInfo output before merging; merely scheduling a future CI benchmark is insufficient for a merge-ready runtime.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
| //! These are hand-written placeholders sized from storage reads and writes so the pallet can be | ||
| //! wired up. CI's reference benchmark run replaces them with measured values. |
There was a problem hiding this comment.
[HIGH] Replace placeholder weights before merge
These weights are explicitly handwritten estimates. They meter the new dispatchables as well as on_idle expiry sweeping and resumable subnet dissolution, so an underestimate can permit substantially more pool and storage work than the block budget accounts for. Run the existing benchmarks on reference hardware and commit their generated WeightInfo output before enabling this pallet.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
Brings in #3162 (SharePool root-cause fix, childkey threshold re-checks, rustls audit fix, lock-dust test fix). Conflicts resolved: - runtime/src/proxy_filters/mod.rs: the subtractive filter tests keep DerivativesCalls in the denied set for NonTransfer/NonFungible and take main's SudoCalls / SubtensorValueCalls exclusions. - sdk error map and descriptions: main's classification and prose for FundsNotSettled and OrderSignerFrozen; the Derivatives block kept. - generated bindings, docs, and catalogs: taken from main for now and regenerated in the next commit from a node built from this tree. spec_version is 462 (460 = hotfix 2, 461 = baskets). derivatives_pool.rs follows the SharePool fix's contract: every decrease of the pallet's alpha checks that the share pool debited exactly what was asked (NotEnoughStakeToWithdraw otherwise), as remove_stake and move_stake now do, so a short debit can never be sold, returned, recycled, or handed on in full. recycle_alpha is transactional like its siblings. Co-authored-by: Arbos <unarbos@users.noreply.github.com>
The release page moves from /releases/v456-upgrade to /releases/v462-upgrade (460 is hotfix 2, 461 is baskets); the guide, the releases index, and the sitemap follow. Co-authored-by: Arbos <unarbos@users.noreply.github.com>
From a release node built from this tree (spec_version 462), run as --dev --tmp; codegen.check --drift/--names/--coverage/--units/--namespaces and export_beta_baselines_rs.py --check pass. The merge had duplicated BasketDepositPending in error_map.py and error_descriptions/subtensor.py; main's entries are kept. Reference docs regenerated with scripts/generate.py. Co-authored-by: Arbos <unarbos@users.noreply.github.com>
Brings in #3164 (share-pool epoch on open and on both denominator transitions). spec_version is 463: 460 is main, 461 is hotfix 2, 462 is baskets. The shorts release page moves to /releases/v463-upgrade and the guide, releases index, and sitemap follow. Generated bindings are taken from main here and regenerated for 463 in the next commit. Co-authored-by: Arbos <unarbos@users.noreply.github.com>
From a release node built from this tree (spec_version 463), run as --dev --tmp; codegen.check --drift/--names/--coverage/--units/--namespaces pass; generate.py --check is clean. Co-authored-by: Arbos <unarbos@users.noreply.github.com>
| if moving.0.is_zero() { | ||
| return Self { | ||
| short: spot, | ||
| long: spot, | ||
| }; |
There was a problem hiding this comment.
[HIGH] Zero moving price restores manipulable dissolution payouts
pallets/derivatives/src/position.rs:83-87 cash-settles both sides at spot when the moving price is zero, while lift_tranche permits positions without an initialized moving price. Once derivatives are enabled, a short opened before EMA initialization can receive a dissolution payout that includes the price decline caused by its own opening sale, at the pool's expense. The added test a_lone_short_that_moved_the_price_keeps_that_move_only_where_there_is_no_moving_price explicitly asserts this excess payout. Require an initialized reference before opening positions and use a manipulation-resistant settlement policy if that reference is unavailable; the zero-price fallback must not select spot.
| let unused = tao.saturating_sub(swap.amount_paid_in.saturating_add(swap.fee_paid)); | ||
| if !unused.is_zero() { | ||
| Self::transfer_tao_from_subnet(netuid, coldkey, unused)?; | ||
| } | ||
| Ok(bought) |
There was a problem hiding this comment.
[HIGH] Partial swaps leave refunded assets without position accounting
pallets/subtensor/src/staking/derivatives_pool.rs:269-273 refunds unused TAO but returns only purchased alpha. The swap engine permits partial fills at its finite price limits. swap_until nevertheless counts the entire requested input as spent, and burn_interest treats a successful partial purchase as payment of all interest. The sell helper similarly restores unused alpha without reporting it to its callers. Consequently, settlement can underpay an owner, interest can be marked paid without reaching the pool, and refunded assets remain in shared custody without a position claim. The new exact stake-debit checks do not address this. Return and account for actual input consumed and all remainders throughout the callers, or atomically reject partial fills.
| if meter.try_consume(T::DbWeight::get().reads(1)).is_err() { | ||
| return meter.consumed(); | ||
| } | ||
| let parked: sp_std::vec::Vec<NetUid> = Parked::<T>::iter_keys().collect(); |
There was a problem hiding this comment.
[MEDIUM] Parking hook scans all entries before checking their weight
pallets/derivatives/src/settle.rs:763 reads and allocates every parked subnet key after charging only one database read. The per-entry budget check happens afterward, so even an idle budget too small to process one entry still scans the entire map. Accumulated parked entries therefore cause repeated work outside the reported block weight. Iterate incrementally, reserve weight before advancing the iterator, and retain a cursor so entries that remain parked cannot indefinitely prevent later entries from being examined.
|
🔄 AI review updated — Skeptic: VULNERABLE |
Brings in the childkey_threshold_suspended read and the refreshed docs-preview lockfile. The one conflict, the js-yaml pin in .github/docs-preview-vercel/package.json, takes main's 4.3.2 to match main's lockfile. spec_version stays 463. The new read's docs page and the reads catalog are re-anchored to this branch's pallets/subtensor line numbers with scripts/generate.py. Co-authored-by: Arbos <unarbos@users.noreply.github.com>
| if moving.0.is_zero() { | ||
| return Self { | ||
| short: spot, | ||
| long: spot, | ||
| }; |
There was a problem hiding this comment.
[HIGH] Zero moving price restores manipulable dissolution payouts
pallets/derivatives/src/position.rs:83–87 settles both sides at manipulable spot when the moving price is zero. This remains reachable: is_dynamic does not require emissions to have started, smoothed_reserves falls back to live reserves, and lift_tranche accepts such pools. A short on an unstarted subnet can therefore lower spot before dissolution and have its alpha debt cash-settled below the protected reference value, retaining proceeds at the pool's expense. The zero-price fallback also disables liquidity parking. Require a valid initialized reference price before opening positions and preserve that protection through dissolution.
| let unused = tao.saturating_sub(swap.amount_paid_in.saturating_add(swap.fee_paid)); | ||
| if !unused.is_zero() { | ||
| Self::transfer_tao_from_subnet(netuid, coldkey, unused)?; | ||
| } | ||
| Ok(bought) |
There was a problem hiding this comment.
[HIGH] Partial swaps leave refunded assets without position accounting
pallets/subtensor/src/staking/derivatives_pool.rs:269–273 refunds unused TAO but returns only alpha output; the sell helper similarly hides refunded alpha. The swap engine can successfully partially fill at its finite price limits. Callers consequently book the entire requested input as spent: swap_until overstates spending, burn_interest treats refunded TAO as paid interest, and opening/closing legs omit unused assets. Funds remain on the shared pallet account without a position or pool claim, reducing payouts or pool repayments. Return actual input consumption and account for every refund throughout settlement, or reject partial fills atomically.
| if meter.try_consume(T::DbWeight::get().reads(1)).is_err() { | ||
| return meter.consumed(); | ||
| } | ||
| let parked: sp_std::vec::Vec<NetUid> = Parked::<T>::iter_keys().collect(); |
There was a problem hiding this comment.
[MEDIUM] Parking hook scans all entries before checking their weight
pallets/derivatives/src/settle.rs:763 reads and allocates every parked key after charging only one database read. The per-release budget is checked afterward, so even an idle allowance too small for one release triggers the entire scan. Parking entries across subnets therefore causes repeated work outside the reported block weight. Iterate incrementally, charge before advancing the iterator, and retain a cursor so persistently parked entries do not starve later entries.
|
🔄 AI review updated — Skeptic: VULNERABLE |
Test-only and btcli-only changes from main (flaky unit test, wallet balance decimals). No conflicts; spec_version stays 463. Co-authored-by: Arbos <unarbos@users.noreply.github.com>
| return Self { | ||
| short: spot, | ||
| long: spot, | ||
| }; |
There was a problem hiding this comment.
[HIGH] Zero moving price restores manipulable dissolution payouts
pallets/derivatives/src/position.rs:84-87 settles both sides at spot when the moving price is zero. This state is reachable: do_start_call enables trading immediately, while moving-price initialization occurs in a later block, and lift_tranche does not require an initialized moving price. A short opened before initialization can therefore be cash-settled at its own depressed spot price if the subnet is dissolved in that window, underpaying its alpha debt at the pool's expense. The zero-price fallback in spot_is_off_moving also disables parking protection. Require a valid initialized reference price before borrowing and avoid spot-only settlement or liquidity-release fallbacks.
| let unused = tao.saturating_sub(swap.amount_paid_in.saturating_add(swap.fee_paid)); | ||
| if !unused.is_zero() { | ||
| Self::transfer_tao_from_subnet(netuid, coldkey, unused)?; | ||
| } | ||
| Ok(bought) |
There was a problem hiding this comment.
[HIGH] Partial swaps leave refunded assets without position accounting
pallets/subtensor/src/staking/derivatives_pool.rs:269-273 refunds unused input but returns only output. The swap engine can successfully fill partially at its finite price limits. Callers consequently cannot distinguish the requested input from actual spending: swap_until increments spent by the requested amount, burn_interest treats success as spending all interest, and opening/closing legs omit refunded assets. The sell wrapper has the same problem with refunded alpha. This can strand user or pool assets in the shared pallet account and overstate settlement costs. Return actual input consumption and account for every refund, or atomically reject partial fills.
| if meter.try_consume(T::DbWeight::get().reads(1)).is_err() { | ||
| return meter.consumed(); | ||
| } | ||
| let parked: sp_std::vec::Vec<NetUid> = Parked::<T>::iter_keys().collect(); |
There was a problem hiding this comment.
[MEDIUM] Parking hook scans all entries before checking their weight
pallets/derivatives/src/settle.rs:763 reads and allocates every parked key after charging only one database read. The per-release budget is checked afterward, so even a block with insufficient weight for one release scans the entire map. Users can populate entries across subnets by closing positions outside the price band, causing repeated unaccounted work in on_idle. Iterate incrementally with weight charged before advancing, and persist a cursor if needed to avoid repeatedly scanning the same entries.
| except Exception: | ||
| # The floor is a convenience; a quoting hiccup must not block the add. | ||
| quoted = None | ||
| floor = _floor(quoted, max_slippage) if quoted is not None else Balance.from_rao(0) |
There was a problem hiding this comment.
[MEDIUM] Quote failures disable the requested slippage limit
sdk/python/bittensor/cli/commands/deriv.py:207-210 catches any quote failure and submits with min_amount_out=0, even when the user requested --max-slippage 1. For an opposite-side add, this permits an unrestricted reduction or close that can lose the entire payout to price manipulation. close_position uses the same fallback. The generic warning does not enforce the requested limit, particularly for unattended submissions. Abort when a required settlement quote fails; require an explicit --max-slippage 100 to submit without protection.
|
🔄 AI review updated — Skeptic: VULNERABLE |
btcli-only change from main (typer exit on prompt abort). No conflicts; spec_version stays 463. Co-authored-by: Arbos <unarbos@users.noreply.github.com>
| if moving.0.is_zero() { | ||
| return Self { | ||
| short: spot, | ||
| long: spot, | ||
| }; |
There was a problem hiding this comment.
[HIGH] Zero moving price restores manipulable dissolution payouts
When the moving price is zero, dissolution values both sides at manipulable spot. lift_tranche permits these pools, and smoothed_reserves also falls back to live reserves. A short opened before dissolution therefore lowers the price used to repay its own debt and can extract pool TAO through its own price impact. The related spot_is_off_moving fallback also disables parking in this state, allowing liquidity to return at a manipulated price.
Require an initialized, usable reference price before lending and preserve conservative settlement/parking behavior when that reference is unavailable. Cover zero-moving-price pools in the manipulation regression tests.
| let unused = tao.saturating_sub(swap.amount_paid_in.saturating_add(swap.fee_paid)); | ||
| if !unused.is_zero() { | ||
| Self::transfer_tao_from_subnet(netuid, coldkey, unused)?; | ||
| } | ||
| Ok(bought) |
There was a problem hiding this comment.
[HIGH] Partial swaps leave refunded assets without position accounting
The swap engine can partially fill at its price limit. This helper refunds unused TAO but returns only alpha output; swap_until consequently counts the requested input as spent, while burn_interest treats the entire interest payment as consumed. The sell helper similarly restores unsold alpha without reporting it. Opening and settlement then omit these refunded assets from position accounting, leaving pool or user funds stranded on the shared pallet account and potentially overstating debt or shortfall.
Propagate actual input spent and unused input through the interface and account for both tokens in every caller, or reject partial fills atomically.
| if meter.try_consume(T::DbWeight::get().reads(1)).is_err() { | ||
| return meter.consumed(); | ||
| } | ||
| let parked: sp_std::vec::Vec<NetUid> = Parked::<T>::iter_keys().collect(); |
There was a problem hiding this comment.
[MEDIUM] Parking hook scans all entries before checking their weight
iter_keys().collect() reads and allocates the entire parked map after charging only one database read. Even when the remaining idle budget cannot cover one release, every parked subnet is scanned first. User-triggered parking can therefore cause recurring storage work beyond the hook's reported weight.
Iterate incrementally and reserve weight before fetching each key. Use a resumable cursor so entries beyond the first budget-sized batch can also progress.
| except Exception: | ||
| # The floor is a convenience; a quoting hiccup must not block the add. | ||
| quoted = None | ||
| floor = _floor(quoted, max_slippage) if quoted is not None else Balance.from_rao(0) |
There was a problem hiding this comment.
[MEDIUM] Quote failures disable the requested slippage limit
A quote exception replaces the requested slippage protection with min_amount_out = 0 and proceeds to submission. For an opposite-side add, settles also remains false, skipping the settlement review section. close_position has the same fail-open behavior. Thus an RPC or decoding failure can turn a default 1% slippage request into an unrestricted settlement vulnerable to adverse price movement or sandwiching.
Abort when a protected settlement cannot be quoted. Require an explicit opt-out such as --max-slippage 100 before submitting with no floor.
|
🔄 AI review updated — Skeptic: VULNERABLE |
test-balancer-edge-emission-issuance saw SubnetTAO grow by more than the emission counters in the injection block twice in a row; the same runtime passed it twice earlier with identical emission figures, and the extra TAO varies (64k, 1.24M rao), which points at the per-block basket deposit drain landing on the chosen subnet rather than at this branch. Co-authored-by: Arbos <unarbos@users.noreply.github.com>
| return Self { | ||
| short: spot, | ||
| long: spot, | ||
| }; |
There was a problem hiding this comment.
[HIGH] Zero moving price restores manipulable dissolution payouts
pallets/derivatives/src/position.rs:87 falls back to spot for both dissolution prices when the moving price is zero; spot_is_off_moving likewise disables parking. This state is reachable: opening checks do not require an initialized moving price, smoothed_reserves falls back to live reserves, and the EMA updates only for emitting subnets. After derivatives are enabled, a position on an uninitialized pool can therefore settle against a manipulated spot price without either safeguard. A short can reduce its cash-settled debt by pushing spot down before dissolution, leaving less TAO for the pool's stakers. Require a valid initialized reference price before lending and preserve a manipulation-resistant reference for settlement and liquidity returns.
| let unused = tao.saturating_sub(swap.amount_paid_in.saturating_add(swap.fee_paid)); | ||
| if !unused.is_zero() { | ||
| Self::transfer_tao_from_subnet(netuid, coldkey, unused)?; | ||
| } | ||
| Ok(bought) |
There was a problem hiding this comment.
[HIGH] Partial swaps leave refunded assets without position accounting
pallets/subtensor/src/staking/derivatives_pool.rs:269-273 refunds unused input to the shared pallet account but returns only the output. The swap engine permits price-limit-clamped partial fills. Consequently, swap_until counts the requested input as spent, settlement subtracts that amount from the owner's pot, and burn_interest treats a successful partial swap as consuming all interest. The refunded TAO remains without a position or pool claim. The sell helper has the equivalent problem for refunded alpha. Return actual input consumption and account for every refund throughout opening, settlement, and interest collection, or atomically reject partial fills.
| if meter.try_consume(T::DbWeight::get().reads(1)).is_err() { | ||
| return meter.consumed(); | ||
| } | ||
| let parked: sp_std::vec::Vec<NetUid> = Parked::<T>::iter_keys().collect(); |
There was a problem hiding this comment.
[MEDIUM] Parking hook scans all entries before checking their weight
pallets/derivatives/src/settle.rs:763 collects every parked subnet after charging only one database read. Even when the remaining budget cannot afford a single release, the hook scans and allocates the entire map before reaching the per-entry weight check. Positions can create parked entries across subnets, causing repeated work beyond the declared idle budget. Iterate incrementally and reserve weight before fetching each entry, including the terminal lookup.
| except Exception: | ||
| # The floor is a convenience; a quoting hiccup must not block the add. | ||
| quoted = None | ||
| floor = _floor(quoted, max_slippage) if quoted is not None else Balance.from_rao(0) |
There was a problem hiding this comment.
[MEDIUM] Quote failures disable the requested slippage limit
sdk/python/bittensor/cli/commands/deriv.py:210 catches quote failures and continues with min_amount_out=0, even when the user requested the default 1% slippage limit. The close command does the same. A transient RPC or decoding failure thus turns a protected settlement into an unrestricted one, permitting adverse price movement to consume the payout. Abort when a required quote fails; allow submission without a floor only when the user explicitly selects --max-slippage 100.
|
🔄 AI review updated — Skeptic: VULNERABLE |
This branch was successfully deployed
3 participants
Summary
Shorts-only launch.
pallet-derivativesships both sides, and only shorts are enabled at launch. Longs are designed, implemented, tested, and benchmarked, and are disabled behind their own governance switch pending a decision on their collateral and leverage. TAO-only collateral is unchanged.DerivativesEnabled(defaultfalse) gatesaddand onlyadd: while it is off every open, grow, reduce, and flip fails withDerivativesDisabled, checked first indo_add.LongsEnabled(defaultfalse) gates the long side: checked right afterDerivativesEnabledand before the leverage, on what the call would leave open, not on the side asked for. Opening a long, growing one, or a long-side add on a short that would flip it through zero (with at leastMinDepositpast the flip point) fails withLongsDisabled; a long-side add that only reduces or closes a short goes through, as does every short-side add. Neither switch stops aclose: owners can always exit, on a short or on a long opened while longs were on, and the weekly interest collection, forfeits, parked-liquidity releases, and dissolution settlement of open positions run unchanged. Root flips them withsudo_set_derivatives_enabled(enabled)(call index 3;DerivativesToggled) andsudo_set_longs_enabled(enabled)(call index 4;LongsToggled); both are in theDerivativesCallsproxy group.derivatives_params/btcli deriv paramsreport them asenabledandlongs_enabled. The launch plan is the network-wide switch on, the long switch off.long_interest_rateis inert until longs are enabled.pallet-derivatives(index 33): longs and shorts on a subnet's alpha, borrowed from the subnet's own pool. A position lifts a slicephiof both reserves without moving price, trades one half through the ordinary swap, and reverses the trade at settlement. Nothing is minted or burned; the pool only ever gets its own liquidity back.add(netuid, side, deposit, leverage_percent)opens, grows, reduces, and flips. Adding on the held side folds a new tranche in; adding on the other side settles that share at the current price; asking for more than is held closes the position and opens the rest (if it reachesMinDeposit) on the new side.closesettles in full. Every quantity in a position is a plain sum over its tranches;do_addruns in one storage layer. The flip arithmetic is one function,flip_surplus, used both to predict what an add would leave open (for the long switch) and to settle it.DerivativesParams,sudo_set_params):pool_share(25%), the most all open positions of one side on one subnet may borrow of the lent reserve;short_interest_rate(52% a year) andlong_interest_rate(26% a year), each a flat rate charged on a tranche's TAO exposure and fixed per tranche when it is added at its side's rate. Shorts pay more because the pool carries open-ended exposure to a short that nobody but its owner can close, while a long is the buy pressure the design wants. The starting values are theDEFAULT_SHORT_INTEREST_RATE_PERCENT/DEFAULT_LONG_INTEREST_RATE_PERCENTconstants inposition.rs.sudo_set_paramsrefuses a zero rate on either side (ZeroInterestRate); apool_shareof zero pauses new adds and open positions still settle. No utilization or floating rates, and no per-subnet overrides. No migration: the pallet has never shipped, soParamshas never been written on any network.depositis taken from the caller's free balance and comes back as TAO.MaxShortLeverage100 (1x) andMaxLongLeverage150 (1.5x; binds nothing until longs are enabled). At 1x a long can never cost the pool anything. The long ceiling is bounded by one attack: the largest long the cap admits can dump alpha it holds outside into its own lifted price and abandon the debt, which pays iffL > 1 + sqrt(1 - pool_share)(1.87x at 25%; 1.57x at a TAO weight of 0.7). 1.5x is below the bound for every share up to 25% and every balancer weight.interest_per_year = rate(side) × exposure_taoper tranche (params.interest_for(side, lifted_tao)inlift_tranche), accrued pro rata per block (BLOCKS_PER_YEAR = 365 × 7,200). Each position has adueblock oneINTEREST_PERIOD(50,400 blocks, 7 days) ahead, listed in theDuequeue.on_initialize→collect_duewalks the queue fromNextDue, at mostCOLLECTIONS_PER_BLOCK = 20collections a block, and defers a collection that cannot go through by one block. Each collection takes the interest due out of the cushion, buys alpha with it, and recycles the alpha (burn_interest); the pool keeps the TAO. Same-side adds and reductions do not movedue. Settlements pay the interest owed through the same path.closeis owner-only (ensure_signed,Positions::take(owner, …)); there is no third-party closer and no health check.CloserisOwner | Underwater | Starved | Dissolution. A position lives until its owner closes it, its cushion cannot pay at a weekly collection, or the subnet is dissolved. A price move alone never ends it: a doubling only puts a 1x short underwater, where a close returns nothing and the pool keeps the pot, and the owner may keep holding while the cushion pays its weeks. Only the starved case forfeits:forfeithands everything the pallet holds for it back to the pool in kind, with no swap, and the owner gets nothing (Closer::Starved).buy_alpha_for), pays interest, and the rest is the owner's; a long sells its proceeds alpha, repays its TAO debt from the pot, pays interest, and the rest is the owner's. Escrow returns untouched via price-neutralreturn_liquidity.do_settleasks the pool's own quote (quote_buyfor a short's debt,quote_sellfor a long's proceeds). If the share's pot cannot cover the debt plus the interest due, nothing is swapped: the cushion share, proceeds, and escrow go to the pool in kind, the owner is paid nothing,PositionClosedsaysclosed_by: Underwaterand reports the whole debt as the shortfall. Once the owner has lost the cushion a swap could only cost the pool more, and a limit-less market order announced in advance is what a sandwich trades against. The swap remains the last word for a quote that was a rao off. A partial settlement is the same fraction of every leg, with the whole position's interest paid first from the settling share, then from the cushion that stays.return_to_poolcompares the spot price with the subnet's moving price before re-adding a returned pair. If they differ by more thanPARK_THRESHOLD_PERCENT(5%), the pair is parked in the newParkedmap (TAO on the pallet account, alpha at the pallet hotkey;LiquidityParked) instead of deepening the pool at a pushed price.on_idlereleases parked pairs once the spot is back within the band (LiquidityReleased, weight-metered atWeightInfo::release_parkedeach); the dissolution hook returns any parked pair to the reserves before settling positions; parked alpha counts inlong_alpha_outstandingfor the emission price. An honest close on a quiet pool parks nothing.pool_shareis enforced onproceeds + escrowafter the opening swap, not on the equal-weight projectionphi(2 - phi), so a pool whose balancer weights drifted cannot lend one side more than its share (at TAO weight 0.7 the projection admitted a long that took 38% of the alpha reserve).get_emission_alpha_price), so a team cannot long its own subnet for emission. Shorts are left in.DerivativesSettlecleanup phase, before stakers are paid. The hook reads the pool's spot price and the subnet's moving price once, storesDissolutionPrices { short: max(spot, moving), long: min(spot, moving) }inDissolutionPrice, emitsDissolutionPriced { short, long }, and returns any parked pair to the reserves. Every position is then cash-settled independently with no swap and no netting. A short's alpha debt is converted to TAO at the short price rounded up and repaid from cushion + proceeds; a short that pushed the spot down in the dissolving block is charged the moving price it could not push. A long paysdebt + interestfrom its cushion first, then in alpha at the long price rounded up; the pool buys the long's remaining alpha for TAO at the same price as far asSubnetTAOgoes, and any alpha the reserve cannot buy is handed to the owner as stake at the pallet hotkey (hand_alpha) to be paid out with every other stake. No long is paid nothing because another drew the reserve first; order changes only how much of a surplus arrives as TAO now versus alpha later. Weekly interest collection skips positions on a dissolving subnet, so a tick in the window cannot forfeit a position the settlement is about to pay. Settlement is weight-metered and never blocks dissolution; transfer failures are logged.DerivativesPoolInterfaceinswap-interfacewith price-neutrallift_liquidity/return_liquidity, internal buy and sell through the balancer, exact-outputbuy_alpha_for, no-swapquote_buy/quote_sell,recycle_alpha,spot_priceandmoving_priceon the same 32.32 scale,draw_tao,hand_alpha;exp_scaledin the balancer saturates instead of returning 0;SubnetDissolveHook/DerivativesHookincommon; pallet hotkey claimed inon_runtime_upgradefrom the parent block hash; derivatives callsNonCriticalAllowedonly.AddPosition(netuid, side, amount, leverage)andClosePosition(netuid)intents;derivative_position,derivative_positions,derivative_positions_on_subnet,derivatives_params(both rates and both switches asenabled/longs_enabled) reads withinterest_due,runway_days, and estimatedequity_tao;btcli deriv short|long|list|close|params;LongsDisabledhas a description and a remediation that points atderiv short/deriv close. Bindings regenerated from the built node.docs/guides/derivatives.mdx, framed as a shorts-only launch (long sections kept and marked "not enabled at launch"; both switches in the launch-state note and switch section), with the animated lifecycle deck, payoff figure, numbered close flows for shorts and longs, the weekly-interest section, and the dissolution steps; generated query/tx/error pages; the v456 release page and releases index, same framing. Wording rule throughout: a price move alone never ends a position; it can only make it underwater (a close returns nothing, the pool keeps the pot, the owner may keep holding); forfeiture happens only when the cushion cannot pay a weekly collection, or at dissolution.spec_version455 → 456 (rebased on main's release 455).cargo auditaccepts RUSTSEC-2026-0269 (wasmtime 8.0.1, same polkadot-sdk pin as the existing wasmtime ignores; the runtime WASM has no filesystem), and the docs-preview lock pinsfast-uri3.1.7.Weights
pallets/derivatives/src/weights.rsholds measured values from the repo's own benchmark pipeline: therun-benchmarkslabel triggered Validate-Benchmarks on theBenchmarkingrunner (run 34763489612, head b8d2dff, steps 50, repeat 20, standard invocation and.maintain/frame-weight-template.hbs).add,close,collect_interestandrelease_parkedare verbatim from the run'sbench-patchartifact.sudo_set_paramsandsudo_set_derivatives_enabledwere within the 75% drift threshold, so the patch left them; their base weights are the same run'sweight-comparereadings and the file says so inline.sudo_set_longs_enabledwas added after that run and carries a placeholder (thesudo_set_derivatives_enabledreading; same one-bool write);addnow readsLongsEnabledon a long-side call. The next reference run picks both up. The patch also reported drift inpallet_subtensor,pallet_admin_utils,pallet_limit_ordersandpallet_commitments; that drift predates this branch (the same set shows on PR #3155) and was not applied.addclose(1.5 s), 30+35 r / 20+25 wclosecollect_interestrelease_parkedsudo_set_paramssudo_set_derivatives_enabledsudo_set_longs_enabledsudo_set_derivatives_enabled(same shape: one write of one bool), marked as a placeholder in the fileBenchmark setups were re-pointed at the current worst cases (commit b8d2dff). The underwater rule made the old setups the cheap path: a short pumped underwater is handed back in kind with no swap. Measured on the same machine, the old
closesetup came to 145 ms / 15 r / 11 w against 556 ms / 29 r / 21 w for the setup now in tree.addandclosenow settle a covered short the price moved against (300 TAO pumped into a 1000 TAO pool, a week of interest owed; the setup asserts the same coverage testdo_settleapplies), so the exact-output buyback, the interest swap and recycle, the payout and the liquidity return all run.collect_interestmeasures a paid collection (a year of interest, swapped and recycled) rather than a forfeit: a short forfeit measured 101 ms / 15 r / 11 w and a long forfeit 172 ms / 23 r / 17 w against 231 ms / 24 r / 15 w for the paid path, on the same machine.release_parkedwas already at its worst case (both price reads, both tokens returned through the live-pool path). Dissolution pacing atclose()per position was checked against dedicated one-off measurements ofsettle_at_dissolution: 113 ms / 13 r / 12 w for a short and 216 ms / 21 r / 18 w for a long, both underclose(). Those comparison numbers are from a cloud VM (4 vCPU Intel Xeon) whose readings for the committed benchmarks sit about 10% under the runner's; they are there for the ratios, not as committed weights.Test plan
cargo test -p pallet-derivatives(80 tests, 87 withruntime-benchmarks; the long switch: launch default and root origin, a long cannot be opened or grown while off and the switch is checked before the leverage, a short can be reduced and closed with a long-side add but not flipped and a refused flip leaves the short, the balance, and the pool untouched, shorts open, grow, and close as usual, a long opened while on keeps paying interest, can be reduced, and can be closed once the switch is off; the network-wide switch: launch default and root origin, every kind of add refused while off, close allowed while off, interest collected and a starved position forfeited while off; per-side rates: short and long tranches booked at their own rate, a flip books the new side's rate,sudo_set_paramsrejects zero on either side,Paramsreturns both; includingtests/safety.rs: self-sandwich of a close at 0.25x..4x of the reserve is non-positive on both sides, a front-run cannot take from the pool, long-then-dump loses at 1.5x, the cap holds at TAO weight 0.7, a self-impacting short loses at dissolution, two winning longs are both paid, an interest tick during dissolution forfeits nothing, parked liquidity is released and included in dissolution) and the benchmark test suite withruntime-benchmarks; runtime andpallet-subtensorcompile;cargo clippy -p pallet-derivatives --all-targets -D warningscargo fmt --check --allcodegen.check --drift|--coverage|--names|--units|--namespacesagainst a dev node built from this branch;btcli deriv paramsagainst it showsenabled: false, longs_enabled: false;btcli explain LongsDisabledgenerate.py --checkMade with Cursor