Merge pull request #271 from CoinFabrik/dont-use-instantiate-contract-v1

Dont-use-instantiate-contract-v1 detector and testcase

detectors/dont-use-instantiate-contract-v1/Cargo.toml

@@ -0,0 +1,19 @@
+[package]
+name = "dont-use-instantiate-contract-v1"
+version = "0.1.0"
+edition = "2021"
+
+[lib]
+crate-type = ["cdylib"]
+
+[dependencies]
+scout-audit-clippy-utils = { workspace = true }
+dylint_linting = { workspace = true }
+if_chain = { workspace = true }
+
+
+[dev-dependencies]
+dylint_testing = { workspace = true }
+
+[package.metadata.rust-analyzer]
+rustc_private = true

detectors/dont-use-instantiate-contract-v1/src/lib.rs

@@ -0,0 +1,74 @@
+#![feature(rustc_private)]
+#![feature(let_chains)]
+extern crate rustc_ast;
+extern crate rustc_hir;
+extern crate rustc_span;
+
+use rustc_hir::{
+    intravisit::{walk_expr, Visitor},
+    Expr, ExprKind,
+};
+use rustc_lint::LateLintPass;
+use rustc_span::{Span, Symbol};
+
+const LINT_MESSAGE: &str =
+    "This is a low level way to instantiate another smart contract, calling the legacy `instantiate_v1` host function.";
+
+dylint_linting::declare_late_lint! {
+    pub DONT_USE_INSTANTIATE_CONTRACT_V1,
+    Warn,
+    LINT_MESSAGE,
+    {
+        name: "Dont use instantiate_contract_v1",
+        long_message: LINT_MESSAGE,
+        severity: "Enhancement",
+        help: "https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/dont-use-instantiate-contract-v1",
+        vulnerability_class: "Best practices",
+    }
+}
+
+impl<'tcx> LateLintPass<'tcx> for DontUseInstantiateContractV1 {
+    fn check_fn(
+        &mut self,
+        cx: &rustc_lint::LateContext<'tcx>,
+        _: rustc_hir::intravisit::FnKind<'tcx>,
+        _: &'tcx rustc_hir::FnDecl<'tcx>,
+        body: &'tcx rustc_hir::Body<'tcx>,
+        _: rustc_span::Span,
+        _: rustc_hir::def_id::LocalDefId,
+    ) {
+        struct DontUseInstantiateContractV1Visitor {
+            has_instantiate_contract_v1_span: Vec<Option<Span>>,
+        }
+
+        impl<'tcx> Visitor<'tcx> for DontUseInstantiateContractV1Visitor {
+            fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
+                if let ExprKind::MethodCall(path_segment, _, _, _) = &expr.kind {
+                    if path_segment.ident.name == Symbol::intern("instantiate_contract_v1") {
+                        self.has_instantiate_contract_v1_span.push(Some(expr.span));
+                    }
+                }
+                walk_expr(self, expr);
+            }
+        }
+
+        let mut visitor = DontUseInstantiateContractV1Visitor {
+            has_instantiate_contract_v1_span: Vec::new(),
+        };
+
+        walk_expr(&mut visitor, body.value);
+
+        visitor.has_instantiate_contract_v1_span.iter().for_each(|span| {
+            if let Some(span) = span {
+                scout_audit_clippy_utils::diagnostics::span_lint_and_help(
+                    cx,
+                    DONT_USE_INSTANTIATE_CONTRACT_V1,
+                    *span,
+                    LINT_MESSAGE,
+                    None,
+                    "Prefer to use methods on a `ContractRef` or the `CreateBuilder` through `build_create` instead",
+                );
+            }
+        });
+    }
+}

docs/docs/detectors/33-dont-use-instantiate-contract-v1.md

@@ -0,0 +1,19 @@
+# Don't use instantiate_contract_v1
+
+### What it does
+
+Checks that method `instantiate_contract_v1` is not used in the contract.
+
+### Why is this bad?
+
+This is a low level way to instantiate another smart contract, calling the legacy `instantiate_v1` host function.
+
+Prefer to use methods on a `ContractRef` or the `CreateBuilder` through `build_create` instead.
+
+#### More info
+
+- https://docs.rs/ink_env/5.0.0/ink_env/fn.instantiate_contract_v1.html
+
+### Implementation
+
+The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout/tree/main/detectors/dont-use-instantiate-contract-v1).

docs/docs/vulnerabilities/33-dont-use-instantiate-contract-v1.md

@@ -0,0 +1,98 @@
+# Don't use instantiate_contract_v1
+
+## Description 
+- Vulnerability Category: `Best Practices`
+- Vulnerability Severity: `Enhancement`
+- Detectors: [`dont-use-instantiate-contract-v1`](https://github.com/CoinFabrik/scout/tree/main/detectors)
+- Test Cases: [`dont-use-instantiate-contract-v1`](https://github.com/CoinFabrik/scout/tree/main/test-cases)
+
+Avoid using `instantiate_contract_v1` as it is a low level way to evaluate another smart contract. If needed, use `instantiate_contract` instead. Also, use methods on a `ContractRef` or the `CreateBuilder` through `build_create` if possible.
+
+
+## Exploit Scenario
+
+Consider the following example
+
+```rust
+    impl MyContract {
+        #[ink(constructor)]
+        pub fn new() -> Self {
+            Self {}
+        }
+
+        #[ink(message)]
+        pub fn instantiate_contract(&self) -> MyContractRef {
+            let create_params = build_create::<OtherContractRef>()
+                .instantiate_v1()
+                .code_hash(Hash::from([0x42; 32]))
+                .gas_limit(500_000_000)
+                .endowment(25)
+                .exec_input(
+                    ExecutionInput::new(Selector::new(ink::selector_bytes!("new")))
+                        .push_arg(42)
+                        .push_arg(true)
+                        .push_arg(&[0x10u8; 32]),
+                )
+                .salt_bytes(&[0xCA, 0xFE, 0xBA, 0xBE])
+                .returns::<OtherContractRef>()
+                .params();
+            self.env()
+                .instantiate_contract_v1(&create_params)
+                .unwrap_or_else(|error| {
+                    panic!(
+                        "Received an error from the Contracts pallet while instantiating: {:?}",
+                        error
+                    )
+                })
+                .unwrap_or_else(|error| {
+                    panic!("Received a `LangError` while instatiating: {:?}", error)
+                })
+        }
+    }
+```
+
+## Remediation
+
+```rust
+    impl MyContract {
+        #[ink(constructor)]
+        pub fn new() -> Self {
+            Self {}
+        }
+
+        #[ink(message)]
+        pub fn instantiate_contract(&self) -> MyContractRef {
+            let create_params = build_create::<OtherContractRef>()
+                .code_hash(Hash::from([0x42; 32]))
+                .ref_time_limit(500_000_000)
+                .proof_size_limit(100_000)
+                .storage_deposit_limit(500_000_000_000)
+                .endowment(25)
+                .exec_input(
+                    ExecutionInput::new(Selector::new(ink::selector_bytes!("new")))
+                        .push_arg(42)
+                        .push_arg(true)
+                        .push_arg(&[0x10u8; 32]),
+                )
+                .salt_bytes(&[0xCA, 0xFE, 0xBA, 0xBE])
+                .returns::<OtherContractRef>()
+                .params();
+            self.env()
+                .instantiate_contract(&create_params)
+                .unwrap_or_else(|error| {
+                    panic!(
+                        "Received an error from the Contracts pallet while instantiating: {:?}",
+                        error
+                    )
+                })
+                .unwrap_or_else(|error| {
+                    panic!("Received a `LangError` while instatiating: {:?}", error)
+                })
+        }
+    }
+```
+
+
+## References
+
+- https://docs.rs/ink_env/5.0.0/ink_env/fn.instantiate_contract_v1.html

test-cases/dont-use-instantiate-contract-v1/Cargo.toml

@@ -0,0 +1,24 @@
+[workspace]
+exclude = [".cargo", "target"]
+members = ["dont-use-instantiate-contract-v1-*/*"]
+resolver = "2"
+
+[workspace.dependencies]
+ink = { version = "5.0.0", 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"]}
+ink_e2e = { version = "=5.0.0" }
+
+[profile.release]
+codegen-units = 1
+debug = 0
+debug-assertions = false
+lto = true
+opt-level = "z"
+overflow-checks = false
+panic = "abort"
+strip = "symbols"
+
+[profile.release-with-logs]
+debug-assertions = true
+inherits = "release"

test-cases/dont-use-instantiate-contract-v1/dont-use-instantiate-contract-v1-1/remediated-example/Cargo.toml

@@ -0,0 +1,28 @@
+[package]
+name = "dont-use-instantiate-contract-v1-1-remediated"
+version = "0.1.0"
+edition = "2021"
+authors = ["[your_name] <[your_email]>"]
+
+[lib]
+path = "src/lib.rs"
+
+[features]
+default = ["std"]
+std = [
+    "ink/std",
+    "scale/std",
+    "scale-info/std",
+]
+ink-as-dependency = []
+e2e-tests = []
+
+
+[dependencies]
+ink = { workspace = true }
+scale = { workspace = true }
+scale-info = { workspace = true }
+
+
+[dev-dependencies]
+ink_e2e = { workspace = true }

test-cases/dont-use-instantiate-contract-v1/dont-use-instantiate-contract-v1-1/remediated-example/src/lib.rs

@@ -0,0 +1,56 @@
+#![cfg_attr(not(feature = "std"), no_std, no_main)]
+// This is an example from ink docs.
+#[ink::contract]
+pub mod my_contract {
+    // In order for this to actually work with another contract we'd need a way
+    // to turn the `ink-as-dependency` crate feature on in doctests, which we
+    // can't do.
+    //
+    // Instead we use our own contract's `Ref`, which is fine for this example
+    // (just need something that implements the `ContractRef` trait).
+    pub mod other_contract {
+        pub use super::MyContractRef as OtherContractRef;
+    }
+    use ink::env::call::{build_create, ExecutionInput, Selector};
+    use other_contract::OtherContractRef;
+
+    #[ink(storage)]
+    pub struct MyContract {}
+
+    impl MyContract {
+        #[ink(constructor)]
+        pub fn new() -> Self {
+            Self {}
+        }
+
+        #[ink(message)]
+        pub fn instantiate_contract(&self) -> MyContractRef {
+            let create_params = build_create::<OtherContractRef>()
+                .code_hash(Hash::from([0x42; 32]))
+                .ref_time_limit(500_000_000)
+                .proof_size_limit(100_000)
+                .storage_deposit_limit(500_000_000_000)
+                .endowment(25)
+                .exec_input(
+                    ExecutionInput::new(Selector::new(ink::selector_bytes!("new")))
+                        .push_arg(42)
+                        .push_arg(true)
+                        .push_arg(&[0x10u8; 32]),
+                )
+                .salt_bytes(&[0xCA, 0xFE, 0xBA, 0xBE])
+                .returns::<OtherContractRef>()
+                .params();
+            self.env()
+                .instantiate_contract(&create_params)
+                .unwrap_or_else(|error| {
+                    panic!(
+                        "Received an error from the Contracts pallet while instantiating: {:?}",
+                        error
+                    )
+                })
+                .unwrap_or_else(|error| {
+                    panic!("Received a `LangError` while instatiating: {:?}", error)
+                })
+        }
+    }
+}

test-cases/dont-use-instantiate-contract-v1/dont-use-instantiate-contract-v1-1/vulnerable-example/Cargo.toml

@@ -0,0 +1,28 @@
+[package]
+name = "dont-use-instantiate-contract-v1-1-vulnerable"
+version = "0.1.0"
+edition = "2021"
+authors = ["[your_name] <[your_email]>"]
+
+[lib]
+path = "src/lib.rs"
+
+[features]
+default = ["std"]
+std = [
+    "ink/std",
+    "scale/std",
+    "scale-info/std",
+]
+ink-as-dependency = []
+e2e-tests = []
+
+
+[dependencies]
+ink = { workspace = true }
+scale = { workspace = true }
+scale-info = { workspace = true }
+
+
+[dev-dependencies]
+ink_e2e = { workspace = true }