Skip to content

Commit

Permalink
fix lint-6 tests and add readme
Browse files Browse the repository at this point in the history
  • Loading branch information
oslfmt committed Jul 19, 2022
1 parent d0f89cc commit 660c76b
Show file tree
Hide file tree
Showing 5 changed files with 117 additions and 40 deletions.
47 changes: 47 additions & 0 deletions lints/duplicate-mutable-accounts/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
# duplicate_mutable_accounts

**What it does:** Checks to make sure there is a key check on identical Anchor accounts.
The key check serves to make sure that two identical accounts do not have the same key,
ie, they are unique. An Anchor account (`Account<'info, T>`) is identical to another if
the generic parameter `T` is the same type for each account.

**Why is this bad?** If a program contains two identical, mutable Anchor accounts, and
performs some operation on those accounts, then a user could pass in the same account
twice. Then any previous operations may be overwritten by the last operation, which may
not be what the program wanted if it expected different accounts.

**Known problems:** If a program is not using the anchor `#[account]` macro constraints,
and is instead using checks in the function bodies, and the program uses boolean operator
&& or || to link constraints in a single if statement, the lint will flag this as a false
positive since the lint only catches statements with `==` or `!=`.

Another issue is if a program uses an if statement such as `a.key() == b.key()` and then
continues to modify the accounts, then this will not be caught. The reason is because the
lint regards expressions with `==` as a secure check, since it assumes the program will
then return an error (see the secure example). However, it does not explicitly check that
an error is returned.

In general, this lint will catch all vulnerabilities if the anchor macro constraints are
used (see the recommended example). It is not as robust if alternative methods are utilized.
Thus it is encouraged to use the anchor `#[account]` macro constraints.

**Example:**

```rust
#[derive(Accounts)]
pub struct Update<'info> {
user_a: Account<'info, User>,
user_b: Account<'info, User>,
}
```

Use instead:

```rust
#[derive(Accounts)]
pub struct Update<'info> {
#[account(constraint = user_a.key() != user_b.key())]
user_a: Account<'info, User>,
user_b: Account<'info, User>,
}
```
43 changes: 35 additions & 8 deletions lints/duplicate-mutable-accounts/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,24 +27,51 @@ use solana_lints::paths;
const ANCHOR_ACCOUNT_GENERIC_ARG_COUNT: usize = 2;

dylint_linting::impl_late_lint! {
/// **What it does:**
/// **What it does:** Checks to make sure there is a key check on identical Anchor accounts.
/// The key check serves to make sure that two identical accounts do not have the same key,
/// ie, they are unique. An Anchor account (`Account<'info, T>`) is identical to another if
/// the generic parameter `T` is the same type for each account.
///
/// **Why is this bad?**
/// **Why is this bad?** If a program contains two identical, mutable Anchor accounts, and
/// performs some operation on those accounts, then a user could pass in the same account
/// twice. Then any previous operations may be overwritten by the last operation, which may
/// not be what the program wanted if it expected different accounts.
///
/// **Known problems:** None.
/// **Known problems:** If a program is not using the anchor #[account] macro constraints,
/// and is instead using checks in the function bodies, and the program uses boolean operator
/// && or || to link constraints in a single if statement, the lint will flag this as a false
/// positive since the lint only catches statements with `==` or `!=`.
/// Another issue is if a program uses an if statement such as `a.key() == b.key()` and then
/// continues to modify the accounts, then this will not be caught. The reason is because the
/// lint regards expressions with `==` as a secure check, since it assumes the program will
/// then return an error (see the secure example). However, it does not explicitly check that
/// an error is returned.
///
/// In general, this lint will catch all vulnerabilities if the anchor macro constraints are
/// used (see the recommended example). It is not as robust if alternative methods are utilized.
/// Thus it is encouraged to use the anchor `#[account]` macro constraints.
///
/// **Example:**
/// **Example:**
///
/// ```rust
/// // example code where a warning is issued
/// #[derive(Accounts)]
/// pub struct Update<'info> {
/// user_a: Account<'info, User>,
/// user_b: Account<'info, User>,
/// }
/// ```
/// Use instead:
/// ```rust
/// // example code that does not raise a warning
/// #[derive(Accounts)]
/// pub struct Update<'info> {
/// #[account(constraint = user_a.key() != user_b.key())]
/// user_a: Account<'info, User>,
/// user_b: Account<'info, User>,
/// }
/// ```
pub DUPLICATE_MUTABLE_ACCOUNTS,
Warn,
"description goes here",
"does not check if multiple identical Anchor accounts have different keys",
DuplicateMutableAccounts::default()
}

