Merge pull request #164 from CoinFabrik/151-delegate-call-upgrades

151 delegate call upgrades

apps/cargo-scout-audit/tests/integration_test_configuration.yaml

@@ -75,3 +75,8 @@ detectors:
     examples:
       - vulnerable_path: "../../test-cases/assert-violation/assert-violation-1/vulnerable-example/Cargo.toml"
         remediated_path: "../../test-cases/assert-violation/assert-violation-1/remediated-example/Cargo.toml"
+  lazy-delegate:
+    warning_message: "Delegate call with non-lazy, non-mapping storage"
+    examples:
+      - vulnerable_path: "../../test-cases/lazy-delegate/lazy-delegate-1/vulnerable-example/Cargo.toml"
+        remediated_path: "../../test-cases/lazy-delegate/lazy-delegate-1/remediated-example/Cargo.toml"

detectors/lazy-delegate/.cargo/config.toml

@@ -0,0 +1,11 @@
+[target.aarch64-apple-darwin]
+linker = "dylint-link"
+
+[target.x86_64-apple-darwin]
+linker = "dylint-link"
+
+[target.x86_64-unknown-linux-gnu]
+linker = "dylint-link"
+
+[target.x86_64-pc-windows-msvc]
+linker = "dylint-link"

detectors/lazy-delegate/Cargo.toml

@@ -0,0 +1,19 @@
+[package]
+name = "delegate-call"
+version = "0.1.0"
+edition = "2021"
+
+[lib]
+crate-type = ["cdylib"]
+
+[dependencies]
+clippy_utils = { git = "https://github.com/rust-lang/rust-clippy", rev = "1480cea393d0cee195e59949eabdfbcf1230f7f9" }
+dylint_internal = "2.1.5"
+dylint_linting = "2.1.5"
+if_chain = "1.0.2"
+
+[dev-dependencies]
+dylint_testing = "2.1.5"
+
+[package.metadata.rust-analyzer]
+rustc_private = true

detectors/lazy-delegate/rust-toolchain

@@ -0,0 +1,3 @@
+[toolchain]
+channel = "nightly-2023-01-27"
+components = ["llvm-tools-preview", "rustc-dev"]

detectors/lazy-delegate/src/lib.rs

