-
Notifications
You must be signed in to change notification settings - Fork 316
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[HIGH] Capped lock-aware amount can mutate stake before failing minimum check
For same-subnet lock-aware transfers, this cap can reduce a caller-supplied alpha_amount to a dust-sized selected bucket after any transaction-extension or input-level minimum checks have seen the original amount. The capped value then reaches transfer_stake_within_subnet, which calls transfer_lock and decreases/increases stake before computing tao_equivalent and returning AmountTooLow for sub-minimum transfers. A caller can submit a large alpha_amount, have it capped to a below-minimum locked/unlocked remainder, and still move stake/lock state through a dispatch that reports failure. Validate the capped move_amount against DefaultMinStake before any lock/stake mutation, or move the same-netuid minimum check ahead of transfer_lock and the stake balance updates.
🛡️ AI Review — Skeptic (security review)VERDICT: VULNERABLE BASELINE scrutiny: author has write permission and substantial prior subtensor activity; no Gittensor allowlist hit; branch feat/transfer-free-locked-alpha -> devnet-ready. No Findings
Prior-comment reconciliation
ConclusionThe prior HIGH issue remains: the new lock-aware transfer path caps against coldkey-level lock state while debiting the caller-selected hotkey. This can detach lock state from the stake it is meant to constrain, so the PR remains vulnerable until the cap and lock movement are bound to the selected hotkey. 📜 Previous run (superseded)
🔍 AI Review — Auditor (domain review)VERDICT: 👎 Gittensor: LIKELY by recent subtensor-focused PR history; established contributor with write permission. Prior cross-subnet lock finding remains. PR description is substantive. I did not run builds/tests; the blocking issue is visible statically. I checked local Findings
Prior-comment reconciliation
ConclusionBlocking because 📜 Previous run (superseded)
|
🔄 AI review updated — Skeptic: VULNERABLE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[HIGH] Cross-subnet locked transfers do not move lock state
Some(locked) is passed into the generic transition path even when origin_netuid != destination_netuid, but the cross-subnet branch later uses unstake_from_subnet and stake_into_subnet rather than transfer_lock. That means no lock state is moved to the destination subnet/coldkey. Worse, validate_stake_transition still enforces ensure_available_to_unstake for cross-subnet moves, so a fully locked position fails with StakeUnavailable, while a partially locked position with enough unlocked alpha can succeed by moving unlocked stake and leaving the source lock behind. This contradicts the new extrinsic docs and the PR body claim that locked transfers preserve/move the lock state. Either reject origin_netuid != destination_netuid for this extrinsic, or implement explicit cross-subnet lock migration and add a test that locked=true moves lock state across netuids.
🔄 AI review updated — Skeptic: SAFE Auditor: 👎
🔄 AI review updated — Skeptic: SAFE Auditor: 👎
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.
🔄 AI review updated — Skeptic: VULNERABLE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.
🔄 AI review updated — Skeptic: VULNERABLE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.
🔄 AI review updated — Skeptic: VULNERABLE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.
🔄 AI review updated — Skeptic: VULNERABLE
Description
This change adds
transfer_stake_lock_awareextrinsic, a variant oftransfer_stakethat lets callers choose whether to transfer from the locked or unlocked portion of stake via a newlockedboolean. The transition path now caps the requested amount to the selected bucket, so it transfers the maximum available amount without erroring when the request exceeds locked or unlocked alpha. For locked transfers, the existing lock state follows the moved stake and preserves the lock hotkey and proportional conviction; for unlocked transfers, only free stake moves and lock state remains untouched.Type of Change
Checklist
./scripts/fix_rust.shto ensure my code is formatted and linted correctly