fix: store_data may cache incorrect owner

[ananas-block]

Summary by CodeRabbit

• Refactor

  • CPI context now propagates the invoking program identifier through processing to ensure stored context reflects the caller.

• Bug Fixes

  • Calls that previously returned a benign default when no input accounts existed now raise a clear error to surface missing-account issues.

• Chores

  • Updated tests to exercise and validate the new invoking-program propagation and error behavior.

[coderabbitai]

Warning

Rate limit exceeded

@ananas-block has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 11 minutes and 31 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

📝 Walkthrough

Walkthrough

Threads an invoking_program: Pubkey through CPI context handling: process_invoke_cpi now passes the invoking program into process_cpi_context → set_cpi_context → ZCpiContextAccount2::store_data, which uses the provided invoking_program as owner bytes. Also, instruction_data.owner() implementations in zero-copy now panic when called with no input accounts.

Changes

Cohort / File(s)	Summary
Zero-copy owner behavior program-libs/compressed-account/src/instruction_data/zero_copy.rs	owner() implementations changed to panic with "owner() called with no input accounts" when there are no input accounts instead of returning Pubkey::default().
CPI context core programs/system/src/cpi_context/process_cpi_context.rs, programs/system/src/cpi_context/state.rs	Added invoking_program: Pubkey parameter to process_cpi_context and set_cpi_context; updated ZCpiContextAccount2::store_data to accept invoking_program and use it to set owner bytes rather than extracting from instruction data.
Invoke CPI callsite programs/system/src/invoke_cpi/processor.rs	Threaded invoking_program into process_cpi_context call within process_invoke_cpi.
Tests programs/system/tests/cpi_context.rs	Updated tests and helpers to pass the new invoking_program parameter through set_cpi_context, process_cpi_context, and store_data calls; signatures and assertions updated accordingly.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant Caller
participant InvokeCPI
participant CPIContext
participant Storage
Caller->>InvokeCPI: send invoke_cpi(instruction, invoking_program, accounts)
InvokeCPI->>CPIContext: process_cpi_context(instruction_data, cpi_account, fee_payer, invoking_program, remaining)
CPIContext->>Storage: ZCpiContextAccount2::store_data(wrapped_instruction_data, invoking_program)
Storage-->>CPIContext: acknowledge stored state (owner := invoking_program)
CPIContext-->>InvokeCPI: context processed
InvokeCPI-->>Caller: return result

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• chore: remove unwraps system program #1803: Modifies CPI-context processing paths that overlap with the set_cpi_context / process_cpi_context changes.
• fix: ctoken create mint cpi context address tree check #2039: Changes zero-copy CPI-context types/deserialization, overlapping with owner/store_data adjustments.

Suggested labels

ai-review

Suggested reviewers

• sergeytimoshin
• SwenSchaeferjohann

Poem

✨ A Pubkey threaded down the lane,
From caller to context — owner’s name.
Tests updated, and zero-copy cries,
If no inputs come, it loudly sighs. 😊

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix: store_data may cache incorrect owner' directly aligns with the core change: modifying how the owner field is set in store_data by using invoking_program instead of instruction_data.owner().
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 70.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch jorrit/fix-store_data-may-cache-incorrect-owner

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

programs/system/tests/cpi_context.rs (2)

350-394: ⚠️ Potential issue | 🟠 Major

create_test_instruction_data uses empty new_address_params, so the owner fix is not exercised.

new_address_params is vec![] on line 357. Since the entire point of this PR is fixing owner assignment in store_data for new addresses, none of the existing tests actually store any new addresses via the fixed code path. This means:

1. The fix at state.rs:136 (let owner_bytes = invoking_program) is never reached in tests.
2. There's no regression coverage for the bug being fixed.

Consider adding a test variant where new_address_params is non-empty and then asserting that the deserialized CpiContextNewAddressParamsAssignedPacked.owner equals invoking_program (not whatever owner the instruction data originally specified).

Would you like me to draft a test case that exercises the new address path and verifies the owner is correctly set to invoking_program?

---

836-886: 🛠️ Refactor suggestion | 🟠 Major

Malicious input scenario: good coverage for the clearing behavior, but missing owner assertion.

The test at line 846-858 injects malicious data via store_data, then verifies the first_set_context path clears it. This is valuable for testing the clear-on-first-set behavior.

However, even in this scenario, no new addresses are present (per create_test_instruction_data), so the invoking_program → owner_bytes path in store_data remains untested.

programs/system/src/cpi_context/state.rs (1)

138-153: ⚠️ Potential issue | 🟡 Minor

Test coverage gap: the new_addresses_eq helper doesn't verify stored address owners match invoking_program.

