-
Notifications
You must be signed in to change notification settings - Fork 0
Lint docs updates #10
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
Merged
Merged
Changes from 12 commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
d456009
fix(arbitrary_cpi_call): use Instruction program_id local for validat…
meumar-osec 649e236
docs(cpi_no_result): update README wording and examples
meumar-osec 9d2aea0
docs(direct_lamport_cpi_dos): update README wording and examples
meumar-osec e5bbc3e
docs(duplicate_mutable_accounts): update README examples
meumar-osec e897950
feat: LazyAccount support for missing_account_reload lint
meumar-osec 5c62dec
docs: overconstrained_seed_account readme updated with example
meumar-osec a46dc5d
docs(pda_signer_account_overlap): readme update
meumar-osec 74cbd8f
chore: cargo fmt
meumar-osec 5fefa51
feat: new lint missing_mut_constraint
meumar-osec 7169d67
Revert "feat: new lint missing_mut_constraint"
meumar-osec 4867cbf
Revert "feat: LazyAccount support for missing_account_reload lint"
meumar-osec 767878d
Revert "fix(arbitrary_cpi_call): use Instruction program_id local for…
meumar-osec 8d00c20
chore: Use `cargo-install` action to install package (#14)
jamie-osec d4c56c8
Update lints/cpi_no_result/README.md
meumar-osec 3298995
Update lints/direct_lamport_cpi_dos/README.md
meumar-osec f547cf8
Update lints/duplicate_mutable_accounts/README.md
meumar-osec File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,28 @@ | ||
| # `cpi_no_result` | ||
|
|
||
| ### What it does | ||
| Detects CPI calls where the result is silently suppressed using methods like `unwrap_or_default()` or `unwrap_or(())`. | ||
| Flags CPI calls when **discard the result** with one of these methods: | ||
|
|
||
| - `unwrap_or_default()` | ||
| - `unwrap_or(())` or `unwrap_or(some_value)` | ||
| - `unwrap_or_else(|_| ...)` | ||
|
|
||
| ### Why it matters | ||
| Discarding the CPI result makes it unclear how failures are handled. Even though many CPI failures abort the transaction, hiding the result makes the code harder to understand and debug. Using ? or explicit error handling makes it clear when a CPI can fail and ensures failures are handled in a consistent and readable way. | ||
|
|
||
| ### Example | ||
|
|
||
| **Flagged:** | ||
| ```rust | ||
| system_program::transfer(cpi_ctx, amount).unwrap_or_default(); // [cpi_no_result] | ||
| system_program::transfer(cpi_ctx, amount).unwrap_or(()); // [cpi_no_result] | ||
| system_program::transfer(cpi_ctx, amount).unwrap_or_else(|_| ()); // [cpi_no_result] | ||
| ``` | ||
|
|
||
| **OK():** | ||
| ```rust | ||
| system_program::transfer(cpi_ctx, amount)?; | ||
| system_program::transfer(cpi_ctx, amount).unwrap(); | ||
| system_program::transfer(cpi_ctx, amount).expect("Transfer failed"); | ||
| ``` | ||
|
|
||
| ### Why is this bad? | ||
| Silent suppression methods hide CPI failures, allowing the program to continue execution even when critical operations failed, leading to silent failures, security vulnerabilities, potential fund loss, and state corruption. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,31 @@ | ||
| # `direct_lamport_cpi_dos` | ||
|
|
||
| ### What it does | ||
| Detects when accounts with direct lamport mutations (via `lamports.borrow_mut()`) are not included in subsequent CPI calls. | ||
| Flags when an account’s lamports are mutated (e.g. `**ctx.accounts.fee_collector.lamports.borrow_mut() += x`) and that account is not included in a later CPI in the same function. | ||
|
|
||
| ### Why is this bad? | ||
| The Solana runtime performs balance checks on CPI calls using only the accounts involved in the CPI. When an account's lamports are directly mutated but the account is not included in the CPI, the runtime balance check will fail, causing a DoS error. All accounts whose lamports were changed directly must be included in subsequent CPIs as remaining accounts using `with_remaining_accounts`. | ||
| ### Why it matters | ||
| The runtime checks balances for accounts in the CPI. If you changed an account’s lamports but don’t pass it in the CPI, the tx will abort. The runtime catches this too; the lint gives earlier feedback and a clearer message so you can fix it before running the failing path. Include every mutated account in the CPI, e.g. via `with_remaining_accounts`. | ||
|
|
||
| ### Example | ||
|
|
||
| **Flagged:** lamport mutation then CPI without that account | ||
| ```rust | ||
| **ctx.accounts.vault.lamports.borrow_mut() -= WITHDRAW_FEE; | ||
| **ctx.accounts.fee_collector.lamports.borrow_mut() += WITHDRAW_FEE; | ||
|
|
||
| token::transfer( // [direct_lamport_cpi_dos] — fee_collector not in CPI | ||
| CpiContext::new(ctx.accounts.token_program.key(), Transfer { ... }), | ||
| amount, | ||
| )?; | ||
| ``` | ||
|
meumar-osec marked this conversation as resolved.
Outdated
|
||
|
|
||
| **OK():** same mutations, account included in CPI | ||
| ```rust | ||
| **ctx.accounts.vault.lamports.borrow_mut() -= WITHDRAW_FEE; | ||
| **ctx.accounts.fee_collector.lamports.borrow_mut() += WITHDRAW_FEE; | ||
|
|
||
| token::transfer( | ||
| CpiContext::new(...).with_remaining_accounts(vec![ctx.accounts.fee_collector.to_account_info()]), | ||
| amount, | ||
| )?; | ||
| ``` | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,30 @@ | ||
| # `pda_signer_account_overlap` | ||
|
|
||
| ### What it does | ||
| Detects when user-controlled accounts (`UncheckedAccount` or `Option<UncheckedAccount>`) are passed to CPIs that use PDAs as signers. | ||
| Warns when a **mutable user-controlled account** (`UncheckedAccount` or `Option<UncheckedAccount>`) is passed into a CPI that also uses a **PDA as a signer** (e.g. `CpiContext::new_with_signer`). The lint only fires if both show up in the same CPI and the account is mutable. | ||
|
|
||
| ### Why is this bad? | ||
| This is especially risky when the callee expects the account to be uninitialized. An attacker could pass the PDA signer itself as the account, causing the PDA to be initialized and losing its lamports, leading to security vulnerabilities. | ||
| ### Why it matters | ||
| If the callee program expects an account to be uninitialized and then initializes it inside the CPI (e.g. with `invoke_signed`), an attacker could pass the PDA signer itself—or another account they control—into that slot. The callee might then initialize it with the program’s authority, which can lock lamports or break security assumptions. This pattern has appeared in real bugs (e.g. Meteora-style). The risk is highest when the callee creates or initializes accounts in the CPI. | ||
|
|
||
| ### Example | ||
|
|
||
| **Bad:** A mutable `UncheckedAccount` and a PDA are both passed to a CPI that uses a signer. The user can supply any account (including the PDA) for `user_account`. | ||
|
|
||
| ```rust | ||
| #[derive(Accounts)] | ||
| pub struct UnsafeAccountWithPdaSigner<'info> { | ||
| #[account(mut)] | ||
| pub user_account: UncheckedAccount<'info>, // user-controlled | ||
|
|
||
| #[account(seeds = [b"pool_authority", pool.key().as_ref()], bump)] | ||
| pub pool_authority: AccountInfo<'info>, // PDA used as signer | ||
|
|
||
| pub pool: Account<'info, PoolState>, | ||
| pub target_program: UncheckedAccount<'info>, | ||
| } | ||
|
|
||
| // In the instruction: both user_account and pool_authority are passed to CpiContext::new_with_signer(...) | ||
| // So lint warns: user-controlled account passed to CPI with PDA signer | ||
| ``` | ||
|
|
||
| **Good:** Either avoid passing mutable `UncheckedAccount` in the same CPI as the PDA signer, or add a constraint so the unchecked account cannot be the PDA (e.g. `constraint = user_account.key() != pool_authority.key()`). If the account is not mutable, the lint does not warn. |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.