Fix hotkey swap conviction locks#2731
Conversation
github-actions
Bot
added
the
hotfix
🚨🚨🚨 HOTFIX DETECTED 🚨🚨🚨It looks like you are trying to merge a hotfix PR into If you are trying to merge a hotfix PR, please complete the following essential steps:
If you do not complete these steps, your hotfix may be inadvertently removed in the future when branches are promoted to |
gztensor
added
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 hotkey swap moves locks on every subnet
swap_hotkey_on_subnet is only supposed to operate on the requested netuid, but swap_hotkey_locks(old_hotkey, new_hotkey) scans all subnets and moves every Lock for old_hotkey to new_hotkey. The caller only checks that new_hotkey is unregistered on the selected subnet, so this can mutate unrelated subnet locks/conviction and potentially interact with registrations or ownership on other subnets. Use a subnet-scoped lock mover that only touches (coldkey, netuid, old_hotkey) plus that subnet's aggregate lock state.
| .map(|lock| vec![(coldkey, lock)]) | ||
| .unwrap_or_default() | ||
| } else { | ||
| let locks: Vec<(T::AccountId, LockState)> = Lock::<T>::iter() |
There was a problem hiding this comment.
[HIGH] Runtime upgrade performs an unbounded Lock scan
For the coldkey: None fix, the migration iterates the entire Lock storage map during on_runtime_upgrade, collects matching entries, then only charges weight based on the number of matches. Lock is keyed by coldkey first, so this is a full-map scan whose execution time scales with all live locks, not the handful of affected rows. A mainnet runtime upgrade must avoid unbounded storage scans or account for them with a proven bounded input; enumerate the affected coldkeys or split the repair into bounded keys instead.
🛡️ AI Review — Skeptic (security review)VERDICT: VULNERABLE LOW scrutiny: write-permission contributor with established subtensor history, author/committer match; direct-to-main hotfix is explicitly justified. No Findings
Prior-comment reconciliation
ConclusionThe PR appears legitimate, but the prior resource-exhaustion issues remain: both the runtime migration and the subnet hotkey-swap dispatch can still scan the full 📜 Previous run (superseded)
# 🔍 AI Review — Auditor (domain review) has not yet run on this PR. |
|
🔄 AI review updated — Skeptic: VULNERABLE |
| .map(|lock| vec![(coldkey, lock)]) | ||
| .unwrap_or_default() | ||
| } else { | ||
| let locks: Vec<(T::AccountId, LockState)> = Lock::<T>::iter() |
There was a problem hiding this comment.
[HIGH] Runtime upgrade still scans the entire Lock map
This Lock::<T>::iter() path remains unbounded in on_runtime_upgrade. It scans every Lock entry just to find the one hotkey/netuid pair with an unknown coldkey, and the reported weight only accounts for locks_to_fix_count, not for every scanned entry. On mainnet, a large lock map can make the runtime upgrade exceed block limits or materially underreport upgrade weight. Require an explicit coldkey for this fix, use a bounded/paged migration, or otherwise avoid whole-map iteration during the upgrade.
|
|
||
| // 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 can scan all locks under a fixed dispatch weight
This new subnet-swap path calls swap_hotkey_locks_on_subnet, which delegates to a helper that runs Lock::<T>::iter() over the entire lock map whenever the old hotkey has any relevant aggregate lock on the subnet. The dispatch is admitted with the static swap_hotkey_v2 weight, so a normal user can make a single-subnet hotkey swap perform work proportional to all locks in storage. Move the per-coldkey locks through indexed/bounded storage for the target (netuid, old_hotkey) or otherwise cap the work before exposing it from this dispatch.
|
🔄 AI review updated — Skeptic: VULNERABLE |
| .map(|lock| vec![(coldkey, lock)]) | ||
| .unwrap_or_default() | ||
| } else { | ||
| let locks: Vec<(T::AccountId, LockState)> = Lock::<T>::iter() |
There was a problem hiding this comment.
[HIGH] Runtime upgrade still scans the entire Lock map
For the coldkey: None fix, this migration iterates Lock::<T>::iter() and collects every matching entry by filtering on (netuid, old_hotkey). Lock is keyed by (coldkey, netuid, hotkey), so this scans the whole map during on_runtime_upgrade; the returned weight only accounts for locks_to_fix_count, not the total entries scanned. On mainnet this can make the upgrade block exceed its real work budget. Make every fix target explicit coldkeys, or add/use a bounded index that can be iterated by (netuid, hotkey) with weight charged for every scanned entry.
|
|
||
| // 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 can scan all locks under a fixed dispatch weight
This call enters swap_hotkey_locks_on_subnet, which still uses Lock::<T>::iter() whenever the old hotkey has lock aggregate state on the selected subnet. The extrinsic is declared with a fixed #[pallet::weight], so block admission does not know it may scan the entire lock map; returning the measured post-dispatch weight after execution does not bound the pre-dispatch resource use. A user can trigger an unbounded storage scan from a normal subnet-scoped hotkey swap. This needs a bounded lookup path, a hard cap with continuation, or a declared maximum weight that actually covers the worst-case scan.
|
🔄 AI review updated — Skeptic: VULNERABLE |
| .map(|lock| vec![(coldkey, lock)]) | ||
| .unwrap_or_default() | ||
| } else { | ||
| let locks: Vec<(T::AccountId, LockState)> = Lock::<T>::iter() |
There was a problem hiding this comment.
[HIGH] Runtime upgrade still scans the entire Lock map
This coldkey: None migration path still iterates Lock::<T>::iter() and collects every matching lock for one hard-coded hotkey. Because this runs during on_runtime_upgrade, the work is bounded by total Lock storage size rather than the number of known fixes, and the returned weight only charges locks_to_fix_count + 1 reads instead of every scanned entry. On a large mainnet Lock map, that can make the runtime upgrade exceed the block budget and stall the chain. Require an explicit coldkey for this fix or use a storage layout/prefix that bounds the scan to the affected keyspace and accounts every read.
|
|
||
| // 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 can scan all locks under a fixed dispatch weight
swap_hotkey_locks_on_subnet delegates into swap_hotkey_locks_for_netuids, which still calls Lock::<T>::iter() when the selected subnet has any relevant aggregate lock. The extrinsic’s declared pre-dispatch weight is still fixed, so an accepted normal transaction can perform work proportional to the entire Lock map before post-dispatch accounting reports the larger dynamic weight. An attacker only needs to choose an old hotkey with aggregate lock state on the target subnet to force a full-map scan. This needs a bounded per-hotkey/per-subnet index or another statically bounded transfer path before it is safe in a dispatchable call.
|
🔄 AI review updated — Skeptic: VULNERABLE |
3 participants
Description
Hotfix: Hotkey swap v2 did not move conviction. This PR fixes the issue on mainnet as a hotfix.
Related Issue(s)
Type of Change
Checklist
./scripts/fix_rust.shto ensure my code is formatted and linted correctly