use-ink / ink

Polkadot's ink! to write smart contracts.
https://use.ink
Apache License 2.0
1.34k stars 426 forks source link

Linter: Dangereous `self.env().transferred_value()` pattern #2072

Open jubnzv opened 7 months ago

jubnzv commented 7 months ago

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:

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.