The NewAddress trait provides an owner() method (returning Option<&[u8; 32]>), and CpiContextNewAddressParamsAssignedPacked implements it to return the stored owner. However, the test helper at programs/system/tests/cpi_context.rs:98-110 compares seed, address_queue_index, address_merkle_tree_account_index, address_merkle_tree_root_index, and assigned_compressed_account_index — but skips owner().

Since this PR's purpose is to fix the owner being cached incorrectly, the test should verify that:

1. Addresses are stored with owner equal to the invoking_program pubkey passed to store_data().
2. This is confirmed by deserializing the CPI context account and asserting each address's owner matches the invoking program.

Without this check, the fix lacks test coverage at the unit level.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (5)

program-libs/compressed-account/src/instruction_data/zero_copy.rs (2)

540-551: 🧹 Nitpick | 🔵 Trivial

Duplicate owner() implementation — inherent method shadows the trait method.

ZInstructionDataInvokeCpi has two identical owner() methods: one as an inherent impl (line 540) and one in the InstructionData trait impl (line 585). The inherent method will be called by default on a concrete ZInstructionDataInvokeCpi value, potentially surprising callers who expect the trait method. Since the bodies are identical, consider removing the inherent method and always going through the trait.

Also applies to: 585-596

---

1303-1315: ⚠️ Potential issue | 🔴 Critical

This test will now panic — the else branch calls owner() with no inputs.

With the new panic behavior on line 469, the else branch at line 1314 (z_copy.owner()) will panic whenever input_compressed_accounts_with_merkle_context is empty (which happens for 80% of iterations — every i where i % 5 != 0).

You need to update this test to match the new semantics. The simplest fix: only assert owner() when inputs are non-empty, and use std::panic::catch_unwind (or #[should_panic] in a dedicated test) to verify the panic path.

🐛 Proposed fix

             // Check owner() method returns expected value
             if !invoke_ref
                 .input_compressed_accounts_with_merkle_context
                 .is_empty()
             {
                 let expected_owner: Pubkey = invoke_ref
                     .input_compressed_accounts_with_merkle_context[0]
                     .compressed_account
                     .owner;
                 assert_eq!(z_copy.owner(), expected_owner);
-            } else {
-                assert_eq!(z_copy.owner(), Pubkey::default());
             }

programs/system/src/cpi_context/state.rs (1)

127-153: 🧹 Nitpick | 🔵 Trivial

This is the heart of the bug fix — looks correct.

Previously, owner_bytes was derived from instruction_data.owner(), which could return an incorrect value (or panic when no input accounts exist). Now it's taken directly from the verified invoking_program, which is the actual program that made the CPI call.

Note that owner_bytes is only used for new addresses (line 144). Input accounts (line 167) and output accounts (line 219) correctly use per-account owners from the instruction data itself — that's the right design since those owners are individually validated.

One minor naming nit: owner_bytes is now a Pubkey, not raw bytes. Consider renaming to just owner or invoking_program_key for clarity, though this is non-blocking.

✏️ Optional: rename for clarity

-        // Use invoking_program as the owner for new addresses
-        let owner_bytes = invoking_program;
+        // Use invoking_program as the owner for new addresses
+        let owner = invoking_program;

And update line 144 accordingly:

-                owner: owner_bytes, // Use cached owner bytes
+                owner, // Use invoking program as owner

programs/system/tests/cpi_context.rs (2)

846-858: 🛠️ Refactor suggestion | 🟠 Major

Malicious data injection scenario updated correctly, but same owner verification gap applies.

The test injects malicious data then overwrites it via first_set_context. The store_data call at line 857 now correctly passes invoking_program. However, since instruction_data_eq doesn't compare the owner field of new addresses, this test wouldn't detect if a malicious invocation cached a wrong owner that survived the overwrite (in the new address context specifically). The assertions at line 880 verify data integrity but not owner integrity.

That said, the first_set_context path clears the account entirely (via deserialize_cpi_context_account_cleared), so old malicious owner data would be wiped regardless. The scenario is sound — just add the owner assertion as suggested above for completeness.

---

420-444: ⚠️ Potential issue | 🟠 Major

Tests don't verify that the owner field is correctly stored in new addresses.

This PR fixes a bug where invoking_program could be cached as the wrong owner. The tests have been updated to thread invoking_program through all call sites—good. However, new_addresses_eq (lines 103–109) deliberately omits the owner field from its comparison, even though the NewAddress trait exposes it. This means the tests don't verify that the stored owner matches invoking_program.

Add an explicit assertion after deserializing the CPI context to confirm that each new address has owner set to the expected invoking_program:

for addr in cpi_context.new_addresses.iter() {
    assert_eq!(addr.owner, invoking_program, "owner must match invoking_program");
}

Without this check, the fix exists in the code but remains unverified by tests.