Skip to content
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

Linter: Warn when conditional behavior depends on contract's balance #1811

Closed
cmichi opened this issue Jun 16, 2023 · 2 comments · Fixed by #1914
Closed

Linter: Warn when conditional behavior depends on contract's balance #1811

cmichi opened this issue Jun 16, 2023 · 2 comments · Fixed by #1914
Assignees
Labels
A-linter Issue regarding the ink! linter. B-feature-request A request for a new feature.

Comments

@cmichi
Copy link
Collaborator

cmichi commented Jun 16, 2023

We should add a lint that checks if the contract's balance (env().balance()) is used in a comparison. Such usage is an anti-pattern for smart contract development, as there's no way of preventing anyone from sending value to the contract, thus possibly invalidating the check.

Possibly it makes sense to only lint for equality operations ==.

@cmichi cmichi added B-feature-request A request for a new feature. A-linter Issue regarding the ink! linter. labels Jun 16, 2023
@SkymanOne
Copy link
Contributor

It makes sense to use it with any comparison operations.

jubnzv added a commit to jubnzv/ink that referenced this issue Sep 6, 2023
The current implementation works only with intraprocedural MIR and does
not support taint propagation across function calls.

Closes use-ink#1811
@jubnzv
Copy link
Member

jubnzv commented Sep 15, 2023

@SkymanOne Not really. The point is that the conditions with strict balance equality could be used to perform an attack, when the attacker forcefully sends some funds using self.env().terminate_contract(target).

For example, in the following contract:

#[ink::contract]
pub mod target {
  // ...
  #[ink(message)]
  pub fn do_something(&mut self) {
      if self.env().balance() != 100 { // Bad: Strict balance equality
          // ... some logic
      }
  }
}

There is a condition with strict balance equality, which could be manipulated by the attacker using the following contract:

#[ink::contract]
pub mod attacker {
  // ...
  #[ink(message)]
  pub fn attack(&mut self, target: &AccountId) {
      self.env().terminate_contract(target);
  }
}

Therefore, introducing non-strict equality comparison with the balance (>, <, >=, <=) will fix that problem in the most cases. For example, if a balance is used as some threshold, it will be safer to use < instead of strict equality.

cmichi pushed a commit that referenced this issue Oct 25, 2023
* feat(linter): strict balance equality lint

The current implementation works only with intraprocedural MIR and does
not support taint propagation across function calls.

Closes #1811

* feat(lint): Handle temporary values resulted after Rvalue::Use

* fix(lint): spans to emit diagnostics

Previously, diagnostics did not work, since `terminator.span` is
resulted after macro expansion

* feat(tests): more tests

* feat(lint): Manually traverse functions in user-defined code

This is required to implement interprocedural analysis

* feat(lint): interprocedural analysis that finds tainted returns

* fix(lint): recursive calls in interprocedural analysis

* fix(lint): false negative on `CheckedBinaryOp`

* feat(lint): propagation through references

* feat(lint): Propagate tainted values through `&mut` arguments

* chore(lint): docstring, comments

* feat(lint): handle comparison of references in functions

* chore(tests): comments

* feat(lint+tests): updated `pass` test, fixed binop conditions

* feat(tests): test for lint suppressions

* chore(tests): fmt

* chore(tests): fmt

* chore: Add changelog entry

* chore(lint): Reuse utility functions introduced in #1932

* chore: Fix changelog

* chore: Fix comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linter Issue regarding the ink! linter. B-feature-request A request for a new feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants