fix: add validate_account_owner to Withdraw and admin config handlers - #106
Conversation
process_withdraw, process_update_config, process_admin_set_hwm_config, and process_admin_set_tranche_config all read/write pool_pda without verifying it is owned by the stake program. All four had _program_id (unused). While vault_auth PDA derivation (Withdraw) and admin field checks (config handlers) provide partial protection, explicit ownership validation is required for defense-in-depth — consistent with process_deposit which validates at line 390. Also added validate_account_writable for process_withdraw since it mutates pool state. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 17 minutes and 20 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Sentinel security review — MERGE Fix is correct. Current master has This is a defense-in-depth gap consistent with what PR #100 and #101 fix in their respective handlers. The Solana runtime prevents cross-program data writes, but same-program type confusion (a StakeDeposit PDA passed as pool_pda) remains a vector for state corruption. No conflicts with other open PRs — these handlers are not touched by #113 (which handles accrue_fees only). Severity confirmed: MEDIUM (CVSS ~5.9 — defense-in-depth gap, partial protection exists via PDA derivation and field checks). Security APPROVED PR #106 — findings: none | ownership validation on Withdraw and admin config handlers, consistent with process_deposit pattern |
Summary
process_withdraw,process_update_config,process_admin_set_hwm_config, andprocess_admin_set_tranche_configall read/writepool_pdawithout verifying it is owned by the stake program_program_id(underscore = explicitly unused), confirming the oversightSeverity
MEDIUM — defense-in-depth gap; inconsistent with
process_deposit(line 390) which validates ownershipFix
_program_id→program_idin all four handlersvalidate_account_owner(pool_pda, program_id)?;before data borrowvalidate_account_writable(pool_pda)?;inprocess_withdraw(mutates state)Test plan
cargo build— compiles cleanlycargo clippy— zero warnings🤖 Generated with Claude Code