@@ -0,0 +1,131 @@
+#![feature(rustc_private)]
+#![warn(unused_extern_crates)]
+#![feature(let_chains)]
+extern crate rustc_ast;
+extern crate rustc_span;
+
+use clippy_utils::diagnostics::span_lint_and_help;
+use if_chain::if_chain;
+use rustc_ast::ast::GenericArgs;
+use rustc_ast::{
+    tokenstream::{TokenStream, TokenTree},
+    AngleBracketedArgs, AttrArgs, AttrKind, Item, ItemKind, TyKind,
+};
+use rustc_lint::{EarlyContext, EarlyLintPass};
+use rustc_span::Span;
+
+dylint_linting::impl_pre_expansion_lint! {
+    /// ### What it does
+    /// Checks for non-lazy storage when using delegate calls.
+    /// ### Why is this bad?
+    /// ink! has a bug that makes delegated calls not modify the storage of the caller.
+    /// ### Example
+    /// ```rust
+    ///    #[ink(storage)]
+    ///    pub struct Contract {
+    ///        admin: AccountId,
+    ///    }
+    ///     ```
+    /// Use instead:
+    ///```rust
+    ///    #[ink(storage)]
+    ///    pub struct DelegateCall {
+    ///        admin: Lazy<AccountId, ManualKey<12345>>,
+    ///    }
+    ///
+    /// ### More info
+    /// - https://github.com/paritytech/ink/issues/1825
+    /// - https://github.com/paritytech/ink/issues/1826
+    ///```
+
+    pub DELEGATE_CALL,
+    Warn,
+    "Use of delegate call with non-lazy, non-mapping storage won't modify the storage of the contract.",
+    DelegateCall::default()
+}
+
+#[derive(Default)]
+pub struct DelegateCall {
+    non_lazy_manual_storage_spans: Vec<Span>,
+    delegate_uses: Vec<Span>,
+}
+
+impl EarlyLintPass for DelegateCall {
+    fn check_item(&mut self, _: &EarlyContext<'_>, item: &Item) {
+        if is_storage_item(item)
+        && let ItemKind::Struct(strt, _) = &item.kind
+        {
+            for field in strt.fields() {
+                if let Some(_) = field.ident
+                && let TyKind::Path(_, path) = &field.ty.kind
+                && path.segments.len() == 1
+                && (path.segments[0].ident.name.to_string() == "Lazy".to_string() || path.segments[0].ident.name.to_string() == "Mapping".to_string())
+                && let Some(arg) = &path.segments[0].args
+                && let GenericArgs::AngleBracketed(AngleBracketedArgs { args, .. }) = arg.clone().into_inner()
+                && args.len() > 1 {} else {
+                    if !self.non_lazy_manual_storage_spans.contains(&item.span) {
+                        self.non_lazy_manual_storage_spans.push(item.span);
+                    }
+                }
+            }
+        }
+    }
+
+    fn check_ident(&mut self, cx: &EarlyContext<'_>, id: rustc_span::symbol::Ident) {
+        if id.name.to_string() == "delegate" {
+            dbg!(id.span);
+            self.delegate_uses.push(id.span);
+        }
+
+        if !self.delegate_uses.is_empty() && !self.non_lazy_manual_storage_spans.is_empty() {
+            span_lint_and_help(
+                cx,
+                DELEGATE_CALL,
+                id.span,
+                "Delegate call with non-lazy, non-mapping storage",
+                None,
+                "Use lazy storage with manual keys",
+            );
+
+            for span in &self.non_lazy_manual_storage_spans {
+                span_lint_and_help(
+                    cx,
+                    DELEGATE_CALL,
+                    *span,
+                    "Non-lazy non-mapping storage",
+                    None,
+                    "Use lazy storage with manual keys. \nMore info in https://github.com/paritytech/ink/issues/1826 and https://github.com/paritytech/ink/issues/1825",
+                );
+            }
+
+            self.delegate_uses.clear();
+        }
+    }
+}
+
+fn is_storage_item(item: &Item) -> bool {
+    item.attrs.iter().any(|attr| {
+        if_chain!(
+            if let AttrKind::Normal(normal) = &attr.kind;
+            if let AttrArgs::Delimited(delim_args) = &normal.item.args;
+            if is_storage_present(&delim_args.tokens);
+            then {
+                return true
+            }
+        );
+        return false;
+    })
+}
+
+fn is_storage_present(token_stream: &TokenStream) -> bool {
+    token_stream.trees().any(|tree| match tree {
+        TokenTree::Token(token, _) => {
+            if let Some(ident) = token.ident() {
+                return ident.0.name.to_ident_string().contains("storage");
+            } else {
+                false
+            }
+        }
+        TokenTree::Delimited(_, _, token_stream) => is_storage_present(token_stream),
+    })
+}

detectors/lazy-delegate/ui/main.rs

@@ -0,0 +1 @@
+fn main() {}

docs/docs/detectors/23-lazy-delegate.md

@@ -0,0 +1,36 @@
+# Lazy storage on delegate
+
+### What it does
+
+Checks for non-lazy storage when using delegate calls.
+
+### Why is this bad?
+
+ink! has a bug that makes delegated calls not modify the storage of the caller.
+
+#### More info
+
+- https://github.com/paritytech/ink/issues/1825
+- https://github.com/paritytech/ink/issues/1826
+
+### Example
+
+```rust
+    #[ink(storage)]
+    pub struct Contract {
+       admin: AccountId,
+    }
+```
+
+Use instead:
+
+```rust
+    #[ink(storage)]
+    pub struct Contract {
+        admin: Lazy<AccountId, ManualKey<12345>>,
+    }
+```
+
+### Implementation
+
+The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout/tree/main/detectors/lazy-delegate).

docs/docs/vulnerabilities/23-lazy-delegate.md

