Add balance tree and output message effects to CEI analysis (FuelLabs#3648)

Author: anton-trunov

docs/book/src/blockchain-development/calling_contracts.md

@@ -141,7 +141,7 @@ warning
 29 | |
 30 | |         // Storage update _after_ external call
 31 | |         storage.balances.insert(sender, 0);
-   | |__________________________________________- Storage modification after external contract interaction in function or method "withdraw". Consider making all storage writes before calling another contract
+   | |__________________________________________- Storage write after external contract interaction in function or method "withdraw". Consider making all storage writes before calling another contract
 32 |       }
 33 |   }
    |
@@ -150,6 +150,12 @@ ____
 
 In case there is a storage read after an interaction, the CEI analyzer will issue a similar warning.
 
+In addition to storage reads and writes after an interaction, the CEI analyzer reports analogous warnings about:
+
+- balance tree updates, i.e. balance tree reads with subsequent writes, which may be produced by the `tr` and `tro` asm instructions or library functions using them under the hood;
+- balance trees reads with `bal` instruction;
+- changes to the output messages that can be produced by the `__smo` intrinsic function or the `smo` asm instruction.
+
 ## Differences from the EVM
 
 While the Fuel contract calling paradigm is similar to the EVM's (using an ABI, forwarding gas and data), it differs in _two_ key ways:

sway-core/src/semantic_analysis/cei_pattern_analysis.rs

@@ -5,6 +5,8 @@
 // for more detail on vulnerabilities in case of storage modification after interaction
 // and this [blog post](https://chainsecurity.com/curve-lp-oracle-manipulation-post-mortem)
 // for more information on storage reads after interaction.
+// We also treat the balance tree reads and writes separately,
+// as well as modifying output messages.
 
 use crate::{
     declaration_engine::{DeclarationEngine, DeclarationId},
@@ -15,6 +17,7 @@ use crate::{
     Engines,
 };
 use std::collections::HashSet;
+use std::fmt;
 use sway_error::warning::{CompileWarning, Warning};
 use sway_types::{Ident, Span, Spanned};
 
@@ -23,6 +26,38 @@ enum Effect {
     Interaction,  // interaction with external contracts
     StorageWrite, // storage modification
     StorageRead,  // storage read
+    // Note: there are no operations that only write to the balance tree
+    BalanceTreeRead,      // balance tree read operation
+    BalanceTreeReadWrite, // balance tree read and write operation
+    OutputMessage,        // operation creates a new `Output::Message`
+}
+
+impl fmt::Display for Effect {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        use Effect::*;
+        match self {
+            Interaction => write!(f, "Interaction"),
+            StorageWrite => write!(f, "Storage write"),
+            StorageRead => write!(f, "Storage read"),
+            BalanceTreeRead => write!(f, "Balance tree read"),
+            BalanceTreeReadWrite => write!(f, "Balance tree update"),
+            OutputMessage => write!(f, "Output message sent"),
+        }
+    }
+}
+
+impl Effect {
+    fn to_suggestion(&self) -> String {
+        use Effect::*;
+        String::from(match self {
+            Interaction => "making all interactions",
+            StorageWrite => "making all storage writes",
+            StorageRead => "making all storage reads",
+            BalanceTreeRead => "making all balance tree reads",
+            BalanceTreeReadWrite => "making all balance tree updates",
+            OutputMessage => "sending all output messages",
+        })
+    }
 }
 
 // The algorithm that searches for storage operations after interaction
