harden: add optional owner validation to fetchSlab#192
harden: add optional owner validation to fetchSlab#1920x-SquidSol wants to merge 1 commit intodcccrypto:mainfrom
Conversation
fetchSlab returned raw account data without checking info.owner against the expected Percolator program ID. A malicious RPC could return crafted data from an arbitrary account, and the SDK would parse it as a valid slab (magic + detectSlabLayout checks pass for any account with correct magic and matching size). Added optional expectedOwner parameter. When provided, validates info.owner.equals(expectedOwner) before returning data. Backward compatible — existing callers without the param are unaffected. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/solana/slab.ts (1)
1602-1603: Consider adding a strict variant to avoid accidental non-validation at call sites.Because validation is opt-in, callers can still unintentionally skip owner checks. A small follow-up (e.g.,
fetchSlabStrict(connection, slabPubkey, expectedOwner)wrapper) would make safer usage easier for keeper/ADL paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/solana/slab.ts` around lines 1602 - 1603, Add a strict wrapper that enforces owner validation so callers can't accidentally skip it: create a new function (e.g., fetchSlabStrict) that wraps the existing fetchSlab (or the function taking slabPubkey and expectedOwner?) and requires expectedOwner to be provided, performs the owner check, and throws an error if the slab owner is missing or mismatches expectedOwner; update or document usages in keeper/ADL paths to call fetchSlabStrict instead of the optional-validate variant to guarantee validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/solana/slab.ts`:
- Around line 1602-1603: Add a strict wrapper that enforces owner validation so
callers can't accidentally skip it: create a new function (e.g.,
fetchSlabStrict) that wraps the existing fetchSlab (or the function taking
slabPubkey and expectedOwner?) and requires expectedOwner to be provided,
performs the owner check, and throws an error if the slab owner is missing or
mismatches expectedOwner; update or document usages in keeper/ADL paths to call
fetchSlabStrict instead of the optional-validate variant to guarantee
validation.
Summary
fetchSlabreturned raw account data without checkinginfo.owneragainst the expected Percolator program IDgetMarketsByAddressalready has owner validation (PR fix(discovery): validate account owner in getMarketsByAddress #128), butfetchSlab(used directly by keepers and ADL bots) did notexpectedOwner?: PublicKeyparameter. When provided, validatesinfo.owner.equals(expectedOwner)before returning. Fully backward compatible.Test plan
fetchSlab(conn, key, wrongOwner)throws with descriptive owner mismatch errorfetchSlab(conn, key)(no owner) still works as before🤖 Generated with Claude Code
Summary by CodeRabbit