Precompile advanced order#2685
Conversation
…e-advanced-orders
…e-advanced-orders
… alpha! that is not needed
…e-advanced-orders
…e-advanced-orders
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /// subnet are skipped. | ||
| #[pallet::call_index(1)] | ||
| #[pallet::weight(T::WeightInfo::execute_batched_orders(orders.len() as u32))] | ||
| pub fn execute_batched_orders( |
There was a problem hiding this comment.
[HIGH] Batched execution is not atomic after collecting user assets
execute_batched_orders pulls TAO/staked alpha from every order signer into the pallet account via collect_assets, then makes later fallible calls (net_pool_swap, distribution transfers, zero-output checks). Without a transaction boundary, an error after collect_assets can return Err while leaving balances/stake already moved into or through the pallet account and orders not marked fulfilled. A public relayer can trigger this with valid signed orders plus a later slippage/zero-output/distribution failure, locking or misrouting user funds. Wrap the whole batched dispatch in a FRAME transaction so any later error rolls back the asset collection and intermediate transfers.
| pub fn execute_batched_orders( | |
| #[frame_support::transactional] | |
| pub fn execute_batched_orders( |
| let available = | ||
| Self::get_stake_for_hotkey_and_coldkey_on_subnet(from_hotkey, from_coldkey, netuid); | ||
| ensure!(available >= amount, Error::<T>::NotEnoughStakeToWithdraw); | ||
| Self::decrease_stake_for_hotkey_and_coldkey_on_subnet( | ||
| from_hotkey, | ||
| from_coldkey, | ||
| netuid, | ||
| amount, | ||
| ); | ||
| Self::increase_stake_for_hotkey_and_coldkey_on_subnet( | ||
| to_hotkey, to_coldkey, netuid, amount, | ||
| ); | ||
| LastColdkeyHotkeyStakeBlock::<T>::insert( | ||
| to_coldkey, | ||
| to_hotkey, | ||
| Self::get_current_block_as_u64(), | ||
| ); | ||
| if validate_receiver { | ||
| ensure!( | ||
| Self::hotkey_account_exists(to_hotkey), | ||
| Error::<T>::HotKeyAccountNotExists | ||
| ); | ||
| Self::set_stake_operation_limit(to_hotkey, to_coldkey, netuid); | ||
| } |
There was a problem hiding this comment.
[HIGH] Stake transfer mutates balances before receiver validation
transfer_staked_alpha decreases the sender stake, increases receiver stake, and writes the rate-limit block before checking hotkey_account_exists(to_hotkey) when validate_receiver is true. In the new batched limit-order distribution path, a buy order with an unregistered destination hotkey can therefore make this helper return Err after stake has already been moved. Validate the receiver before any stake mutation, then set the receiver rate limit after the successful move.
| let available = | |
| Self::get_stake_for_hotkey_and_coldkey_on_subnet(from_hotkey, from_coldkey, netuid); | |
| ensure!(available >= amount, Error::<T>::NotEnoughStakeToWithdraw); | |
| Self::decrease_stake_for_hotkey_and_coldkey_on_subnet( | |
| from_hotkey, | |
| from_coldkey, | |
| netuid, | |
| amount, | |
| ); | |
| Self::increase_stake_for_hotkey_and_coldkey_on_subnet( | |
| to_hotkey, to_coldkey, netuid, amount, | |
| ); | |
| LastColdkeyHotkeyStakeBlock::<T>::insert( | |
| to_coldkey, | |
| to_hotkey, | |
| Self::get_current_block_as_u64(), | |
| ); | |
| if validate_receiver { | |
| ensure!( | |
| Self::hotkey_account_exists(to_hotkey), | |
| Error::<T>::HotKeyAccountNotExists | |
| ); | |
| Self::set_stake_operation_limit(to_hotkey, to_coldkey, netuid); | |
| } | |
| if validate_receiver { | |
| ensure!( | |
| Self::hotkey_account_exists(to_hotkey), | |
| Error::<T>::HotKeyAccountNotExists | |
| ); | |
| } | |
| let available = | |
| Self::get_stake_for_hotkey_and_coldkey_on_subnet(from_hotkey, from_coldkey, netuid); | |
| ensure!(available >= amount, Error::<T>::NotEnoughStakeToWithdraw); | |
| Self::decrease_stake_for_hotkey_and_coldkey_on_subnet( | |
| from_hotkey, | |
| from_coldkey, | |
| netuid, | |
| amount, | |
| ); | |
| Self::increase_stake_for_hotkey_and_coldkey_on_subnet( | |
| to_hotkey, to_coldkey, netuid, amount, | |
| ); | |
| LastColdkeyHotkeyStakeBlock::<T>::insert( | |
| to_coldkey, | |
| to_hotkey, | |
| Self::get_current_block_as_u64(), | |
| ); | |
| if validate_receiver { | |
| Self::set_stake_operation_limit(to_hotkey, to_coldkey, netuid); | |
| } |
🛡️ AI Review — Skeptic (security review)VERDICT: VULNERABLE MEDIUM scrutiny: open-junius has repo write permission and substantial prior activity; no Gittensor allowlist hit; branch is precompile-advanced-order -> devnet-ready. No trusted review prompt or copilot-instruction files are modified in this PR. Static review found the prior fund-safety issues still present in the new limit-orders batch path; no malicious backdoor pattern was identified. Findings
Prior-comment reconciliation
ConclusionThe PR appears legitimate, but batched order execution can still leave user assets moved after a later failure. That is a security vulnerability and should block merge until the batch path is atomic and stake-transfer receiver validation happens before mutation. 📜 Previous run (superseded)
# 🔍 AI Review — Auditor (domain review) has not yet run on this PR. |
|
🔄 AI review updated — Skeptic: VULNERABLE |
| #[pallet::call_index(1)] | ||
| #[pallet::weight(T::WeightInfo::execute_batched_orders(orders.len() as u32))] | ||
| pub fn execute_batched_orders( |
There was a problem hiding this comment.
[HIGH] Batched execution is not atomic after collecting user assets
execute_batched_orders can transfer TAO/staked alpha into the pallet account in collect_assets, then return Err from net_pool_swap, distribute_alpha_pro_rata, or distribute_tao_pro_rata. Without a transaction boundary on the extrinsic, those earlier balance/stake mutations are not rolled back, so a failing batch can strand or move user assets while the order remains unfulfilled. Wrap the whole batched extrinsic in #[frame_support::transactional] or otherwise make every post-collection failure non-fatal and fully accounted.
| #[pallet::call_index(1)] | |
| #[pallet::weight(T::WeightInfo::execute_batched_orders(orders.len() as u32))] | |
| pub fn execute_batched_orders( | |
| #[pallet::call_index(1)] | |
| #[pallet::weight(T::WeightInfo::execute_batched_orders(orders.len() as u32))] | |
| #[frame_support::transactional] | |
| pub fn execute_batched_orders( |
| let available = | ||
| Self::get_stake_for_hotkey_and_coldkey_on_subnet(from_hotkey, from_coldkey, netuid); | ||
| ensure!(available >= amount, Error::<T>::NotEnoughStakeToWithdraw); | ||
| Self::decrease_stake_for_hotkey_and_coldkey_on_subnet( | ||
| from_hotkey, | ||
| from_coldkey, | ||
| netuid, | ||
| amount, | ||
| ); | ||
| Self::increase_stake_for_hotkey_and_coldkey_on_subnet( | ||
| to_hotkey, to_coldkey, netuid, amount, | ||
| ); | ||
| LastColdkeyHotkeyStakeBlock::<T>::insert( | ||
| to_coldkey, | ||
| to_hotkey, | ||
| Self::get_current_block_as_u64(), | ||
| ); | ||
| if validate_receiver { | ||
| ensure!( | ||
| Self::hotkey_account_exists(to_hotkey), | ||
| Error::<T>::HotKeyAccountNotExists | ||
| ); | ||
| Self::set_stake_operation_limit(to_hotkey, to_coldkey, netuid); | ||
| } |
There was a problem hiding this comment.
[HIGH] Stake transfer mutates balances before receiver validation
When validate_receiver is true, this function decreases the sender stake, increases the receiver stake, and writes LastColdkeyHotkeyStakeBlock before checking hotkey_account_exists(to_hotkey). If that receiver check fails, the function returns an error after mutating stake accounting. In the current batched-order path this compounds the missing atomicity issue; the receiver-side validation needs to happen before any stake movement.
| let available = | |
| Self::get_stake_for_hotkey_and_coldkey_on_subnet(from_hotkey, from_coldkey, netuid); | |
| ensure!(available >= amount, Error::<T>::NotEnoughStakeToWithdraw); | |
| Self::decrease_stake_for_hotkey_and_coldkey_on_subnet( | |
| from_hotkey, | |
| from_coldkey, | |
| netuid, | |
| amount, | |
| ); | |
| Self::increase_stake_for_hotkey_and_coldkey_on_subnet( | |
| to_hotkey, to_coldkey, netuid, amount, | |
| ); | |
| LastColdkeyHotkeyStakeBlock::<T>::insert( | |
| to_coldkey, | |
| to_hotkey, | |
| Self::get_current_block_as_u64(), | |
| ); | |
| if validate_receiver { | |
| ensure!( | |
| Self::hotkey_account_exists(to_hotkey), | |
| Error::<T>::HotKeyAccountNotExists | |
| ); | |
| Self::set_stake_operation_limit(to_hotkey, to_coldkey, netuid); | |
| } | |
| if validate_receiver { | |
| ensure!( | |
| Self::hotkey_account_exists(to_hotkey), | |
| Error::<T>::HotKeyAccountNotExists | |
| ); | |
| } | |
| let available = | |
| Self::get_stake_for_hotkey_and_coldkey_on_subnet(from_hotkey, from_coldkey, netuid); | |
| ensure!(available >= amount, Error::<T>::NotEnoughStakeToWithdraw); | |
| Self::decrease_stake_for_hotkey_and_coldkey_on_subnet( | |
| from_hotkey, | |
| from_coldkey, | |
| netuid, | |
| amount, | |
| ); | |
| Self::increase_stake_for_hotkey_and_coldkey_on_subnet( | |
| to_hotkey, to_coldkey, netuid, amount, | |
| ); | |
| LastColdkeyHotkeyStakeBlock::<T>::insert( | |
| to_coldkey, | |
| to_hotkey, | |
| Self::get_current_block_as_u64(), | |
| ); | |
| if validate_receiver { | |
| Self::set_stake_operation_limit(to_hotkey, to_coldkey, netuid); | |
| } |
|
🔄 AI review updated — Skeptic: VULNERABLE |
3 participants
Description
Related Issue(s)
Type of Change
Breaking Change
If this PR introduces a breaking change, please provide a detailed description of the impact and the migration path for existing applications.
Checklist
./scripts/fix_rust.shto ensure my code is formatted and linted correctlyScreenshots (if applicable)
Please include any relevant screenshots or GIFs that demonstrate the changes made.
Additional Notes
Please provide any additional information or context that may be helpful for reviewers.