Precompile advanced order#2685
Conversation
… 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, plus a fee-evasion path in direct buy-order execution. Findings
Prior-comment reconciliation
ConclusionThe PR appears legitimate, but the new limit-order execution paths can still leave assets moved after later failures, and direct buy execution can mark orders filled without collecting the signed fee. These are security/economic vulnerabilities and should block merge. 📜 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 |
| let pallet_hotkey = T::PalletHotkey::get(); | ||
|
|
||
| // Pull all input assets into the pallet intermediary before touching the pool. | ||
| Self::collect_assets( |
There was a problem hiding this comment.
[HIGH] Batched execution is not atomic after collecting user assets
execute_batched_orders still starts by moving every buyer's TAO and seller's staked alpha into the pallet intermediary, then performs the net pool swap, pro-rata distributions, fee forwarding, order-status writes, and event emission as separate fallible steps. There is no outer #[transactional] / with_transaction boundary around the whole batch. If any later transfer or swap fails after this line, earlier collect_assets transfers are not rolled back, so users can lose assets while the extrinsic returns an error and some orders remain unfilled. Wrap the full do_execute_batched_orders asset-collection/swap/distribution/status-write flow in a transaction, or prevalidate every fallible post-collection transfer so no failure is possible after user assets are pulled.
| 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 { |
There was a problem hiding this comment.
[HIGH] Stake transfer mutates balances before receiver validation
transfer_staked_alpha decreases the sender's stake, increases the receiver's stake, and writes LastColdkeyHotkeyStakeBlock before checking validate_receiver. The batch distributor calls this with validate_receiver = true for buyers, so a bad buyer hotkey can make the function return HotKeyAccountNotExists after stake has already been moved out of the pallet intermediary. Because this helper is not transactional and the outer batch is not transactional, that failure leaves mutated stake/accounting behind. Validate to_hotkey before lines 138-151, or make this helper transactional so receiver-validation failures roll back the mutations.
| let (amount_in, amount_out) = if order.order_type.is_buy() { | ||
| // partial fill validations have passed, it is safe here to do this | ||
| let tao_in = TaoBalance::from(signed_order.partial_fill.unwrap_or(order.amount)); | ||
| // Deduct fee from TAO input before swapping. | ||
| let fee_tao = TaoBalance::from(order.fee_rate.mul_floor(tao_in.to_u64())); | ||
| let tao_after_fee = tao_in.saturating_sub(fee_tao); | ||
|
|
||
| let alpha_out = T::SwapInterface::buy_alpha( |
There was a problem hiding this comment.
[MEDIUM] Direct buy execution can fill an order without collecting its fee
For buy orders, the pallet only passes tao_after_fee into buy_alpha, so the subtensor-side balance validation only proves the signer can pay the net swap amount. The fee is then forwarded best-effort from the signer after the swap. A signer with exactly amount - fee free TAO can have the order execute and be marked fulfilled, while forward_fee fails and leaves the fee unpaid. This makes the signed fee field unenforceable in the direct execute_orders path. Require/fund the gross buy amount atomically, or make buy fee transfer a hard precondition before marking the order executed.
|
🔄 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.