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: Dangereous self.env().transferred_value() pattern #2072

Open
jubnzv opened this issue Jan 25, 2024 · 0 comments
Open

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

jubnzv opened this issue Jan 25, 2024 · 0 comments
Labels
A-linter Issue regarding the ink! linter. B-feature-request A request for a new feature.

Comments

@jubnzv
Copy link
Member

jubnzv commented Jan 25, 2024

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 of message functions. We must check for and similar control-flow statements. And it would be great to check these inside closures for iter_mut loops.
  • Using self.env().transferred_value() in private functions. If some private function is accessing self.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.

@jubnzv jubnzv added B-feature-request A request for a new feature. A-linter Issue regarding the ink! linter. labels Jan 25, 2024
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

No branches or pull requests

1 participant