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

fix: 🐛 Components CPI caller check #87

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

iamnamananand996
Copy link
Contributor

@iamnamananand996 iamnamananand996 commented Oct 13, 2024

Status Type ⚠️ Core Change Issue
Ready Bug Yes #29

Problem

  • 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, This may contains a bug, since a transaction could contain a valid instruction at index [0], but be malicious.

Full description is available: #29 (comment)

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.

  • Closes: [Bug/Investigation] Components CPI caller check #29

cc - @GabrielePicco

@GabrielePicco GabrielePicco force-pushed the bug/Investigation-components-CPI-caller-check branch from 1975579 to 93962aa Compare October 16, 2024 10:20
@GabrielePicco
Copy link
Contributor

@iamnamananand996 the PR looks good to me, but we should enforce that the check is now correct. There are 2 integration tests for the CPI check that were passing with the previous implementation. Is there a case where prev check fails and this pass that we can add>

@iamnamananand996
Copy link
Contributor Author

Hi @GabrielePicco , thanks for pointing out, this a valid check we should add before merging it, I will check for the above suggestion and update this accordingly.

@iamnamananand996 iamnamananand996 force-pushed the bug/Investigation-components-CPI-caller-check branch 3 times, most recently from 51432c7 to 0e11c24 Compare November 6, 2024 14:40
@iamnamananand996 iamnamananand996 force-pushed the bug/Investigation-components-CPI-caller-check branch from 10dd493 to f05556d Compare November 6, 2024 16:02
@iamnamananand996
Copy link
Contributor Author

Hi @GabrielePicco, I check all the test, and regarding the test case you mention 2 integration tests for the CPI check, that are passing.

Screenshot 2024-11-06 213727 Screenshot 2024-11-06 201710

and there where 2 test which where failing, that I have updated. failed test

@iamnamananand996 iamnamananand996 force-pushed the bug/Investigation-components-CPI-caller-check branch 2 times, most recently from 5a69de5 to baaedb3 Compare November 6, 2024 16:17
@GabrielePicco
Copy link
Contributor

Thanks @iamnamananand996 ,
What I meant is that we should add a test to verify that this implementation is actually solving a problem. The two integration tests for the CPI check also pass in the current version of the master branch, which means they don't cover the issue this PR is trying to address

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug/Investigation] Components CPI caller check
2 participants