Added root_claim_for#2784
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
🛡️ AI Review — Skeptic (security review)VERDICT: SAFE BASELINE scrutiny: evgeny-s has repo write permission, an account from 2012, substantial contribution history, and no matched Gittensor allowlist signal; branch feat/root_claim_for -> devnet-ready. No FindingsNo findings. Prior-comment reconciliation
ConclusionSAFE because the prior arbitrary-target root-claim path and duplicate runtime call-index issue are no longer present, and I found no malicious code or new security vulnerability in the remaining self-claim precompile wrapper. 📜 Previous run (superseded)
🔍 AI Review — Auditor (domain review)VERDICT: 👍 Gittensor: UNKNOWN direct allowlist match; author has repo write permission and substantial prior PR history, so reviewed as an established contributor. The Auditor proposed a replacement PR description, but the current body is non-trivial; not overwriting. Maintainers: ask the Auditor to regenerate if you want it. Implementation adds Description discrepancy: the current one-sentence PR body says this claims on behalf of another coldkey, but the implementation and Solidity docs only support self-claim for the EVM caller's derived coldkey. I populated Spec-version auto-fix was not applied: this sandbox could not resolve FindingsNo findings. ConclusionNo blocking domain issues found in the diff. The remaining risk is the normal devnet spec-version CI check, which I could not confirm from this network-restricted environment. |
|
🔄 AI review updated — Skeptic: VULNERABLE |
evgeny-s
added
the
skip-cargo-audit
evgeny-s
changed the title
| Error::<T>::InvalidSubnetNumber | ||
| ); | ||
|
|
||
| Self::maybe_add_coldkey_index(&coldkey); |
There was a problem hiding this comment.
[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.
| Self::maybe_add_coldkey_index(&coldkey); | |
| ensure!( | |
| Self::coldkey_has_root_stake(&coldkey), | |
| Error::<T>::NotEnoughStakeToWithdraw | |
| ); | |
| Self::maybe_add_coldkey_index(&coldkey); |
|
🔄 AI review updated — Skeptic: VULNERABLE |
|
Concern: claim_root_for is permissionless and, combined with PR #2759, lets anyone force-sell other users' rewards Under that model, claim_root_for lets any signed account trigger a claim on behalf of any coldkey, with no authorization check. A malicious actor could call claim_root_for on every coldkey staking on a given validator. Since claiming now forces a swap of the basket alpha into TAO, this would create sell pressure on exactly the subnets that validator chose to weight. A malicious actor can force the timing of this sell pressure. Suggestion: rather than introducing a standalone permissionless claim_root_for, integrate claim_root into the existing staking proxy mechanism. If a coldkey has set a staking proxy, that's an explicit signal of trust that the proxy may claim its rewards on its behalf. This keeps the "claim on behalf of" capability gated behind consent the coldkey has already granted, instead of making it open to anyone. |
Thanks for sharing the concern, but the thing is that I need to provide a possibility to claim rewards for the coldkey, which doesn't have a private key, like a derivative key for an ,EVM smart contract let's say. This way, there is no possibility of adding a proxy, because it doesn't have a private key. |
| /// # Event: | ||
| /// * RootClaimed; | ||
| /// - On successfully claiming the root emissions for `coldkey`. | ||
| #[pallet::call_index(142)] |
There was a problem hiding this comment.
[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.
| #[pallet::call_index(142)] | |
| #[pallet::call_index(143)] |
| Error::<T>::InvalidSubnetNumber | ||
| ); | ||
|
|
||
| Self::maybe_add_coldkey_index(&coldkey); |
There was a problem hiding this comment.
[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.
|
🔄 AI review updated — Skeptic: VULNERABLE |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
2 participants
Description
This PR adds a new
claim_root_forfunction to be able to call root claim on behalf of another coldkey.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.