Fix lint errors and clarify documentation

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

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 {

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

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! {

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