Repatriate residual stake and locks on lease termination#2709
Conversation
l0r1s
requested review from
JohnReedV,
evgeny-s,
gztensor,
open-junius and
sam0x17
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
🛡️ AI Review — Skeptic (security review)VERDICT: VULNERABLE LOW scrutiny: l0r1s has repo write permission, a 2018 account, substantial contribution history, matching commit authors, no Gittensor allowlist hit found, and branch is fix-lease-handoff -> devnet-ready. No Findings
Prior-comment reconciliation
ConclusionThis head still has non-atomic fallible runtime handoffs, and the expanded lease termination path is not reflected in generated weights. Failed crowdloan finalization can persist finalized/current-id state, lease termination can commit partial handoff state, and undercharging the new worst-case termination work can create an overweight block path. 📜 Previous run (superseded)
# 🔍 AI Review — Auditor (domain review) has not yet run on this PR. |
|
🔄 AI review updated — Skeptic: VULNERABLE |
|
🔄 AI review updated — Skeptic: VULNERABLE |
|
🔄 AI review updated — Skeptic: VULNERABLE |
|
🔄 AI review updated — Skeptic: VULNERABLE |
l0r1s
added
the
skip-cargo-audit
|
🔄 AI review updated — Skeptic: VULNERABLE |
|
🔄 AI review updated — Skeptic: VULNERABLE |
| // Mark finalized *before* dispatching the creator-controlled call so re-entrant | ||
| // withdraw/refund/dissolve are rejected mid-dispatch. Reverts if the dispatch fails. | ||
| crowdloan.finalized = true; | ||
| Crowdloans::<T>::insert(crowdloan_id, &crowdloan); |
There was a problem hiding this comment.
[HIGH] Failed finalize can persist finalized state and trap funds
This writes crowdloan.finalized = true before several fallible operations: preimage lookup/drop, dispatch of the creator-controlled call, target transfer, and invalid-config return. I do not see #[transactional] on finalize or a production with_storage_layer around the whole body, so a later Err can leave the crowdloan marked finalized while funds were not delivered and contributors can no longer refund/withdraw. The new test only wraps one path in with_storage_layer; the dispatchable itself needs the rollback boundary, or the state write must happen only after all fallible finalization work succeeds while still blocking reentrancy by another mechanism.
| SubnetOwner::<T>::insert(lease.netuid, lease.beneficiary.clone()); | ||
| // Set the owner hotkey before moving locks so the destination lock is | ||
| // accounted in the subnet-owner aggregate, not the regular hotkey aggregate. | ||
| Self::set_subnet_owner_hotkey(lease.netuid, &hotkey)?; | ||
| Self::reassign_subnet_owner_lock_aggregates(lease.netuid, &lease.hotkey, &hotkey); | ||
|
|
||
| Self::repatriate_lease_coldkey_alpha(&lease, &hotkey)?; | ||
| Self::transfer_full_lock_to_coldkey( |
There was a problem hiding this comment.
[MEDIUM] Lease termination can still commit a partial handoff on lock conflict
The production terminate_lease dispatchable still mutates owner state and aggregate lock buckets before later fallible calls such as repatriate_lease_coldkey_alpha, transfer_full_lock_to_coldkey, perform_hotkey_swap_on_one_subnet, proxy removal, and the balance sweep. If any later call rejects, for example transfer_full_lock_to_coldkey returning LockHotkeyMismatch, there is no visible #[transactional] or production with_storage_layer on this path, so the lease can remain open with subnet ownership and lock aggregates already changed. Wrap the whole handoff in a storage transaction or preflight every fallible condition before the first state mutation.
|
🔄 AI review updated — Skeptic: VULNERABLE |
| // Mark finalized *before* dispatching the creator-controlled call so re-entrant | ||
| // withdraw/refund/dissolve are rejected mid-dispatch. Reverts if the dispatch fails. | ||
| crowdloan.finalized = true; | ||
| Crowdloans::<T>::insert(crowdloan_id, &crowdloan); |
There was a problem hiding this comment.
[HIGH] Failed finalize can persist finalized state and trap funds
finalize writes finalized = true before fallible preimage lookup / creator-call dispatch, but the production dispatchable still has no visible #[transactional], with_transaction, or with_storage_layer boundary. In FRAME, returning Err after storage writes is not enough to undo them unless a transaction layer is active. A missing preimage or failing creator-controlled call can therefore leave the crowdloan marked finalized; in the call path it can also leave CurrentCrowdloanId set because kill() is after the fallible dispatch. The new test manually wraps one case in with_storage_layer, but that does not add the production rollback boundary. Wrap the whole finalization mutation/dispatch section in a transaction layer or make the dispatchable transactional before moving this guard write ahead of the fallible work.
| #[pallet::weight( | ||
| <T as crate::pallet::Config>::WeightInfo::terminate_lease(T::MaxContributors::get()) | ||
| .saturating_add(<T as crate::pallet::Config>::WeightInfo::swap_hotkey()) | ||
| )] |
There was a problem hiding this comment.
[HIGH] Lease termination weight does not cover the new worst-case work
The dispatch weight is still just generated terminate_lease(k) plus swap_hotkey(), but the PR adds more worst-case work to do_terminate_lease: lock migration, owner-lock aggregate reassignment, stake repatriation, full-lock transfer, proxy removal, currency sweep, and extra storage cleanup. The benchmark scenario was updated, but pallets/subtensor/src/weights.rs is not changed in this PR, so the runtime still charges the old generated weight. Undercharging a runtime extrinsic can let a valid lease termination exceed its declared block weight. Regenerate and commit the weights for the expanded benchmark, or add a conservative explicit weight component that accounts for the added reads/writes and balance/proxy operations.
| SubnetOwner::<T>::insert(lease.netuid, lease.beneficiary.clone()); | ||
| // Set the owner hotkey before moving locks so the destination lock is | ||
| // accounted in the subnet-owner aggregate, not the regular hotkey aggregate. | ||
| Self::set_subnet_owner_hotkey(lease.netuid, &hotkey)?; | ||
| Self::reassign_subnet_owner_lock_aggregates(lease.netuid, &lease.hotkey, &hotkey); | ||
|
|
||
| Self::repatriate_lease_coldkey_alpha(&lease, &hotkey)?; | ||
| Self::transfer_full_lock_to_coldkey( |
There was a problem hiding this comment.
[MEDIUM] Lease termination can still commit a partial handoff on lock conflict
This sequence mutates subnet ownership and owner-lock aggregates before the later transfer_full_lock_to_coldkey / hotkey swap / proxy-removal calls can return Err. I do not see a production #[transactional], with_transaction, or with_storage_layer wrapper on terminate_lease or do_terminate_lease, so an error after these writes can leave the subnet partially handed off while the lease remains present. The new rollback test asserts the desired behavior, but the production call path still needs an explicit transaction boundary around the mutation sequence or all fallible conflict checks must happen before any state writes.
|
🔄 AI review updated — Skeptic: VULNERABLE |
4 participants
Repatriate residual stake and locks on lease termination
Closes #2663.
Problem
do_terminate_leasehanded subnet ownership to the beneficiary but left theaccumulated owner-cut alpha and lock state on the lease's derived coldkey
(
blake2_256("leasing/coldkey", lease_id)). That coldkey has no private key andthe beneficiary proxy can't extract stake, so every fixed-end lease termination
permanently stranded the owner emissions accrued during the lease.
Change
Before tearing down the lease coldkey, termination now:
conviction earned during the lease (
do_move_lock(..., preserve_conviction));(
reassign_subnet_owner_lock_aggregates, extracted so it's shared with theexisting owner-change path);
transfer_full_lock_to_coldkey);SubnetUidToLeaseId;dec_providers, then sweeps theunreserved proxy deposit to the beneficiary, so the keyless coldkey is reaped
cleanly with no dangling account, refcount, or stranded funds.
Post-condition: no alpha, lock, or ownership rows remain under the lease coldkey.