Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug/Investigation] Components CPI caller check #29

Open
GabrielePicco opened this issue Mar 14, 2024 · 1 comment · May be fixed by #87
Open

[Bug/Investigation] Components CPI caller check #29

GabrielePicco opened this issue Mar 14, 2024 · 1 comment · May be fixed by #87
Assignees
Labels
bug Something isn't working priority: medium question Further information is requested

Comments

@GabrielePicco
Copy link
Contributor

GabrielePicco commented Mar 14, 2024

Describe the bug
Components make use of solana_program::sysvar::instructions::get_instruction_relative to enforce that they are called from CPI and the identify of the caller, e.g.: https://github.com/magicblock-labs/bolt/blob/main/programs/bolt-component/src/lib.rs#L11

This may contains a bug, since a transaction could contain a valid instruction at index [0], but be malicious .

Proposed solution

  • Investigate if that's the case and add stronger test cases and fixes if needed.
  • Investigate an alternative solution, e.g. invoke the systems/components signing with a PDA
@GabrielePicco GabrielePicco added bug Something isn't working question Further information is requested labels Mar 14, 2024
@GabrielePicco GabrielePicco self-assigned this Apr 2, 2024
@iamnamananand996
Copy link
Contributor

iamnamananand996 commented Oct 13, 2024

The issue created from using solana_program::sysvar::instructions::get_instruction_relative(0, ...) to check if a program was called via Cross-Program Invocation (CPI) or directly. The code wrongly assumes this will return the calling instruction during a CPI and the current instruction when called directly. However, get_instruction_relative(0, ...) always returns the current program's instruction.

Understanding get_instruction_relative

The get_instruction_relative function retrieves an instruction relative to the currently executing instruction based on the call stack.
Here's how it works:

  • Index 0: Refers to the currently executing instruction.
  • Index -1: Refers to the instruction that called the currently executing instruction (useful for CPI).

Why the Bug Occurs

In bellow code, following check is used:

let instruction = get_instruction_relative(0, &ctx.accounts.instruction_sysvar_account.to_account_info()).unwrap();

if instruction.program_id == id() {
    panic!("The instruction must be called from a CPI");
}

This check fails because get_instruction_relative(0, ...) always retrieves the current program’s instruction, even during CPI. So the panic is triggered both for direct calls and CPI.

Potential Exploit

An attacker could exploit this behavior by crafting a transaction that bypasses this check. For instance, they could create a transaction where they call your program directly but manipulate the instruction sysvar to make it appear as if it was called via CPI, or they could exploit the misunderstanding of how get_instruction_relative works to bypass the check entirely.

Proposed Solution

To correctly determine if your program was called via CPI, you should:

  • Use get_instruction_relative(-1, ...): This will attempt to retrieve the instruction that called the currently executing instruction. If the program was called directly, there will be no instruction at index -1, and the function will return an error.

  • Check for Errors: If get_instruction_relative(-1, ...) returns an Err, it means the program was called directly. If it returns Ok, you can proceed, knowing it was called via CPI.

cc - @GabrielePicco

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority: medium question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants