feat: prevent cranking privileged instructions#1223
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds security validation to the crank instruction processing pipeline. It imports Assessment against linked issues
Suggested reviewers
✨ 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.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@magicblock-api/src/fund_account.rs`:
- Around line 48-50: Change init_execution_authority to return Result<(),
ApiError> and remove the panic-prone unwrap: replace the call to
accountsdb.get_account(execution_authority_id).unwrap() with error-aware code
that returns a structured ApiError when the account is missing or retrieval
fails (e.g., use Option/Result combinators or map_err/ok_or to produce
ApiError::StorageError or another appropriate ApiError variant). After setting
authority.set_delegated(true), propagate any error from
accountsdb.insert_account(execution_authority_id, &authority) by mapping its
failure into an ApiError instead of ignoring it; ensure callers of
init_execution_authority are updated to handle the Result. Reference symbols:
init_execution_authority, accountsdb.get_account, authority.set_delegated,
accountsdb.insert_account, and ApiError.
In `@magicblock-api/src/magic_validator.rs`:
- Around line 286-294: The code currently creates a fresh execution authority
each startup via Keypair::new() (execution_authority_keypair) and calls
init_execution_authority(&accountsdb, &execution_authority_keypair.pubkey()),
which rotates crank authority and breaks replay; instead persist and load the
execution keypair similar to how validator identity is handled by
init_validator_identity/validator_pubkey. Modify startup to attempt to load the
saved execution keypair from storage (or generate-and-save if missing), then
pass its pubkey to init_execution_authority; ensure the same persistent
storage/location and serialization format used for validator identity is reused
so the execution authority survives restarts.
In `@programs/magicblock/src/execution_authority.rs`:
- Around line 18-24: The current init_execution_authority and
set_execution_authority_override discard the Result from OnceLock::set, silently
ignoring double-initialization; change both functions (init_execution_authority
and set_execution_authority_override) to check the Result returned by
EXECUTION_AUTHORITY.set(...) and EXECUTION_AUTHORITY_OVERRIDE.set(...), and fail
fast on Err (e.g., panic or return a Result) with a clear message including the
attempted pubkey/keypair to surface misconfiguration rather than silently
proceeding. Ensure the error branch is reachable so re-init attempts do not get
ignored.
In `@programs/magicblock/src/utils/instruction_utils.rs`:
- Around line 268-269: The AccountMeta list was changed to require
execution_authority_id() as a signer but execute_task(...) still signs with
validator_authority(), causing missing signatures; update the execute_task
implementation to sign the transaction with the execution authority keypair
(replace usage of validator_authority()/validator signer with the execution
authority keypair or call that returns the execution authority signer) so the
signer matches AccountMeta::new_readonly(execution_authority_id(), true) and the
transaction includes the required execution-authority signature.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 61919831-6458-46e1-bb0b-cf6098663eac
📒 Files selected for processing (7)
magicblock-api/src/fund_account.rsmagicblock-api/src/magic_validator.rsmagicblock-task-scheduler/src/service.rsprograms/magicblock/src/execution_authority.rsprograms/magicblock/src/lib.rsprograms/magicblock/src/schedule_task/process_execute_task.rsprograms/magicblock/src/utils/instruction_utils.rs
ab3c87c to
a85f5d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@programs/magicblock/src/schedule_task/mod.rs`:
- Around line 48-73: The code currently attempts to bincode::deserialize every
instruction into MagicBlockInstruction (decoded_instruction) without verifying
the instruction is for the MagicBlock program; change this to first check
instruction.program_id matches the MagicBlock program id (e.g., the MagicBlock
program's public key constant or id() function) and only then call
bincode::deserialize::<MagicBlockInstruction>(&instruction.data); if the
program_id does not match, skip/continue instead of attempting to decode. Update
the logic around decoded_instruction in schedule_task/mod.rs so the program id
guard happens before deserialization and the privileged-instruction rejection
remains only for true MagicBlock instructions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 38faae46-3a48-4e4d-b1e4-1c1862ee790b
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
programs/magicblock/src/schedule_task/mod.rs
Closes #1222
Summary
Introduce a new signing authority for cranks
Breaking Changes
Summary by CodeRabbit