Skip to content
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 21 additions & 8 deletions pallets/subtensor/src/macros/dispatches.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ mod dispatches {
use crate::MAX_CRV3_COMMIT_SIZE_BYTES;
use crate::MAX_NUM_ROOT_CLAIMS;
use crate::MAX_ROOT_CLAIM_THRESHOLD;
use crate::MAX_SUBNET_CLAIMS;

/// Dispatchable functions allow users to interact with the pallet and invoke state changes.
/// These functions materialize as "extrinsics", which are often compared to transactions.
Expand Down Expand Up @@ -2169,15 +2168,29 @@ mod dispatches {
) -> DispatchResultWithPostInfo {
let coldkey: T::AccountId = ensure_signed(origin)?;

ensure!(!subnets.is_empty(), Error::<T>::InvalidSubnetNumber);
ensure!(
subnets.len() <= MAX_SUBNET_CLAIMS,
Error::<T>::InvalidSubnetNumber
);
let weight = Self::do_root_claim_checked(coldkey, subnets)?;
Ok((Some(weight), Pays::Yes).into())
}

Self::maybe_add_coldkey_index(&coldkey);
/// --- Claims the root emissions on behalf of any coldkey.
/// # Args:
/// * 'origin': any signed account; only authorizes (and pays for) the call.
/// * 'coldkey': the coldkey whose claims are settled; all realized value is credited here.
/// * 'subnets': the subnets to claim on (1..=MAX_SUBNET_CLAIMS).
///
/// # Event:
/// * RootClaimed;
/// - On successfully claiming the root emissions for `coldkey`.
#[pallet::call_index(142)]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] claim_root_for reuses an existing runtime call index

142 is already assigned to set_reject_locked_alpha later in this pallet. Runtime calls are SCALE-encoded by call index, so a duplicate index makes dispatch bytes ambiguous across the runtime upgrade and can cause one call to decode as the other or make an existing call inaccessible. Assign claim_root_for an unused index, e.g. 143, and update any metadata/tests accordingly.

Suggested change
#[pallet::call_index(142)]
#[pallet::call_index(143)]

#[pallet::weight(<T as crate::pallet::Config>::WeightInfo::claim_root())]
pub fn claim_root_for(
origin: OriginFor<T>,
coldkey: T::AccountId,
subnets: BTreeSet<NetUid>,
) -> DispatchResultWithPostInfo {
let _who: T::AccountId = ensure_signed(origin)?;

let weight = Self::do_root_claim(coldkey, Some(subnets))?;
let weight = Self::do_root_claim_checked(coldkey, subnets)?;
Comment thread
evgeny-s marked this conversation as resolved.
Ok((Some(weight), Pays::Yes).into())
}

Expand Down
15 changes: 15 additions & 0 deletions pallets/subtensor/src/staking/claim_root.rs
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,21 @@ impl<T: Config> Pallet<T> {
}
}

pub fn do_root_claim_checked(
coldkey: T::AccountId,
subnets: BTreeSet<NetUid>,
) -> Result<Weight, DispatchError> {
ensure!(!subnets.is_empty(), Error::<T>::InvalidSubnetNumber);
ensure!(
subnets.len() <= crate::MAX_SUBNET_CLAIMS,
Error::<T>::InvalidSubnetNumber
);

Self::maybe_add_coldkey_index(&coldkey);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] Arbitrary coldkeys can pollute the root auto-claim index

claim_root_for accepts any signed origin and passes an arbitrary coldkey into this shared helper. The helper indexes that coldkey before do_root_claim proves the coldkey has any root stake; for a coldkey with no staking hotkeys, try_do_root_claim simply iterates an empty vector, deposits RootClaimed, and commits. A caller can therefore pay only transaction fees to persistently grow StakingColdkeys, StakingColdkeysByIndex, and NumStakingColdkeys with arbitrary accounts, diluting run_auto_claim_root_divs selection and creating unbounded state bloat. Gate the index insertion on actual root stake, or make claim_root_for reject targets without root stake before calling maybe_add_coldkey_index.

Suggested change
Self::maybe_add_coldkey_index(&coldkey);
ensure!(
Self::coldkey_has_root_stake(&coldkey),
Error::<T>::NotEnoughStakeToWithdraw
);
Self::maybe_add_coldkey_index(&coldkey);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] Arbitrary coldkeys can pollute the root auto-claim index

claim_root_for lets any signed account choose coldkey, and this helper adds that target to StakingColdkeys before proving the target has any root stake. Because try_do_root_claim succeeds even for a coldkey with no staking hotkeys, an attacker can pay fees to persistently add arbitrary coldkeys to the auto-claim sampling set, inflating NumStakingColdkeys and reducing the probability that legitimate root stakers are selected each block. Gate this index insertion on the target currently holding root stake, or only add coldkeys at the root-stake creation paths.


Self::do_root_claim(coldkey, Some(subnets))
}

pub fn do_root_claim(
coldkey: T::AccountId,
subnets: Option<BTreeSet<NetUid>>,
Expand Down
63 changes: 63 additions & 0 deletions pallets/subtensor/src/tests/claim_root.rs
Original file line number Diff line number Diff line change
Expand Up @@ -588,6 +588,69 @@ fn test_claim_root_with_changed_stake() {
});
}

#[test]
fn test_claim_root_for_credits_target_not_caller() {
new_test_ext(1).execute_with(|| {
let owner_coldkey = U256::from(1001);
let hotkey = U256::from(1002);
let contract_coldkey = U256::from(1003);
let keeper = U256::from(2099);
let netuid = add_dynamic_network(&hotkey, &owner_coldkey);
remove_owner_registration_stake(netuid);

SubtensorModule::set_tao_weight(u64::MAX);

let root_stake = 2_000_000u64;
mock_increase_stake_for_hotkey_and_coldkey_on_subnet(
&hotkey,
&contract_coldkey,
NetUid::ROOT,
root_stake.into(),
);
mock_increase_stake_for_hotkey_and_coldkey_on_subnet(
&hotkey,
&owner_coldkey,
netuid,
10_000_000u64.into(),
);

SubtensorModule::distribute_emission(
netuid,
AlphaBalance::ZERO,
AlphaBalance::ZERO,
1_000_000u64.into(),
AlphaBalance::ZERO,
);
assert!(
RootClaimable::<Test>::get(hotkey).contains_key(&netuid),
"claimable must have accrued"
);

let root_of = |coldkey: &U256| -> u64 {
SubtensorModule::get_stake_for_hotkey_and_coldkey_on_subnet(
&hotkey,
coldkey,
NetUid::ROOT,
)
.into()
};
let contract_root_before = root_of(&contract_coldkey);
assert_eq!(contract_root_before, root_stake);
assert_eq!(root_of(&keeper), 0);

assert_ok!(SubtensorModule::claim_root_for(
RuntimeOrigin::signed(keeper),
contract_coldkey,
BTreeSet::from([netuid])
));

assert!(root_of(&contract_coldkey) > contract_root_before);
assert_eq!(root_of(&keeper), 0);
assert!(RootClaimed::<Test>::get((netuid, &hotkey, &contract_coldkey)) > 0);
assert_eq!(RootClaimed::<Test>::get((netuid, &hotkey, &keeper)), 0);
});
}

#[test]
fn test_claim_root_with_drain_emissions_and_swap_claim_type() {
new_test_ext(1).execute_with(|| {
Expand Down
Loading