Expand Down Expand Up @@ -156,7 +183,7 @@ impl<'tcx> LateLintPass<'tcx> for DuplicateMutableAccounts {
cx,
DUPLICATE_MUTABLE_ACCOUNTS,
*first,
"the following expressions have equivalent Account types, yet do not contain a proper key check.",
&format!("the expressions on line {:?} and {:?} have identical Account types, yet do not contain a proper key check.", first, second),
Some(*second),
"add a key check to make sure the accounts have different keys, e.g., x.key() != y.key()",
);
Expand Down
3 changes: 3 additions & 0 deletions lints/duplicate-mutable-accounts/ui/insecure-2/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,15 @@ pub mod duplicate_mutable_accounts_insecure {
ctx: Context<Update>,
a: u64,
b: u64,
c: u64,
) -> anchor_lang::solana_program::entrypoint::ProgramResult {
let user_a = &mut ctx.accounts.user_a;
let user_b = &mut ctx.accounts.user_b;
let user_c = &mut ctx.accounts.user_c;

user_a.data = a;
user_b.data = b;
user_c.data = c;
Ok(())
}
}
Expand Down
48 changes: 24 additions & 24 deletions lints/duplicate-mutable-accounts/ui/insecure-2/src/lib.stderr
Original file line number Diff line number Diff line change
@@ -1,39 +1,39 @@
error: user_a and user_b have identical account types but do not have a key check constraint
--> $DIR/lib.rs:25:5
error: the expressions on line $DIR/lib.rs:15:27: 15:46 (#0) and $DIR/lib.rs:16:27: 16:46 (#0) have identical Account types, yet do not contain a proper key check.
--> $DIR/lib.rs:15:27
|
LL | user_a: Account<'info, User>,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
LL | let user_a = &mut ctx.accounts.user_a;
| ^^^^^^^^^^^^^^^^^^^
|
= note: `-D duplicate-mutable-accounts` implied by `-D warnings`
help: add an anchor key check constraint: #[account(constraint = user_a.key() != user_b.key())]
--> $DIR/lib.rs:26:5
help: add a key check to make sure the accounts have different keys, e.g., x.key() != y.key()
--> $DIR/lib.rs:16:27
|
LL | user_b: Account<'info, User>,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
LL | let user_b = &mut ctx.accounts.user_b;
| ^^^^^^^^^^^^^^^^^^^

error: user_a and user_c have identical account types but do not have a key check constraint
--> $DIR/lib.rs:25:5
error: the expressions on line $DIR/lib.rs:15:27: 15:46 (#0) and $DIR/lib.rs:17:27: 17:46 (#0) have identical Account types, yet do not contain a proper key check.
--> $DIR/lib.rs:15:27
|
LL | user_a: Account<'info, User>,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
LL | let user_a = &mut ctx.accounts.user_a;
| ^^^^^^^^^^^^^^^^^^^
|
help: add an anchor key check constraint: #[account(constraint = user_a.key() != user_c.key())]
--> $DIR/lib.rs:27:5
help: add a key check to make sure the accounts have different keys, e.g., x.key() != y.key()
--> $DIR/lib.rs:17:27
|
LL | user_c: Account<'info, User>,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
LL | let user_c = &mut ctx.accounts.user_c;
| ^^^^^^^^^^^^^^^^^^^

error: user_b and user_c have identical account types but do not have a key check constraint
--> $DIR/lib.rs:26:5
error: the expressions on line $DIR/lib.rs:16:27: 16:46 (#0) and $DIR/lib.rs:17:27: 17:46 (#0) have identical Account types, yet do not contain a proper key check.
--> $DIR/lib.rs:16:27
|
LL | user_b: Account<'info, User>,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
LL | let user_b = &mut ctx.accounts.user_b;
| ^^^^^^^^^^^^^^^^^^^
|
help: add an anchor key check constraint: #[account(constraint = user_b.key() != user_c.key())]
--> $DIR/lib.rs:27:5
help: add a key check to make sure the accounts have different keys, e.g., x.key() != y.key()
--> $DIR/lib.rs:17:27
|
LL | user_c: Account<'info, User>,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
LL | let user_c = &mut ctx.accounts.user_c;
| ^^^^^^^^^^^^^^^^^^^

error: aborting due to 3 previous errors

16 changes: 8 additions & 8 deletions lints/duplicate-mutable-accounts/ui/insecure/src/lib.stderr
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
error: user_a and user_b have identical account types but do not have a key check constraint
--> $DIR/lib.rs:25:5
error: the expressions on line $DIR/lib.rs:14:27: 14:46 (#0) and $DIR/lib.rs:15:27: 15:46 (#0) have identical Account types, yet do not contain a proper key check.
--> $DIR/lib.rs:14:27
|
LL | user_a: Account<'info, User>,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
LL | let user_a = &mut ctx.accounts.user_a;
| ^^^^^^^^^^^^^^^^^^^
|
= note: `-D duplicate-mutable-accounts` implied by `-D warnings`
help: add an anchor key check constraint: #[account(constraint = user_a.key() != user_b.key())]
--> $DIR/lib.rs:26:5
help: add a key check to make sure the accounts have different keys, e.g., x.key() != y.key()
--> $DIR/lib.rs:15:27
|
LL | user_b: Account<'info, User>,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
LL | let user_b = &mut ctx.accounts.user_b;
| ^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

0 comments on commit 660c76b

Please sign in to comment.