feat: add transfer queue with crank for processing#18
feat: add transfer queue with crank for processing#18GabrielePicco wants to merge 7 commits intofeat/shuttle-deposit-with-actionfrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2027b6a492
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| vault.mint = *mint_info.address(); | ||
| vault.token_account = *vault_token_acc_info.address(); |
There was a problem hiding this comment.
When vault_info is already on the current layout, this instruction still rewrites vault.mint and vault.token_account from caller-supplied accounts. Because that path no longer exits early, any signer can repoint an already-initialized vault to a different mint/ATA pair, which can break deposits/withdrawals for the original mint and corrupt vault state until someone manually repairs it.
Useful? React with 👍 / 👎.
| let data = unsafe { queue_info.borrow_unchecked_mut() }; | ||
| queue_set_crank_task_id_from_data(data, crank_task_id)?; |
There was a problem hiding this comment.
This persists crank_task_id after the CPI without first asserting that magic_program_info is the expected Magic program ID. A malicious executable account can return success for the schedule CPI, causing the queue to store a nonzero crank id even though no real recurring task was created; subsequent EnsureTransferQueueCrank calls then short-circuit on the stored id and skip legitimate scheduling.
Useful? React with 👍 / 👎.
| let data = unsafe { queue_info.borrow_unchecked_mut() }; | ||
| let popped_transfer = queue_pop_from_data(data)?.ok_or(ProgramError::InvalidAccountData)?; | ||
| if popped_transfer.task_id != queued_transfer.task_id { |
There was a problem hiding this comment.
After invoking the user-provided magic_program_info, the code immediately pops the queued transfer. Because there is no program-id check for magic_program_info, an attacker can call this tick with a custom program that returns success without executing the standalone token transfer, which drops ready queue items and leaves funds stranded in the vault.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3dbe6c4cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| [workspace.dependencies] | ||
| ephemeral-rollups-pinocchio = { version = "0.8.6" } | ||
| ephemeral-rollups-pinocchio = { path = "/Users/gabrielepicco/Documents/Solana/ephemeral-rollups-sdk/rust/pinocchio" } |
There was a problem hiding this comment.
The workspace dependency now points to an absolute local path (/Users/gabrielepicco/...), which makes builds fail everywhere except the author’s machine (CI and other developers cannot resolve that manifest). This is release-blocking for consumers of this commit because cargo cannot even load the workspace without that exact filesystem layout.
Useful? React with 👍 / 👎.
| to: destination_token_acc_info, | ||
| authority: vault_info, | ||
| token_program: token_program_info.address(), | ||
| amount: args.amount(), |
There was a problem hiding this comment.
This internal handler transfers args.amount directly from the vault but never checks a queue account/task id to prove the transfer was actually dequeued, so authorization is effectively reduced to possessing a valid escrow PDA signature. In the Magic standalone-action context used by this feature, invoking this discriminator directly can move arbitrary vault funds to a chosen destination without a corresponding queued transfer.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40d8fd9f90
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !payer_info.is_signer() { | ||
| return Err(ProgramError::MissingRequiredSignature); | ||
| } | ||
|
|
||
| let program_id = ephemeral_spl_api::program::id_address(); | ||
| let delegation_program = ephemeral_spl_api::program::DELEGATION_PROGRAM_ID; | ||
| if !queue_info.owned_by(&program_id) && !queue_info.owned_by(&delegation_program) { | ||
| return Err(ProgramError::IllegalOwner); |
There was a problem hiding this comment.
This instruction lets any signer delegate any mint queue because it only checks payer_info.is_signer() and PDA validity before calling DelegateAccountCpiBuilder; there is no mint owner/admin authorization. In this commit the delegated owner is expected to become DELEGATION_PROGRAM_ID (see e-token/tests/delegate_transfer_queue.rs), but the queue mutating paths (deposit_and_queue_transfer, ensure_transfer_queue_crank, and process_transfer_queue_tick) all reject non-program-owned queues with IllegalOwner, so an arbitrary caller can permanently DoS queue deposits/cranking for a mint by delegating its queue once.
Useful? React with 👍 / 👎.
| transfer_to_vault_for_mint( | ||
| vault_info, | ||
| mint_info, | ||
| user_source_token_acc, | ||
| vault_token_acc, | ||
| user_authority, | ||
| token_program_info, | ||
| mint_info.address(), |
There was a problem hiding this comment.
token_program_info is caller-controlled and never validated against the SPL token program, yet it is used for both destination-account validation and the deposit CPI. A malicious executable can therefore return success without moving funds, after which this handler still pushes queue items; those forged head items later fail settlement because the crank path hardcodes TOKEN_PROGRAM_ID for execution, and failed ticks do not pop the item, effectively wedging the queue for that mint.
Useful? React with 👍 / 👎.
1 participant
No description provided.