@@ -0,0 +1,94 @@
+# Lazy storage on delegate
+
+## Description
+
+- Vulnerability Severity: `High`
+- Detectors: [`lazy-delegate`](https://github.com/CoinFabrik/scout/tree/main/detectors/lazy-delegate)
+- Test Cases: [`lazy-delegate`](https://github.com/CoinFabrik/scout/tree/main/test-cases/lazy-delegate/)
+
+ink! has a bug that makes delegated calls not modify the storage of the caller, unless it's using `Lazy` with `ManualKey` or `Mapping`.
+
+## Exploit Scenario
+
+Consider the following `ink!` contract:
+
+```rust
+
+// With this storage
+    #[ink(storage)]
+    pub struct DelegateCall {
+        admin: AccountId,
+    }
+
+
+    #[ink(message)]
+    pub fn change_admin(
+        &mut self,
+        target: Hash,
+        new_admin: AccountId,
+    ) -> Result<AccountId, Error> {
+        if self.admin != self.env().caller() {
+            return Err(Error::NotAnAdmin);
+        }
+
+        let res = build_call::<DefaultEnvironment>()
+            .delegate(target)
+            .exec_input(
+                ExecutionInput::new(Selector::new(ink::selector_bytes!("change_admin")))
+                    .push_arg(new_admin),
+            )
+            .returns::<AccountId>()
+            .try_invoke()
+            .map_err(|_| Error::DelegateCallFailed)?
+            .map_err(|_| Error::DelegateCallFailed)?;
+
+        Ok(res)
+}
+```
+
+In this example, the function `change_admin` takes `new_admin` and sets it as the new admin. If this function is called, `self.admin` will be the same as before, even if it's setted to a new `AccountId`.
+
+The vulnerable code example can be found [`here`](https://github.com/CoinFabrik/scout/tree/main/test-cases/lazy-delegate/lazy-delegate-1/vulnerable-example).
+
+## Remediation
+
+To remediate this, we can use `Lazy` to store things.
+
+```rust
+    #[ink(storage)]
+    #[derive(Default)]
+    pub struct DelegateCall {
+        admin: Lazy<AccountId, ManualKey<123456>>,
+    }
+
+    #[ink(message, payable)]
+    pub fn change_admin(
+        &mut self,
+        target: Hash,
+        new_admin: AccountId,
+    ) -> Result<AccountId, Error> {
+        if self.admin.get().unwrap() != self.env().caller() {
+            return Err(Error::NotAnAdmin);
+        }
+
+        let res = build_call::<DefaultEnvironment>()
+            .delegate(target)
+            .exec_input(
+                ExecutionInput::new(Selector::new(ink::selector_bytes!("change_admin")))
+                    .push_arg(new_admin),
+            )
+            .returns::<AccountId>()
+            .try_invoke()
+            .map_err(|_| Error::DelegateCallFailed)?
+            .map_err(|_| Error::DelegateCallFailed)?;
+
+        Ok(res)
+    }
+```
+
+The remediated code example can be found [`here`](https://github.com/CoinFabrik/scout/tree/main/test-cases/lazy-delegate/lazy-delegate-1/remediated-example).
+
+## References
+
+- https://github.com/paritytech/ink/issues/1825
+- https://github.com/paritytech/ink/issues/1826

test-cases/lazy-delegate/lazy-delegate-1/exploiter-example/Cargo.toml

@@ -0,0 +1,32 @@
+[package]
+name = "delegate-call-exploiter"
+version = "0.1.0"
+edition = "2021"
+authors = ["[your_name] <[your_email]>"]
+
+[lib]
+path = "lib.rs"
+
+[features]
+default = ["std"]
+std = [
+    "ink/std",
+    "scale/std",
+    "scale-info/std",
+]
+ink-as-dependency = []
+e2e-tests = []
+
+[dependencies]
+ink = { version = "4.2.1", default-features = false }
+scale = { package = "parity-scale-codec", version = "3", default-features = false, features = ["derive"] }
+scale-info = { version = "2.6", default-features = false, features = ["derive"], optional = true }
+
+[dev-dependencies]
+ink_e2e = "4.2.1"
+
+[profile.release]
+overflow-checks = false
+
+[profile.dev]
+overflow-checks = false

test-cases/lazy-delegate/lazy-delegate-1/exploiter-example/lib.rs

@@ -0,0 +1,38 @@
+#![cfg_attr(not(feature = "std"), no_std, no_main)]
+
+#[ink::contract]
+pub mod delegate_call_exploiter {
+
+    use ink::storage::Mapping;
+
+    #[ink(storage)]
+    pub struct DelegateCallExploiter {
+        admin: AccountId,
+        balances: Mapping<AccountId, Balance>,
+    }
+
+    impl DelegateCallExploiter {
+        #[ink(constructor)]
+        pub fn new() -> Self {
+            Self {
+                admin: Self::env().caller(),
+                balances: Mapping::new(),
+            }
+        }
+
+        /// Returns the values to pay dependant on the saved percents
+        #[ink(message)]
+        pub fn change_admin(&mut self, new_admin: AccountId) -> AccountId {
+            self.admin = new_admin;
+            self.admin
+        }
+
+        /// Returns the codehash of the contract
+        #[ink(message)]
+        pub fn codehash(&self) -> Hash {
+            self.env()
+                .code_hash(&self.env().account_id())
+                .expect("Failed to get code hash")
+        }
+    }
+}