-
Notifications
You must be signed in to change notification settings - Fork 430
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
Comments
It makes sense to use it with any comparison operations. |
The current implementation works only with intraprocedural MIR and does not support taint propagation across function calls. Closes use-ink#1811
@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 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 ( |
* 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
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
==
.The text was updated successfully, but these errors were encountered: