diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 955a31b..0153faf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -15,20 +15,20 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - - name: Cache cargo registry, git, bin, and target + - name: Cache cargo registry, git, bin, and target dirs uses: actions/cache@v4 with: path: | - ~/.cargo/bin ~/.cargo/registry ~/.cargo/git + ~/.cargo/bin target tests/target key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }} - name: Install nightly + rustfmt run: rustup update nightly && rustup default nightly && rustup component add rustfmt clippy - name: Install dylint - run: cargo install --locked cargo-dylint@5.0.0 dylint-link@5.0.0 + run: cargo install --locked --force cargo-dylint@5.0.0 dylint-link@5.0.0 - name: Format run: cargo fmt --check - name: Build diff --git a/Cargo.toml b/Cargo.toml index c074fb0..82dcb5a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -6,10 +6,7 @@ authors = ["Anchor Contributors"] publish = false [workspace] -members = [ - "anchor-lints-utils", - "lints/*", -] +members = ["anchor-lints-utils", "lints/*"] resolver = "2" [workspace.package] @@ -29,6 +26,6 @@ regex = "1.12.2" disallowed-methods = "deny" [dependencies] +anyhow = "1.0.100" regex = { workspace = true } tokio = { version = "1.48.0", features = ["fs", "rt-multi-thread", "macros"] } -anyhow = "1.0.100" diff --git a/lints/cpi_no_result/README.md b/lints/cpi_no_result/README.md index c5530a3..fdc870f 100644 --- a/lints/cpi_no_result/README.md +++ b/lints/cpi_no_result/README.md @@ -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(); +system_program::transfer(cpi_ctx, amount).unwrap_or(()); +system_program::transfer(cpi_ctx, amount).unwrap_or_else(|_| ()); +``` + +**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. diff --git a/lints/direct_lamport_cpi_dos/README.md b/lints/direct_lamport_cpi_dos/README.md index 63bec07..59ef12a 100644 --- a/lints/direct_lamport_cpi_dos/README.md +++ b/lints/direct_lamport_cpi_dos/README.md @@ -1,7 +1,30 @@ # `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; +// fee_collector not in CPI +token::transfer( + CpiContext::new(ctx.accounts.token_program.key(), Transfer { ... }), + amount, +)?; + +**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, +)?; +``` diff --git a/lints/duplicate_mutable_accounts/README.md b/lints/duplicate_mutable_accounts/README.md index baa6cfc..2b931b0 100644 --- a/lints/duplicate_mutable_accounts/README.md +++ b/lints/duplicate_mutable_accounts/README.md @@ -4,4 +4,37 @@ Detects duplicate mutable account usage in Anchor functions, where the same account type is passed into multiple mutable parameters without constraint checks. ### Why is this bad? -Duplicate mutable accounts can lead to unexpected aliasing of mutable data, logical errors, and vulnerabilities like account state corruption in Solana smart contracts. \ No newline at end of file +Duplicate mutable accounts can lead to unexpected aliasing of mutable data, logical errors, and vulnerabilities like account state corruption in Solana smart contracts. + +### Example + +**Flagged:** two mutable accounts of the same type, no constraint +```rust +#[derive(Accounts)] +pub struct UnsafeAccounts<'info> { + pub user_a: Account<'info, User>, + pub user_b: Account<'info, User>, +} + +pub fn update(ctx: Context, a: u64, b: u64) -> Result<()> { + ctx.accounts.user_a.data = a; + ctx.accounts.user_b.data = b; + Ok(()) +} +``` + +**OK:** same-type mutable accounts with a constraint so they must be different +```rust +#[derive(Accounts)] +pub struct SafeAccounts<'info> { + #[account(constraint = user_a.key() != user_b.key())] + pub user_a: Account<'info, User>, + pub user_b: Account<'info, User>, +} + +pub fn update(ctx: Context, a: u64, b: u64) -> Result<()> { + ctx.accounts.user_a.data = a; + ctx.accounts.user_b.data = b; + Ok(()) +} +``` \ No newline at end of file diff --git a/lints/overconstrained_seed_account/README.md b/lints/overconstrained_seed_account/README.md index 425d4ec..59cef21 100644 --- a/lints/overconstrained_seed_account/README.md +++ b/lints/overconstrained_seed_account/README.md @@ -6,3 +6,16 @@ Detects when a seed account used in PDA derivation is overconstrained as `System ### Why is this bad? If a seed account's ownership changes after pool creation (e.g., becomes a token account or mint), future instructions will fail forever because `SystemAccount` enforces `owner == system_program`. This can permanently lock funds in the protocol. +### Example + +```rust +// BAD: Using `SystemAccount` for a seed in a non-init instruction is the bad pattern: +#[derive(Accounts)] +pub struct Withdraw<'info> { + #[account(seeds = [b"pool", creator.key().as_ref()], bump)] + pub pool: Account<'info, Pool>, + pub creator: SystemAccount<'info>, // use UncheckedAccount +} +``` + +Use `UncheckedAccount<'info>` (or another appropriate type) for the seed in non-init instructions so that ownership changes do not break the instruction. diff --git a/lints/pda_signer_account_overlap/README.md b/lints/pda_signer_account_overlap/README.md index 76abc90..14f3076 100644 --- a/lints/pda_signer_account_overlap/README.md +++ b/lints/pda_signer_account_overlap/README.md @@ -1,7 +1,30 @@ # `pda_signer_account_overlap` ### What it does -Detects when user-controlled accounts (`UncheckedAccount` or `Option`) are passed to CPIs that use PDAs as signers. +Warns when a **mutable user-controlled account** (`UncheckedAccount` or `Option`) 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.