From 729f02051a37ac59ade813ebc66a3e5247ad5322 Mon Sep 17 00:00:00 2001 From: Vara Prasad Bandaru Date: Thu, 8 Feb 2024 19:00:05 +0530 Subject: [PATCH] Fix lint errors and clarify documentation --- lints/bump_seed_canonicalization/README.md | 16 +++--- lints/bump_seed_canonicalization/src/lib.rs | 44 ++++++++-------- lints/insecure_account_close/README.md | 16 +++--- lints/insecure_account_close/src/lib.rs | 38 +++++++------- lints/missing_owner_check/README.md | 51 +++++++++--------- lints/missing_owner_check/src/lib.rs | 57 +++++++++++---------- lints/missing_signer_check/README.md | 11 ++-- lints/missing_signer_check/src/lib.rs | 25 ++++----- lints/type_cosplay/README.md | 32 ++++++------ lints/type_cosplay/src/lib.rs | 42 +++++++-------- 10 files changed, 167 insertions(+), 165 deletions(-) diff --git a/lints/bump_seed_canonicalization/README.md b/lints/bump_seed_canonicalization/README.md index 8053773..d7ba6c0 100644 --- a/lints/bump_seed_canonicalization/README.md +++ b/lints/bump_seed_canonicalization/README.md @@ -32,12 +32,12 @@ See https://github.com/coral-xyz/sealevel-attacks/blob/master/programs/7-bump-se - For every function containing calls to `solana_program::pubkey::Pubkey::create_program_address` - find the `bump` location from the first argument to `create_program_address` call. - - first argument is the seeds array(`&[&[u8]]`). In general, the seeds are structured with bump as last element: + - first argument is the seeds array(`&[&[u8]]`). In general, the seeds are structured with bump as last element: `&[seed1, seed2, ..., &[bump]]` e.g `&[b"vault", &[bump]]`. - - find the locations of bump. - - If bump is assigned by accessing a struct field - - if bump is assigned from a struct implementing `AnchorDeserialize` trait - - report a warning to use `#[account(...)` macro - - else report "bump may not be constrainted" warning - - else check if the bump is checked using a comparison operation - - report a warning if the bump is not checked + - find the locations of bump. + - If bump is assigned by accessing a struct field + - if bump is assigned from a struct implementing `AnchorDeserialize` trait + - report a warning to use `#[account(...)` macro + - else report "bump may not be constrainted" warning + - else if the bump is checked using a comparison operation; do not report + - else report a warning diff --git a/lints/bump_seed_canonicalization/src/lib.rs b/lints/bump_seed_canonicalization/src/lib.rs index 7272740..88a751e 100644 --- a/lints/bump_seed_canonicalization/src/lib.rs +++ b/lints/bump_seed_canonicalization/src/lib.rs @@ -26,18 +26,18 @@ extern crate rustc_target; dylint_linting::declare_late_lint! { /// **What it does:** - /// + /// /// Finds uses of solana_program::pubkey::PubKey::create_program_address that do not check the bump_seed /// /// **Why is this bad?** - /// + /// /// Generally for every seed there should be a canonical address, so the user should not be /// able to pick the bump_seed, since that would result in a different address. - /// + /// /// See https://github.com/crytic/building-secure-contracts/tree/master/not-so-smart-contracts/solana/improper_pda_validation /// /// **Known problems:** - /// + /// /// False positives, since the bump_seed check may be within some other function (does not /// trace through function calls). The bump seed may be also be safely stored in an account but /// passed from another function. @@ -46,26 +46,26 @@ dylint_linting::declare_late_lint! { /// occur in all possible execution paths) /// /// **Example:** - /// + /// /// See https://github.com/coral-xyz/sealevel-attacks/blob/master/programs/7-bump-seed-canonicalization/insecure/src/lib.rs for an insecure example - /// + /// /// Use instead: - /// + /// /// See https://github.com/coral-xyz/sealevel-attacks/blob/master/programs/7-bump-seed-canonicalization/recommended/src/lib.rs for recommended way to use bump. - /// + /// /// **How the lint is implemented:** - /// + /// /// - For every function containing calls to `solana_program::pubkey::Pubkey::create_program_address` /// - find the `bump` location from the first argument to `create_program_address` call. - /// - first argument is the seeds array(`&[&[u8]]`). In general, the seeds are structured with bump as last element: + /// - first argument is the seeds array(`&[&[u8]]`). In general, the seeds are structured with bump as last element: /// `&[seed1, seed2, ..., &[bump]]` e.g `&[b"vault", &[bump]]`. - /// - find the locations of bump. - /// - If bump is assigned by accessing a struct field - /// - if bump is assigned from a struct implementing `AnchorDeserialize` trait - /// - report a warning to use `#[account(...)` macro - /// - else report "bump may not be constrainted" warning - /// - else check if the bump is checked using a comparison operation - /// - report a warning if the bump is not checked + /// - find the locations of bump. + /// - If bump is assigned by accessing a struct field + /// - if bump is assigned from a struct implementing `AnchorDeserialize` trait + /// - report a warning to use `#[account(...)` macro + /// - else report "bump may not be constrainted" warning + /// - else if the bump is checked using a comparison operation; do not report + /// - else report a warning pub BUMP_SEED_CANONICALIZATION, Warn, "Finds calls to create_program_address that do not check the bump_seed" @@ -191,8 +191,8 @@ enum BackwardDataflowState { } impl BumpSeedCanonicalization { - /// Given the seeds_arg, a location passed to first argument of `create_program_address`, - /// find all locations/alias of bump: &[seed1, .., &[bump]] + /// Given the `seeds_arg`, a location passed to first argument of `create_program_address`, + /// find all locations/alias of bump: `&[seed1, .., &[bump]]` fn find_bump_seed_for_seed_array<'tcx>( cx: &LateContext<'tcx>, body: &'tcx mir::Body<'tcx>, @@ -227,7 +227,7 @@ impl BumpSeedCanonicalization { likely_bump_seed_aliases.push(*rvalue_place); } if_chain! { - // if seed_arg stores bump and rvalue is such that `x.y` (field access) + // if seed_arg stores bump and rvalue is such that `x.y` (field access) if state == BackwardDataflowState::Bump; if let Some(proj) = rvalue_place.iter_projections().find_map(|(_, proj)| { @@ -266,7 +266,7 @@ impl BumpSeedCanonicalization { } } BackwardDataflowState::FirstSeed if elements.len() == 1 => { - // seeds_arg points to bump array [ seed1, ..., &[bump]. seeds_arg stores + // seeds_arg points to bump array [ seed1, ..., &[bump]. seeds_arg stores // the location of &[bump]. update it to store the location of bump. if let Operand::Move(pl) = &elements[FieldIdx::from_u32(0)] { // store the location of bump @@ -309,7 +309,7 @@ impl BumpSeedCanonicalization { return true; } } - // look for chain of assign statements whose value is eventually assigned to the `search_place` and + // look for chain of assign statements whose value is eventually assigned to the `search_place` and // see if any of the intermediate local is in the search_list. // TODO: move this and ArbitraryCPI::is_moved_from to utils. loop { diff --git a/lints/insecure_account_close/README.md b/lints/insecure_account_close/README.md index 364b41b..8cfb4d7 100644 --- a/lints/insecure_account_close/README.md +++ b/lints/insecure_account_close/README.md @@ -2,7 +2,7 @@ **What it does:** -Checks for attempts to close an account by setting its lamports to 0 but +Checks for attempts to close an account by setting its lamports to `0` but not also clearing its data. **Why is this bad?** @@ -18,17 +18,17 @@ None **Example:** See https://github.com/coral-xyz/sealevel-attacks/tree/master/programs/9-closing-accounts for examples of insecure, secure and recommended - approach to close an account. +approach to close an account. **How the lint is implemented:** - For every expression like `(*(*some_expr).lamports.borrow_mut()) = 0;`; assigning `0` to account's lamports - If the body enclosing the expression `is_force_defund`, ignore the expression - - The body contains expressions `some_expr.copy_from_slice(&another_expr[0..8])` and comparison expression - comparing an `[u8; 8]` value. + - The body contains expressions `some_expr.copy_from_slice(&another_expr[0..8])` + and comparison expression comparing an `[u8; 8]` value. - Else If the body contains a manual clear of the account data - - If the body has a for loop like pattern and the loop body has an expression assigning zero - - Assume the loop is clearing the account data and the expression is safe + - If the body has a for loop like pattern and the loop body has an expression + assigning zero + - Assume the loop is clearing the account data and the expression is safe - Else - - report the expression as vulnerable - + - report the expression as vulnerable diff --git a/lints/insecure_account_close/src/lib.rs b/lints/insecure_account_close/src/lib.rs index b89f23c..5d969d7 100644 --- a/lints/insecure_account_close/src/lib.rs +++ b/lints/insecure_account_close/src/lib.rs @@ -16,37 +16,37 @@ use solana_lints::utils::visit_expr_no_bodies; dylint_linting::declare_late_lint! { /// **What it does:** - /// + /// /// Checks for attempts to close an account by setting its lamports to `0` but /// not also clearing its data. - /// + /// /// **Why is this bad?** - /// + /// /// See: https://docs.solana.com/developing/programming-model/transactions#multiple-instructions-in-a-single-transaction - /// + /// /// > An example of where this could be a problem is if a token program, upon transferring the token out of an account, sets the account's lamports to zero, assuming it will be deleted by the runtime. If the program does not zero out the account's data, a malicious user could trail this instruction with another that transfers the tokens a second time. - /// + /// /// **Known problems:** - /// + /// /// None - /// + /// /// **Example:** - /// + /// /// See https://github.com/coral-xyz/sealevel-attacks/tree/master/programs/9-closing-accounts for examples of insecure, secure and recommended - /// approach to close an account. - /// + /// approach to close an account. + /// /// **How the lint is implemented:** - /// + /// /// - For every expression like `(*(*some_expr).lamports.borrow_mut()) = 0;`; assigning `0` to account's lamports /// - If the body enclosing the expression `is_force_defund`, ignore the expression - /// - The body contains expressions `some_expr.copy_from_slice(&another_expr[0..8])` and comparison expression - /// comparing an `[u8; 8]` value. + /// - The body contains expressions `some_expr.copy_from_slice(&another_expr[0..8])` + /// and comparison expression comparing an `[u8; 8]` value. /// - Else If the body contains a manual clear of the account data - /// - If the body has a for loop like pattern and the loop body has an expression assigning zero - /// - Assume the loop is clearing the account data and the expression is safe + /// - If the body has a for loop like pattern and the loop body has an expression + /// assigning zero + /// - Assume the loop is clearing the account data and the expression is safe /// - Else - /// - report the expression as vulnerable - /// + /// - report the expression as vulnerable pub INSECURE_ACCOUNT_CLOSE, Warn, "attempt to close an account without also clearing its data" @@ -134,7 +134,6 @@ fn is_initial_eight_byte_copy_from_slice(expr: &Expr<'_>) -> bool { } } - /// Return true if the body contains an comparison expr and one of the values compared is array: [u8; 8] fn contains_eight_byte_array_comparison<'tcx>( cx: &LateContext<'tcx>, @@ -146,7 +145,6 @@ fn contains_eight_byte_array_comparison<'tcx>( .is_some() } - /// Return true if the expr is a comparison and one of the values is array type: [u8; 8] fn is_eight_byte_array_comparison<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) -> bool { if_chain! { @@ -161,7 +159,7 @@ fn is_eight_byte_array_comparison<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx } } -/// Return true if type of the expr is an Array of type u8 and its length is 8 +/// Return true if type of the expr is an Array of type u8 and its length is 8 fn is_eight_byte_array<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) -> bool { let ty = cx.typeck_results().expr_ty(expr); if_chain! { diff --git a/lints/missing_owner_check/README.md b/lints/missing_owner_check/README.md index 8ad03e2..94dbc84 100644 --- a/lints/missing_owner_check/README.md +++ b/lints/missing_owner_check/README.md @@ -35,33 +35,34 @@ See https://github.com/coral-xyz/sealevel-attacks/blob/master/programs/2-owner-c for a secure example. **How the lint is implemented:** + - for every function defined in the package - exclude functions generated from macro expansion. - Get a list of unique and unsafe AccountInfo's referenced in the body - - for each expression in the function body - - Ignore `.clone()` expressions as the expression referencing original account will be checked - - Check if the expression's type is Solana's AccountInfo (`solana_program::account_info::AccountInfo`) - - Ignore local variable expressions (`x` where x is defined in the function `let x = y`) - - Removes duplcate warnings: both `x` and `y` are reported by the lint. reporting `y` is sufficient. - - Also the owner could be checked on `y`. reporting `x` which a copy/ref of `y` would be false-positive. - - Determine using the expression kind (`.kind`): expr.kind = ExprKind::Path(QPath::Resolved(None, path)); path.segments.len() == 1 - - Ignore safe `.to_account_info()` expressions - - `.to_account_info()` method can be called to convert Anchor different account types to AccountInfo - - The Anchor account types such as `Account` implement `Owner` trait: The owner of the account is checked during deserialization - - The expressions `x.to_account_info` where `x` has one of following types are ignored: - - `Account` requires its type argument to implement `anchor_lang::Owner`. - - `Program`'s implementation of `try_from` checks the account's program id. So there is - no ambiguity in regard to the account's owner. - - `SystemAccount`'s implementation of `try_from` checks that the account's owner is the System Program. - - `AccountLoader` requires its type argument to implement `anchor_lang::Owner`. - - `Signer` are mostly accounts with a private key and most of the times owned by System Program. - - `Sysvar` type arguments checks the account key. - - Ignore `x.to_account_info()` expressions called on Anchor AccountInfo to remove duplicates. - - the lint checks the original expression `x`; no need for checking both. + - for each expression in the function body + - Ignore `.clone()` expressions as the expression referencing original account will be checked + - Check if the expression's type is Solana's `AccountInfo` (`solana_program::account_info::AccountInfo`) + - Ignore local variable expressions (`x` where x is defined in the function `let x = y`) + - Removes duplcate warnings: both `x` and `y` are reported by the lint. reporting `y` is sufficient. + - Also the owner could be checked on `y`. reporting `x` which a copy/ref of `y` would be false-positive. + - Determined using the expression kind (`.kind`): expr.kind = ExprKind::Path(QPath::Resolved(None, path)); path.segments.len() == 1 + - Ignore safe `.to_account_info()` expressions + - `.to_account_info()` method can be called to convert different Anchor account types to `AccountInfo` + - The Anchor account types such as `Account` implement `Owner` trait: The owner of the account is checked during deserialization + - The expressions `x.to_account_info()` where `x` has one of following types are ignored: + - `Account` requires its type argument to implement `anchor_lang::Owner`. + - `Program`'s implementation of `try_from` checks the account's program id. So there is + no ambiguity in regard to the account's owner. + - `SystemAccount`'s implementation of `try_from` checks that the account's owner is the System Program. + - `AccountLoader` requires its type argument to implement `anchor_lang::Owner`. + - `Signer` are mostly accounts with a private key and most of the times owned by System Program. + - `Sysvar` type arguments checks the account key. + - Ignore `x.to_account_info()` expressions called on Anchor `AccountInfo` to remove duplicates. + - the lint checks the original expression `x`; no need for checking both. - For each of the collected expressions, check if `owner` is accessed or if the `key` is compared - - Ignore the `account_expr` if any of the expressions in the function is `{account_expr}.owner` - - Ignore the `account_expr` if `key` is compared - - Check if there is a comparison expression (`==` or `!=`) and one of the expressions being compared accesses key on `account_expr`: - - lhs or rhs of the comparison is `{account_expr}.key()`; The key for Anchor's AccountInfo is accessed using `.key()` - - Or lhs or rhs is `{account_expr}.key`; The key of Solana AccountInfo are accessed using `.key` + - Ignore the `account_expr` if any of the expressions in the function is `{account_expr}.owner` + - Ignore the `account_expr` if `key` is compared + - if there is a comparison expression (`==` or `!=`) and one of the expressions being compared accesses key on `account_expr`: + - lhs or rhs of the comparison is `{account_expr}.key()`; The key for Anchor's `AccountInfo` is accessed using `.key()` + - Or lhs or rhs is `{account_expr}.key`; The key of Solana `AccountInfo` are accessed using `.key` - Report the remaining expressions diff --git a/lints/missing_owner_check/src/lib.rs b/lints/missing_owner_check/src/lib.rs index 94aa77c..b5f7ca2 100644 --- a/lints/missing_owner_check/src/lib.rs +++ b/lints/missing_owner_check/src/lib.rs @@ -53,37 +53,38 @@ dylint_linting::declare_late_lint! { /// /// See https://github.com/coral-xyz/sealevel-attacks/blob/master/programs/2-owner-checks/secure/src/lib.rs /// for a secure example. - /// + /// /// **How the lint is implemented:** + /// /// - for every function defined in the package /// - exclude functions generated from macro expansion. /// - Get a list of unique and unsafe AccountInfo's referenced in the body - /// - for each expression in the function body - /// - Ignore `.clone()` expressions as the expression referencing original account will be checked - /// - Check if the expression's type is Solana's AccountInfo (`solana_program::account_info::AccountInfo`) - /// - Ignore local variable expressions (`x` where x is defined in the function `let x = y`) - /// - Removes duplcate warnings: both `x` and `y` are reported by the lint. reporting `y` is sufficient. - /// - Also the owner could be checked on `y`. reporting `x` which a copy/ref of `y` would be false-positive. - /// - Determine using the expression kind (`.kind`): expr.kind = ExprKind::Path(QPath::Resolved(None, path)); path.segments.len() == 1 - /// - Ignore safe `.to_account_info()` expressions - /// - `.to_account_info()` method can be called to convert Anchor different account types to AccountInfo - /// - The Anchor account types such as `Account` implement `Owner` trait: The owner of the account is checked during deserialization - /// - The expressions `x.to_account_info` where `x` has one of following types are ignored: - /// - `Account` requires its type argument to implement `anchor_lang::Owner`. - /// - `Program`'s implementation of `try_from` checks the account's program id. So there is - /// no ambiguity in regard to the account's owner. - /// - `SystemAccount`'s implementation of `try_from` checks that the account's owner is the System Program. - /// - `AccountLoader` requires its type argument to implement `anchor_lang::Owner`. - /// - `Signer` are mostly accounts with a private key and most of the times owned by System Program. - /// - `Sysvar` type arguments checks the account key. - /// - Ignore `x.to_account_info()` expressions called on Anchor AccountInfo to remove duplicates. - /// - the lint checks the original expression `x`; no need for checking both. + /// - for each expression in the function body + /// - Ignore `.clone()` expressions as the expression referencing original account will be checked + /// - Check if the expression's type is Solana's `AccountInfo` (`solana_program::account_info::AccountInfo`) + /// - Ignore local variable expressions (`x` where x is defined in the function `let x = y`) + /// - Removes duplcate warnings: both `x` and `y` are reported by the lint. reporting `y` is sufficient. + /// - Also the owner could be checked on `y`. reporting `x` which a copy/ref of `y` would be false-positive. + /// - Determined using the expression kind (`.kind`): expr.kind = ExprKind::Path(QPath::Resolved(None, path)); path.segments.len() == 1 + /// - Ignore safe `.to_account_info()` expressions + /// - `.to_account_info()` method can be called to convert different Anchor account types to `AccountInfo` + /// - The Anchor account types such as `Account` implement `Owner` trait: The owner of the account is checked during deserialization + /// - The expressions `x.to_account_info()` where `x` has one of following types are ignored: + /// - `Account` requires its type argument to implement `anchor_lang::Owner`. + /// - `Program`'s implementation of `try_from` checks the account's program id. So there is + /// no ambiguity in regard to the account's owner. + /// - `SystemAccount`'s implementation of `try_from` checks that the account's owner is the System Program. + /// - `AccountLoader` requires its type argument to implement `anchor_lang::Owner`. + /// - `Signer` are mostly accounts with a private key and most of the times owned by System Program. + /// - `Sysvar` type arguments checks the account key. + /// - Ignore `x.to_account_info()` expressions called on Anchor `AccountInfo` to remove duplicates. + /// - the lint checks the original expression `x`; no need for checking both. /// - For each of the collected expressions, check if `owner` is accessed or if the `key` is compared - /// - Ignore the `account_expr` if any of the expressions in the function is `{account_expr}.owner` - /// - Ignore the `account_expr` if `key` is compared - /// - Check if there is a comparison expression (`==` or `!=`) and one of the expressions being compared accesses key on `account_expr`: - /// - lhs or rhs of the comparison is `{account_expr}.key()`; The key for Anchor's AccountInfo is accessed using `.key()` - /// - Or lhs or rhs is `{account_expr}.key`; The key of Solana AccountInfo are accessed using `.key` + /// - Ignore the `account_expr` if any of the expressions in the function is `{account_expr}.owner` + /// - Ignore the `account_expr` if `key` is compared + /// - if there is a comparison expression (`==` or `!=`) and one of the expressions being compared accesses key on `account_expr`: + /// - lhs or rhs of the comparison is `{account_expr}.key()`; The key for Anchor's `AccountInfo` is accessed using `.key()` + /// - Or lhs or rhs is `{account_expr}.key`; The key of Solana `AccountInfo` are accessed using `.key` /// - Report the remaining expressions pub MISSING_OWNER_CHECK, Warn, @@ -105,7 +106,7 @@ impl<'tcx> LateLintPass<'tcx> for MissingOwnerCheck { // get unique and unsafe AccountInfo's referenced in the body let accounts = get_referenced_accounts(cx, body); for account_expr in accounts { - // ignore the account_expr if `.owner` field is accessed in the function + // ignore the account_expr if `.owner` field is accessed in the function // or key of account_expr is compared using `==` or `!=` in the function if !contains_owner_use(cx, body, account_expr) && !contains_key_check(cx, body, account_expr) @@ -239,7 +240,7 @@ fn contains_owner_use<'tcx>( }) } -/// Check if the key of account returned by account_expr is compared +/// Check if the key of account returned by `account_expr` is compared fn contains_key_check<'tcx>( cx: &LateContext<'tcx>, body: &'tcx Body<'tcx>, diff --git a/lints/missing_signer_check/README.md b/lints/missing_signer_check/README.md index 950ce52..ac8b7f5 100644 --- a/lints/missing_signer_check/README.md +++ b/lints/missing_signer_check/README.md @@ -14,7 +14,7 @@ then anyone can create the instruction, call the program and perform a privilege For example if the Token program does not check that the owner of the tokens is a signer in the transfer instruction then anyone can transfer the tokens and steal them. -**Known problems:** +**Known problems:** None. **Example:** @@ -27,8 +27,9 @@ Use instead: See https://github.com/coral-xyz/sealevel-attacks/blob/master/programs/0-signer-authorization/recommended/src/lib.rs for a secure example **How the lint is implemented:** + - For each free function, function not associated with any type or trait. -- Check the function has an expression of type `AccountInfo` -- Check that the function does **not** take a `Context` type argument where `T` has a `Signer` type field -- Check that the function does **not** has an expression `x.is_signer` where the expression `x` is of type `AccountInfo`. -- Report the function +- If the function has an expression of type `AccountInfo` AND +- If the function does **not** take a `Context` type argument where `T` has a `Signer` type field AND +- If the function does **not** has an expression `x.is_signer` where the expression `x` is of type `AccountInfo`. + - Report the function diff --git a/lints/missing_signer_check/src/lib.rs b/lints/missing_signer_check/src/lib.rs index a024d5f..b3366eb 100644 --- a/lints/missing_signer_check/src/lib.rs +++ b/lints/missing_signer_check/src/lib.rs @@ -17,18 +17,18 @@ dylint_linting::declare_late_lint! { /// **What it does:** /// /// This lint reports functions which use `AccountInfo` type and have zero signer checks. - /// + /// /// **Why is this bad?** /// /// The missing-signer-check vulnerability occurs when a program does not check that all the authorative /// accounts have signed the instruction. The issue is lack of proper access controls. Verifying signatures is a way to /// ensure the required entities has approved the operation. If a program does not check the signer field, /// then anyone can create the instruction, call the program and perform a privileged operation. - /// + /// /// For example if the Token program does not check that the owner of the tokens is a signer in the transfer instruction then anyone can /// transfer the tokens and steal them. - /// - /// **Known problems:** + /// + /// **Known problems:** /// None. /// /// **Example:** @@ -39,13 +39,14 @@ dylint_linting::declare_late_lint! { /// Use instead: /// /// See https://github.com/coral-xyz/sealevel-attacks/blob/master/programs/0-signer-authorization/recommended/src/lib.rs for a secure example - /// + /// /// **How the lint is implemented:** + /// /// - For each free function, function not associated with any type or trait. - /// - Check the function has an expression of type `AccountInfo` - /// - Check that the function does **not** take a `Context` type argument where `T` has a `Signer` type field - /// - Check that the function does **not** has an expression `x.is_signer` where the expression `x` is of type `AccountInfo`. - /// - Report the function + /// - If the function has an expression of type `AccountInfo` AND + /// - If the function does **not** take a `Context` type argument where `T` has a `Signer` type field AND + /// - If the function does **not** has an expression `x.is_signer` where the expression `x` is of type `AccountInfo`. + /// - Report the function pub MISSING_SIGNER_CHECK, Warn, "description goes here" @@ -82,7 +83,7 @@ impl<'tcx> LateLintPass<'tcx> for MissingSignerCheck { } } -/// Return true if any of the expression in body has type AccountInfo (`solana_program::account_info::AccountInfo`) +/// Return true if any of the expression in body has type `AccountInfo` (`solana_program::account_info::AccountInfo`) fn body_uses_account_info<'tcx>(cx: &LateContext<'tcx>, body: &'tcx Body<'tcx>) -> bool { visit_expr_no_bodies(body.value, |expr| { let ty = cx.typeck_results().expr_ty(expr); @@ -137,12 +138,12 @@ fn arg_contains_signer_field<'tcx>(cx: &LateContext<'tcx>, arg: GenericArg<'tcx> } } -/// Return true if any of expressions in `body` are `x.is_signer` where `x`'s type is AccountInfo +/// Return true if any of expressions in `body` are `x.is_signer` where `x`'s type is `AccountInfo` fn body_contains_is_signer_use<'tcx>(cx: &LateContext<'tcx>, body: &'tcx Body<'tcx>) -> bool { visit_expr_no_bodies(body.value, |expr| is_is_signer_use(cx, expr)) } -/// Return true if the `expr` is `x.is_signer` where `x`'s type is AccountInfo. +/// Return true if the `expr` is `x.is_signer` where `x`'s type is `AccountInfo`. fn is_is_signer_use<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) -> bool { if_chain! { // `expr` is `x.{field_name}` diff --git a/lints/type_cosplay/README.md b/lints/type_cosplay/README.md index 13bb4e7..d04983f 100644 --- a/lints/type_cosplay/README.md +++ b/lints/type_cosplay/README.md @@ -1,6 +1,6 @@ # type_cosplay -**What it does:** +**What it does:** Checks that all deserialized types have a proper discriminant so that all types are guaranteed to deserialize differently. @@ -27,7 +27,7 @@ tell the difference between deserialized type `X` and deserialized type `Y`. Thi malicious user to substitute `X` for `Y` or vice versa, and the code may perform unauthorized actions with the bytes. -**Known problems:** +**Known problems:** In the case when only one enum is deserialized, this lint by default regards that as secure. However, this is not always the case. For example, if the program @@ -118,22 +118,22 @@ an implicit discriminant, this program will always be secure as long as all type **How the lint is implemented:** - Find call expressions which has an arg that accesses account data - - Arg expression contains `x.data`; `x` is of type `AccountInfo` + - Arg expression contains `x.data`; `x` is of type `AccountInfo` - Get the type that the function was called on, ie X in X::call() - if `X` implements `anchor_lang::Discriminator` trait but the function called is not `try_deserialize` - - warn to use `try_deserialize` or to account for type's discriminator + - warn to use `try_deserialize` or to account for type's discriminator - else if the function called is Borsh `try_from_slice`, collect the deserialized type - Repeat the above for all call expressions and collect all deserialized types; `X` from `X::try_from_slice()` expressions. -- If number of different kinds of types deserialized is more than `1`, i.e the code deserializes `Enum` type, `Struct` type, etc. - - warn to either deserialize from only structs or only an enum -- If the deserialized types are all enum - - If number of deserialized enums are more than `1` - - warn to use single enum that contains all type definitions - - Else assume that the single enum is safe and do not report +- If number of different kinds of types deserialized is more than `1`, i.e the + code deserializes `Enum` type as well as a `Struct` type, etc. + - warn to either deserialize from only structs or only an enum +- Else If the deserialized types are all enum + - If number of deserialized enums are more than `1` + - warn to use single enum that contains all type definitions + - Else assume that the single enum is safe and do not warn - Else the deserialized types are structs - - For each deserialized type - - If the struct has first field of type enum and number of variants of the enum are more than the - number of deserialized types - - The struct has proper discriminant - - Else warn to add an enum with at least as many variants as there are deserialized types. - + - For each deserialized type + - If the struct has first field of type enum and number of variants of the enum are more than the + number of deserialized types + - The struct has proper discriminant; do not warn. + - Else warn to add an enum with at least as many variants as there are deserialized types. diff --git a/lints/type_cosplay/src/lib.rs b/lints/type_cosplay/src/lib.rs index ebf26d9..df09443 100644 --- a/lints/type_cosplay/src/lib.rs +++ b/lints/type_cosplay/src/lib.rs @@ -26,8 +26,8 @@ use rustc_target::abi::FieldIdx; use solana_lints::{paths, utils::visit_expr_no_bodies}; dylint_linting::impl_late_lint! { - /// **What it does:** - /// + /// **What it does:** + /// /// Checks that all deserialized types have a proper discriminant so that /// all types are guaranteed to deserialize differently. /// @@ -46,15 +46,15 @@ dylint_linting::impl_late_lint! { /// are guaranteed to be unique since the program will have to match a specific variant. /// /// **Why is this bad?** - /// + /// /// The type cosplay issue is when one account type can be substituted for another account type. /// This occurs when a type deserializes exactly the same as another type, such that you can't /// tell the difference between deserialized type `X` and deserialized type `Y`. This allows a /// malicious user to substitute `X` for `Y` or vice versa, and the code may perform unauthorized /// actions with the bytes. /// - /// **Known problems:** - /// + /// **Known problems:** + /// /// In the case when only one enum is deserialized, this lint by default /// regards that as secure. However, this is not always the case. For example, if the program /// defines another enum and serializes, but never deserializes it, a user could create this enum, @@ -140,29 +140,29 @@ dylint_linting::impl_late_lint! { /// This example fixes both the insecure and insecure-2 examples. It is secure because it only deserializes /// from a single enum, and that enum encapsulates all of the user-defined types. Since enums contain /// an implicit discriminant, this program will always be secure as long as all types are defined under the enum. - /// + /// /// **How the lint is implemented:** - /// + /// /// - Find call expressions which has an arg that accesses account data - /// - Arg expression contains `x.data`; `x` is of type `AccountInfo` + /// - Arg expression contains `x.data`; `x` is of type `AccountInfo` /// - Get the type that the function was called on, ie X in X::call() /// - if `X` implements `anchor_lang::Discriminator` trait but the function called is not `try_deserialize` - /// - warn to use `try_deserialize` or to account for type's discriminator + /// - warn to use `try_deserialize` or to account for type's discriminator /// - else if the function called is Borsh `try_from_slice`, collect the deserialized type /// - Repeat the above for all call expressions and collect all deserialized types; `X` from `X::try_from_slice()` expressions. - /// - If number of different kinds of types deserialized is more than `1`, i.e the code deserializes `Enum` type, `Struct` type, etc. - /// - warn to either deserialize from only structs or only an enum - /// - If the deserialized types are all enum - /// - If number of deserialized enums are more than `1` - /// - warn to use single enum that contains all type definitions - /// - Else assume that the single enum is safe and do not report + /// - If number of different kinds of types deserialized is more than `1`, i.e the + /// code deserializes `Enum` type as well as a `Struct` type, etc. + /// - warn to either deserialize from only structs or only an enum + /// - Else If the deserialized types are all enum + /// - If number of deserialized enums are more than `1` + /// - warn to use single enum that contains all type definitions + /// - Else assume that the single enum is safe and do not warn /// - Else the deserialized types are structs - /// - For each deserialized type - /// - If the struct has first field of type enum and number of variants of the enum are more than the - /// number of deserialized types - /// - The struct has proper discriminant - /// - Else warn to add an enum with at least as many variants as there are deserialized types. - /// + /// - For each deserialized type + /// - If the struct has first field of type enum and number of variants of the enum are more than the + /// number of deserialized types + /// - The struct has proper discriminant; do not warn. + /// - Else warn to add an enum with at least as many variants as there are deserialized types. pub TYPE_COSPLAY, Warn, "type is equivalent to another type",