ink
ink copied to clipboard
Linter: Dangereous `self.env().transferred_value()` pattern
Using self.env().transferred_value()
inside a loop is a dangerous pattern, as it typically assumes that this value will be updated each iteration.
The Opyn vulnerability described in the ToB and PeckShield blogs could be reduced to the following minimal example in ink!:
#[ink::contract]
pub mod target {
#[ink(storage)]
pub struct Target {
balances: Mapping<AccountId, Balance>,
receivers: Mapping<i64, AccountId>,
}
#[ink(message)]
pub fn add_balance(&mut self) {
for i in 0..balances.len() {
self.balances[self.receivers[i]] += self.env().transferred_value();
}
}
}
In that example, add_balance
might be called for multiple receivers, increasing the balance for each of them, despite the fact that the contract has received only one msg.value
. The business logic of the contract might assume that balances
is used to pay funds to the users, therefore, this vulnerability might lead to draining the contract.
The suggested implementation should check the following:
- Accessing
self.env().transferred_value()
in loops ofmessage
functions. We must checkfor
and similar control-flow statements. And it would be great to check these inside closures foriter_mut
loops. - Using
self.env().transferred_value()
in private functions. If some private function is accessingself.env().transferred_value()
and is called in a loop of other function, it should be highlighted.
Ideally, we should also check local variables that might be assigned to self.env().transferred_value()
. It could be done using MIR dataflow framework, but it might unnecessarily complicate the implementation.