@@ -31,7 +66,7 @@ enum Effect {
 // storage reads or writes.
 enum CEIAnalysisState {
     LookingForInteraction, // initial state of the automaton
-    LookingForStorageReadOrWrite,
+    LookingForEffect,
 }
 
 pub(crate) fn analyze_program(engines: Engines<'_>, prog: &ty::TyProgram) -> Vec<CompileWarning> {
@@ -100,7 +135,7 @@ fn impl_trait_methods<'a>(
 }
 
 // This is the main part of the analysis algorithm:
-// we are looking for state effects after contract interaction
+// we are looking for various effects after contract interaction
 fn analyze_code_block(
     engines: Engines<'_>,
     codeblock: &ty::TyCodeBlock,
@@ -117,11 +152,11 @@ fn analyze_code_block(
         match analysis_state {
             CEIAnalysisState::LookingForInteraction => {
                 if codeblock_entry_effects.contains(&Effect::Interaction) {
-                    analysis_state = CEIAnalysisState::LookingForStorageReadOrWrite;
+                    analysis_state = CEIAnalysisState::LookingForEffect;
                     interaction_span = ast_node.span.clone();
                 }
             }
-            CEIAnalysisState::LookingForStorageReadOrWrite => warn_after_interaction(
+            CEIAnalysisState::LookingForEffect => warn_after_interaction(
                 &codeblock_entry_effects,
                 &interaction_span,
                 &ast_node.span,
@@ -306,27 +341,17 @@ fn analyze_expression(
             set_union(cond_then_effs, cond_else_effs)
         }
         WhileLoop { condition, body } => {
-            // if the loop (condition + body) contains both interaction and storage operations
+            // if the loop (condition + body) contains both interaction and state effects
             // in _any_ order, we report CEI pattern violation
             let cond_effs = analyze_expression(engines, condition, block_name, warnings);
             let body_effs = analyze_code_block(engines, body, block_name, warnings);
             let res_effs = set_union(cond_effs, body_effs);
-            if res_effs.is_superset(&HashSet::from([Effect::Interaction, Effect::StorageRead])) {
-                warnings.push(CompileWarning {
-                    span: expr.span.clone(),
-                    warning_content: Warning::StorageReadAfterInteraction {
-                        block_name: block_name.clone(),
-                    },
-                });
-            };
-            if res_effs.is_superset(&HashSet::from([Effect::Interaction, Effect::StorageWrite])) {
-                warnings.push(CompileWarning {
-                    span: expr.span.clone(),
-                    warning_content: Warning::StorageWriteAfterInteraction {
-                        block_name: block_name.clone(),
-                    },
-                });
-            };
+            if res_effs.contains(&Effect::Interaction) {
+                // TODO: the span is not very precise, we can do better here, but this
+                // will need a bit of refactoring of the CEI analysis
+                let span = expr.span.clone();
+                warn_after_interaction(&res_effs, &span, &span, &block_name.clone(), warnings)
+            }
             res_effs
         }
         AsmExpression {
@@ -386,11 +411,11 @@ fn analyze_expressions(
         match analysis_state {
             CEIAnalysisState::LookingForInteraction => {
                 if expr_effs.contains(&Effect::Interaction) {
-                    analysis_state = CEIAnalysisState::LookingForStorageReadOrWrite;
+                    analysis_state = CEIAnalysisState::LookingForEffect;
                     interaction_span = expr.span.clone();
                 }
             }
-            CEIAnalysisState::LookingForStorageReadOrWrite => warn_after_interaction(
+            CEIAnalysisState::LookingForEffect => warn_after_interaction(
                 &expr_effs,
                 &interaction_span,
                 &expr.span,
@@ -418,11 +443,11 @@ fn analyze_asm_block(
         match analysis_state {
             CEIAnalysisState::LookingForInteraction => {
                 if asm_op_effs.contains(&Effect::Interaction) {
-                    analysis_state = CEIAnalysisState::LookingForStorageReadOrWrite;
+                    analysis_state = CEIAnalysisState::LookingForEffect;
                     interaction_span = asm_op.span.clone();
                 }
             }
-            CEIAnalysisState::LookingForStorageReadOrWrite => warn_after_interaction(
+            CEIAnalysisState::LookingForEffect => warn_after_interaction(
                 &asm_op_effs,
                 &interaction_span,
                 &asm_op.span,
@@ -442,18 +467,14 @@ fn warn_after_interaction(
     block_name: &Ident,
     warnings: &mut Vec<CompileWarning>,
 ) {
-    if ast_node_effects.contains(&Effect::StorageRead) {
-        warnings.push(CompileWarning {
-            span: Span::join(interaction_span.clone(), effect_span.clone()),
-            warning_content: Warning::StorageReadAfterInteraction {
-                block_name: block_name.clone(),
-            },
-        });
-    };
-    if ast_node_effects.contains(&Effect::StorageWrite) {
+    let interaction_singleton = HashSet::from([Effect::Interaction]);
+    let state_effects = ast_node_effects.difference(&interaction_singleton);
+    for eff in state_effects {
         warnings.push(CompileWarning {
             span: Span::join(interaction_span.clone(), effect_span.clone()),
-            warning_content: Warning::StorageWriteAfterInteraction {
+            warning_content: Warning::EffectAfterInteraction {
+                effect: eff.to_string(),
+                effect_in_suggestion: Effect::to_suggestion(eff),
                 block_name: block_name.clone(),
             },
         });
@@ -593,15 +614,19 @@ fn effects_of_intrinsic(intr: &sway_ast::Intrinsic) -> HashSet<Effect> {
     match intr {
         StateStoreWord | StateStoreQuad => HashSet::from([Effect::StorageWrite]),
         StateLoadWord | StateLoadQuad => HashSet::from([Effect::StorageRead]),
+        Smo => HashSet::from([Effect::OutputMessage]),
         Revert | IsReferenceType | SizeOfType | SizeOfVal | Eq | Gtf | AddrOf | Log | Add | Sub
-        | Mul | Div | PtrAdd | PtrSub | GetStorageKey | Smo => HashSet::new(),
+        | Mul | Div | PtrAdd | PtrSub | GetStorageKey => HashSet::new(),
     }
 }
 
 fn effects_of_asm_op(op: &AsmOp) -> HashSet<Effect> {
     match op.op_name.as_str().to_lowercase().as_str() {
         "sww" | "swwq" => HashSet::from([Effect::StorageWrite]),
-        "srw" | "srwq" | "bal" => HashSet::from([Effect::StorageRead]),
+        "srw" | "srwq" => HashSet::from([Effect::StorageRead]),
+        "tr" | "tro" => HashSet::from([Effect::BalanceTreeReadWrite]),
+        "bal" => HashSet::from([Effect::BalanceTreeRead]),
+        "smo" => HashSet::from([Effect::OutputMessage]),
         "call" => HashSet::from([Effect::Interaction]),
         // the rest of the assembly instructions are considered to not have effects
         _ => HashSet::new(),

sway-error/src/warning.rs

@@ -91,10 +91,9 @@ pub enum Warning {
     UnrecognizedAttribute {
         attrib_name: Ident,
     },
-    StorageWriteAfterInteraction {
-        block_name: Ident,
-    },
-    StorageReadAfterInteraction {
+    EffectAfterInteraction {
+        effect: String,
+        effect_in_suggestion: String,
         block_name: Ident,
     },
 }
@@ -222,10 +221,9 @@ impl fmt::Display for Warning {
             ),
             MatchExpressionUnreachableArm => write!(f, "This match arm is unreachable."),
             UnrecognizedAttribute {attrib_name} => write!(f, "Unknown attribute: \"{attrib_name}\"."),
-            StorageWriteAfterInteraction {block_name} => write!(f, "Storage modification after external contract interaction in function or method \"{block_name}\". \
-            Consider making all storage writes before calling another contract"),
-            StorageReadAfterInteraction {block_name} => write!(f, "Storage read after external contract interaction in function or method \"{block_name}\". \
-            Consider making all storage reads before calling another contract"),
+            EffectAfterInteraction {effect, effect_in_suggestion, block_name} =>
+                write!(f, "{effect} after external contract interaction in function or method \"{block_name}\". \
+                          Consider {effect_in_suggestion} before calling another contract"),
         }
     }
 }

test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation/test.toml

@@ -1,3 +1,3 @@
 category = "compile"
 
-# check: $()Storage modification after external contract interaction in function or method "deposit". Consider making all storage writes before calling another contract
+# check: $()Storage write after external contract interaction in function or method "deposit". Consider making all storage writes before calling another contract

test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block/test.toml

@@ -1,3 +1,3 @@
 category = "compile"
 
-# check: $()Storage modification after external contract interaction in function or method "deposit". Consider making all storage writes before calling another contract
+# check: $()Storage write after external contract interaction in function or method "deposit". Consider making all storage writes before calling another contract

test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block_bal/Forc.lock

@@ -0,0 +1,3 @@
+[[package]]
+name = 'cei_pattern_violation_in_asm_block_bal'
+source = 'member'

test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block_bal/Forc.toml

@@ -0,0 +1,6 @@
+[project]
+name = "cei_pattern_violation_in_asm_block_bal"
+authors = ["Fuel Labs <[email protected]>"]
+entry = "main.sw"
+license = "Apache-2.0"
+implicit-std = false

test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block_bal/src/main.sw

@@ -0,0 +1,18 @@
+contract;
+
+abi TestAbi {
+    fn deposit();
+}
+
+impl TestAbi for Contract {
+    fn deposit() {
+        let other_contract = abi(TestAbi, 0x3dba0a4455b598b7655a7fb430883d96c9527ef275b49739e7b0ad12f8280eae);
+
+        // interaction
+        other_contract.deposit();
+        // effect -- therefore violation of CEI where effect should go before interaction
+        asm(r1, r2: 0, r3: 0) {
+            bal r1 r2 r3;
+        }
+    }
+}

test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block_bal/test.toml

@@ -0,0 +1,3 @@
+category = "compile"
+
+# check: $()Balance tree read after external contract interaction in function or method "deposit". Consider making all balance tree reads before calling another contract

test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block_smo/Forc.lock

@@ -0,0 +1,3 @@
+[[package]]
+name = 'cei_pattern_violation_in_asm_block_smo'
+source = 'member'

test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block_smo/Forc.toml

@@ -0,0 +1,6 @@
+[project]
+name = "cei_pattern_violation_in_asm_block_smo"
+authors = ["Fuel Labs <[email protected]>"]
+entry = "main.sw"
+license = "Apache-2.0"
+implicit-std = false

test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block_smo/src/main.sw

@@ -0,0 +1,18 @@
+contract;
+
+abi TestAbi {
+    fn deposit();
+}
+
+impl TestAbi for Contract {
+    fn deposit() {
+        let other_contract = abi(TestAbi, 0x3dba0a4455b598b7655a7fb430883d96c9527ef275b49739e7b0ad12f8280eae);
+
+        // interaction
+        other_contract.deposit();
+        // effect -- therefore violation of CEI where effect should go before interaction
+        asm(r1: 0, r2: 0, r3: 0, r4: 0) {
+            smo r1 r2 r3 r4;
+        }
+    }
+}

test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block_smo/test.toml

@@ -0,0 +1,3 @@
+category = "compile"
+
+# check: $()Output message sent after external contract interaction in function or method "deposit". Consider sending all output messages before calling another contract

test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block_tr/Forc.lock

@@ -0,0 +1,13 @@
+[[package]]
+name = 'cei_pattern_violation_in_asm_block_tr'
+source = 'member'
+dependencies = ['std']
+
+[[package]]
+name = 'core'
+source = 'path+from-root-1F667E1725DAD261'
+
+[[package]]
+name = 'std'
+source = 'path+from-root-1F667E1725DAD261'
+dependencies = ['core']

test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block_tr/Forc.toml

@@ -0,0 +1,8 @@
+[project]
+name = "cei_pattern_violation_in_asm_block_tr"
+authors = ["Fuel Labs <[email protected]>"]
+entry = "main.sw"
+license = "Apache-2.0"
+
+[dependencies]
+std = { path = "../../../../../../../sway-lib-std" }

test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block_tr/src/main.sw

@@ -0,0 +1,23 @@
+contract;
+
+use std::token::force_transfer_to_contract;
+
+abi TestAbi {
+    fn deposit();
+}
+
+impl TestAbi for Contract {
+    fn deposit() {
+        let other_contract = abi(TestAbi, 0x3dba0a4455b598b7655a7fb430883d96c9527ef275b49739e7b0ad12f8280eae);
+
+        // interaction
+        other_contract.deposit();
+        // effect -- therefore violation of CEI where effect should go before interaction
+        let amount = 10;
+        let address = 0x0000000000000000000000000000000000000000000000000000000000000001;
+        let asset = ContractId::from(address);
+        let pool = ContractId::from(address);
+        // `force_transfer_to_contract` uses `tr` asm instruction
+        force_transfer_to_contract(amount, asset, pool);
+    }
+}

test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block_tr/test.toml

@@ -0,0 +1,3 @@
+category = "compile"
+
+# check: $()Balance tree update after external contract interaction in function or method "deposit". Consider making all balance tree updates before calling another contract

test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block_tro/Forc.lock

@@ -0,0 +1,13 @@
+[[package]]
+name = 'cei_pattern_violation_in_asm_block_tro'
+source = 'member'
+dependencies = ['std']
+
+[[package]]
+name = 'core'
+source = 'path+from-root-D9AC6CFA47996221'
+
+[[package]]
+name = 'std'
+source = 'path+from-root-D9AC6CFA47996221'
+dependencies = ['core']

test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block_tro/Forc.toml

@@ -0,0 +1,8 @@
+[project]
+name = "cei_pattern_violation_in_asm_block_tro"
+authors = ["Fuel Labs <[email protected]>"]
+entry = "main.sw"
+license = "Apache-2.0"
+
+[dependencies]
+std = { path = "../../../../../../../sway-lib-std" }