Fix swap_hotkey_v2: moving locks and conviction#2729
Conversation
gztensor
added
the
skip-cargo-audit
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| // 9. Perform the hotkey swap | ||
| // 9. Swap the stake locks | ||
| let (reads, writes) = Self::swap_hotkey_locks(old_hotkey, new_hotkey); |
There was a problem hiding this comment.
[HIGH] Subnet swaps migrate locks for every subnet
swap_hotkey_locks(old_hotkey, new_hotkey) is global: it scans Lock::<T>::iter() and moves all locks from old_hotkey to new_hotkey across all netuids. In this Some(netuid) path, the preconditions only ensure new_hotkey is not registered on the requested subnet, and perform_hotkey_swap_on_one_subnet only moves stake/hotkey metadata for that one subnet. A hotkey owner can therefore perform a single-subnet swap and silently move lock/conviction storage for unrelated subnets where the stake remains on old_hotkey, corrupting lock accounting and potentially freeing locked stake on those subnets. Use a netuid-scoped lock migration here, and keep the global helper only for the all-subnets path.
🛡️ AI Review — Skeptic (security review)VERDICT: VULNERABLE Baseline scrutiny: author has write permission and substantial prior subtensor history; branch fix/hotkey-swap-conviction -> devnet-ready; no .github, dependency, build-script, or lockfile changes. Static review only, per Skeptic constraints. The previous global-lock-migration finding is addressed by the new subnet-scoped lock swap path, but the added lock movement still introduces unbounded storage scans in runtime paths. Findings
Prior-comment reconciliation
ConclusionThe PR is vulnerable because the new subnet hotkey-swap lock migration can scan the entire lock map under a fixed normal extrinsic weight, and the runtime upgrade migration also scans the full map while under-reporting that work. Both paths need bounded/indexed iteration or conservative weight handling before merge. 📜 Previous run (superseded)
# 🔍 AI Review — Auditor (domain review) has not yet run on this PR. |
|
🔄 AI review updated — Skeptic: VULNERABLE |
|
|
||
| // 9. Perform the hotkey swap | ||
| // 9. Swap the stake locks | ||
| let (reads, writes) = Self::swap_hotkey_locks(old_hotkey, new_hotkey); |
There was a problem hiding this comment.
[HIGH] Subnet swaps migrate locks for every subnet
swap_hotkey_on_subnet now calls swap_hotkey_locks(old_hotkey, new_hotkey), but that helper is global: it enumerates every subnet via get_all_subnet_netuids() and transfers every Lock whose hotkey is old_hotkey. In this subnet-scoped extrinsic, perform_hotkey_swap_on_one_subnet only moves stake and subnet state for the requested netuid, so locks and conviction on other subnets can be moved to new_hotkey without moving the corresponding stake or hotkey registration there. Use a subnet-scoped lock migration helper here, or call the global helper only from the all-subnets path.
|
🔄 AI review updated — Skeptic: VULNERABLE |
|
|
||
| // 9. Perform the hotkey swap | ||
| // 9. Swap the stake locks | ||
| let (reads, writes) = Self::swap_hotkey_locks_on_subnet(old_hotkey, new_hotkey, netuid); |
There was a problem hiding this comment.
[HIGH] Subnet hotkey swap scans the entire lock map under fixed weight
This added call reaches swap_hotkey_locks_for_netuids, which uses Lock::<T>::iter() whenever the old hotkey has a relevant aggregate lock on the subnet. swap_hotkey_v2 is still declared with a fixed normal-class pre-dispatch weight, so a single subnet-scoped swap can force a full scan of all (coldkey, netuid, hotkey) lock entries while only being admitted to the block at the static weight. Since Lock is keyed by coldkey first, this cannot be bounded by the target subnet/hotkey without another index or a capped migration-style path. This is a transaction-level DoS/overweight-block risk. Use a bounded/reverse index for locks to old_hotkey on netuid, or do not expose this full-map scan from the normal extrinsic path.
| let locks: Vec<(T::AccountId, LockState)> = Lock::<T>::iter() | ||
| .filter_map(|((coldkey, lock_netuid, hotkey), lock)| { | ||
| (lock_netuid == netuid && hotkey == old_hotkey).then_some((coldkey, lock)) | ||
| }) | ||
| .collect(); | ||
| for (coldkey, _) in &locks { | ||
| Lock::<T>::remove((coldkey.clone(), netuid, old_hotkey.clone())); | ||
| } | ||
| locks | ||
| }; | ||
| let locks_to_fix_count = locks_to_fix.len() as u64; | ||
| weight = weight.saturating_add( | ||
| T::DbWeight::get() | ||
| .reads_writes(locks_to_fix_count.saturating_add(1), locks_to_fix_count), | ||
| ); |
There was a problem hiding this comment.
[HIGH] Runtime upgrade migration undercounts a full Lock iteration
For the coldkey: None fix, this branch iterates every Lock entry and only filters after reading each item. The returned migration weight then charges locks_to_fix_count + 1 reads, which counts only matching locks, not the full map scan. On a large lock map, the runtime upgrade can do substantially more work than it reports, creating an overweight runtime-upgrade block risk. Either avoid the global iterator by enumerating exact coldkeys, or account for every scanned entry and keep the migration within a bounded, known-safe limit.
|
🔄 AI review updated — Skeptic: VULNERABLE |
1 participant
Description
Hotkey swap v2 did not move conviction. This PR fixes the issue.
Related Issue(s)
Type of Change
Checklist
./scripts/fix_rust.shto ensure my code is formatted and linted correctly