From 57b6d08cf70968ddea05ce89815f8431a8ab9923 Mon Sep 17 00:00:00 2001 From: Anton Trunov Date: Thu, 10 Nov 2022 21:40:24 +0400 Subject: [PATCH] Add CEI pattern static analysis (#3168) CEI stands for "Checks, Effects, Interactions". We check no storage reads/writes (effects) occur after calling external contracts (interaction). See this [blog post](https://fravoll.github.io/solidity-patterns/checks_effects_interactions.html) and [this blogpost](https://chainsecurity.com/curve-lp-oracle-manipulation-post-mortem) for more detail. TODOs: - [x] analyze libraries, scripts, and predicates (only contracts are analyzed at the moment) - [x] process assembly blocks - [x] what is the effects of the `__revert` intrinsic - [x] Treat nested code blocks - [x] more tests * [x] Tests using `storage.var = ...` * [x] Tests using `asm` blocks directly * [x] Tests using complex control flow * [x] Tests having multiple contract calls or multiple storage writes in various scenarios * [ ] Tests with nested code blocks (there is a very simple test, this needs more testing in a separate PR) - [x] check for storage reads after interaction * [x] Storage read intrinsics * [x] `StorageAccess` expressions * [x] `asm` blocks using `srw` or `srwq` * [x] `asm` blocks using [`bal`](https://github.com/FuelLabs/fuel-specs/blob/master/specs/vm/instruction_set.md#bal-balance-of-contract-id) because `bal` actually reads from storage - [x] ~~CLI option or attribute to control the warnings~~ (an internal discussion showed that we cannot do this at the moment, I guess this should be handled as a separate issue) - [ ] open an issue about unification of the currently existing storage annotation checks and effects inference in this PR close #10 Co-authored-by: Mohammad Fawaz --- sway-core/src/lib.rs | 5 + sway-core/src/semantic_analysis.rs | 1 + .../semantic_analysis/cei_pattern_analysis.rs | 600 ++++++++++++++++++ sway-error/src/warning.rs | 10 + .../cei_pattern_violation/Forc.lock | 13 + .../cei_pattern_violation/Forc.toml | 8 + .../cei_pattern_violation/src/main.sw | 21 + .../cei_pattern_violation/test.toml | 3 + .../Forc.lock | 3 + .../Forc.toml | 6 + .../src/main.sw | 22 + .../test.toml | 3 + .../Forc.lock | 3 + .../Forc.toml | 6 + .../src/main.sw | 23 + .../test.toml | 3 + .../Forc.lock | 13 + .../Forc.toml | 8 + .../src/main.sw | 25 + .../test.toml | 3 + .../Forc.lock | 13 + .../Forc.toml | 8 + .../src/main.sw | 24 + .../test.toml | 3 + .../Forc.lock | 13 + .../Forc.toml | 8 + .../src/main.sw | 26 + .../test.toml | 3 + .../Forc.lock | 13 + .../Forc.toml | 8 + .../src/main.sw | 125 ++++ .../test.toml | 9 + .../Forc.lock | 3 + .../Forc.toml | 6 + .../src/main.sw | 22 + .../test.toml | 3 + .../Forc.lock | 3 + .../Forc.toml | 6 + .../src/main.sw | 22 + .../test.toml | 3 + 40 files changed, 1100 insertions(+) create mode 100644 sway-core/src/semantic_analysis/cei_pattern_analysis.rs create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation/Forc.lock create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation/Forc.toml create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation/src/main.sw create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation/test.toml create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block/Forc.lock create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block/Forc.toml create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block/src/main.sw create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block/test.toml create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block_read/Forc.lock create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block_read/Forc.toml create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block_read/src/main.sw create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block_read/test.toml create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_codeblocks_other_than_in_functions/Forc.lock create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_codeblocks_other_than_in_functions/Forc.toml create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_codeblocks_other_than_in_functions/src/main.sw create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_codeblocks_other_than_in_functions/test.toml create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_if_statement/Forc.lock create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_if_statement/Forc.toml create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_if_statement/src/main.sw create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_if_statement/test.toml create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_standalone_function/Forc.lock create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_standalone_function/Forc.toml create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_standalone_function/src/main.sw create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_standalone_function/test.toml create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_more_complex_logic/Forc.lock create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_more_complex_logic/Forc.toml create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_more_complex_logic/src/main.sw create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_more_complex_logic/test.toml create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_storage_var_read/Forc.lock create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_storage_var_read/Forc.toml create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_storage_var_read/src/main.sw create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_storage_var_read/test.toml create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_storage_var_update/Forc.lock create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_storage_var_update/Forc.toml create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_storage_var_update/src/main.sw create mode 100644 test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_storage_var_update/test.toml diff --git a/sway-core/src/lib.rs b/sway-core/src/lib.rs index 3d73f448469..a53c2d1e9bf 100644 --- a/sway-core/src/lib.rs +++ b/sway-core/src/lib.rs @@ -217,6 +217,11 @@ pub fn parsed_to_ast( errors.push(e); } + // CEI pattern analysis + let cei_analysis_warnings = + semantic_analysis::cei_pattern_analysis::analyze_program(&typed_program); + warnings.extend(cei_analysis_warnings); + // Check that all storage initializers can be evaluated at compile time. let typed_wiss_res = typed_program.get_typed_program_with_initialized_storage_slots( &mut ctx, diff --git a/sway-core/src/semantic_analysis.rs b/sway-core/src/semantic_analysis.rs index 65dd9acc926..c85514f8cf8 100644 --- a/sway-core/src/semantic_analysis.rs +++ b/sway-core/src/semantic_analysis.rs @@ -1,5 +1,6 @@ //! Type checking for Sway. pub mod ast_node; +pub(crate) mod cei_pattern_analysis; mod module; pub mod namespace; mod node_dependencies; diff --git a/sway-core/src/semantic_analysis/cei_pattern_analysis.rs b/sway-core/src/semantic_analysis/cei_pattern_analysis.rs new file mode 100644 index 00000000000..7d2df1a529f --- /dev/null +++ b/sway-core/src/semantic_analysis/cei_pattern_analysis.rs @@ -0,0 +1,600 @@ +// CEI stands for "Checks, Effects, Interactions". We check that no storage writes +// (effects) occur after calling external contracts (interaction) and issue warnings +// if it's the case. +// See this [blog post](https://fravoll.github.io/solidity-patterns/checks_effects_interactions.html) +// 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. + +use crate::{ + declaration_engine::DeclarationId, + language::{ + ty::{self, TyFunctionDeclaration}, + AsmOp, + }, +}; +use std::collections::HashSet; +use sway_error::warning::{CompileWarning, Warning}; +use sway_types::{Ident, Span}; + +#[derive(PartialEq, Eq, Hash, Clone)] +enum Effect { + Interaction, // interaction with external contracts + StorageWrite, // storage modification + StorageRead, // storage read +} + +// The algorithm that searches for storage operations after interaction +// is organized as an automaton. +// After an interaction is found in a code block, we keep looking either for +// storage reads or writes. +enum CEIAnalysisState { + LookingForInteraction, // initial state of the automaton + LookingForStorageReadOrWrite, +} + +pub(crate) fn analyze_program(prog: &ty::TyProgram) -> Vec { + match &prog.kind { + // Libraries, scripts, or predicates can't access storage + // so we don't analyze these + ty::TyProgramKind::Library { .. } + | ty::TyProgramKind::Script { .. } + | ty::TyProgramKind::Predicate { .. } => vec![], + ty::TyProgramKind::Contract { .. } => analyze_contract(&prog.root.all_nodes), + } +} + +fn analyze_contract(ast_nodes: &[ty::TyAstNode]) -> Vec { + let mut warnings: Vec = vec![]; + for fn_decl in contract_entry_points(ast_nodes) { + analyze_code_block(&fn_decl.body, &fn_decl.name, &mut warnings); + } + warnings +} + +// standalone functions and methods +fn contract_entry_points(ast_nodes: &[ty::TyAstNode]) -> Vec { + use crate::ty::TyAstNodeContent::Declaration; + ast_nodes + .iter() + .flat_map(|ast_node| match &ast_node.content { + Declaration(ty::TyDeclaration::FunctionDeclaration(decl_id)) => { + decl_id_to_fn_decls(decl_id, &ast_node.span) + } + Declaration(ty::TyDeclaration::ImplTrait(decl_id)) => { + impl_trait_methods(decl_id, &ast_node.span) + } + _ => vec![], + }) + .collect() +} + +fn decl_id_to_fn_decls(decl_id: &DeclarationId, span: &Span) -> Vec { + use crate::declaration_engine::de_get_function; + de_get_function(decl_id.clone(), span).map_or(vec![], |fn_decl| vec![fn_decl]) +} + +fn impl_trait_methods<'a>( + impl_trait_decl_id: &'a DeclarationId, + span: &'a Span, +) -> Vec { + use crate::declaration_engine::de_get_impl_trait; + match de_get_impl_trait(impl_trait_decl_id.clone(), span) { + Ok(impl_trait) => impl_trait + .methods + .iter() + .flat_map(|fn_decl| decl_id_to_fn_decls(fn_decl, span)) + .collect(), + Err(_) => vec![], + } +} + +// This is the main part of the analysis algorithm: +// we are looking for state effects after contract interaction +fn analyze_code_block( + codeblock: &ty::TyCodeBlock, + block_name: &Ident, + warnings: &mut Vec, +) -> HashSet { + let mut interaction_span: Span = Span::dummy(); + let mut codeblock_effects = HashSet::new(); + let mut analysis_state: CEIAnalysisState = CEIAnalysisState::LookingForInteraction; + + for ast_node in &codeblock.contents { + let codeblock_entry_effects = analyze_code_block_entry(ast_node, block_name, warnings); + match analysis_state { + CEIAnalysisState::LookingForInteraction => { + if codeblock_entry_effects.contains(&Effect::Interaction) { + analysis_state = CEIAnalysisState::LookingForStorageReadOrWrite; + interaction_span = ast_node.span.clone(); + } + } + CEIAnalysisState::LookingForStorageReadOrWrite => warn_after_interaction( + &codeblock_entry_effects, + &interaction_span, + &ast_node.span, + block_name, + warnings, + ), + }; + codeblock_effects.extend(codeblock_entry_effects) + } + codeblock_effects +} + +fn analyze_code_block_entry( + entry: &ty::TyAstNode, + block_name: &Ident, + warnings: &mut Vec, +) -> HashSet { + match &entry.content { + ty::TyAstNodeContent::Declaration(decl) => { + analyze_codeblock_decl(decl, block_name, warnings) + } + ty::TyAstNodeContent::Expression(expr) + | ty::TyAstNodeContent::ImplicitReturnExpression(expr) => { + analyze_expression(expr, block_name, warnings) + } + ty::TyAstNodeContent::SideEffect => HashSet::new(), + } +} + +fn analyze_codeblock_decl( + decl: &ty::TyDeclaration, + block_name: &Ident, + warnings: &mut Vec, +) -> HashSet { + // Declarations (except variable declarations) are not allowed in a codeblock + use crate::ty::TyDeclaration::*; + match decl { + VariableDeclaration(var_decl) => analyze_expression(&var_decl.body, block_name, warnings), + _ => HashSet::new(), + } +} + +fn analyze_expression( + expr: &ty::TyExpression, + block_name: &Ident, + warnings: &mut Vec, +) -> HashSet { + use crate::ty::TyExpressionVariant::*; + match &expr.expression { + // base cases: no warnings can be emitted + Literal(_) + | VariableExpression { .. } + | FunctionParameter + | StorageAccess(_) + | Break + | Continue + | AbiName(_) => effects_of_expression(expr), + Reassignment(reassgn) => analyze_expression(&reassgn.rhs, block_name, warnings), + StorageReassignment(reassgn) => { + let storage_effs = HashSet::from([Effect::StorageWrite]); + let rhs_effs = analyze_expression(&reassgn.rhs, block_name, warnings); + if rhs_effs.contains(&Effect::Interaction) { + warn_after_interaction( + &storage_effs, + &reassgn.rhs.span, + &expr.span, + block_name, + warnings, + ) + }; + set_union(storage_effs, rhs_effs) + } + CodeBlock(codeblock) => analyze_code_block(codeblock, block_name, warnings), + LazyOperator { + lhs: left, + rhs: right, + .. + } + | ArrayIndex { + prefix: left, + index: right, + } => analyze_two_expressions(left, right, block_name, warnings), + FunctionApplication { + arguments, + function_decl_id, + selector, + .. + } => { + use crate::declaration_engine::de_get_function; + let func = de_get_function(function_decl_id.clone(), &expr.span).unwrap(); + // we don't need to run full analysis on the function body as it will be covered + // as a separate step of the whole contract analysis + // we just need function's effects at this point + let fn_effs = effects_of_codeblock(&func.body); + + // assuming left-to-right arguments evaluation + // we run CEI violation analysis as if the arguments form a code block + let args_effs = analyze_expressions( + arguments.iter().map(|(_, e)| e).collect(), + block_name, + warnings, + ); + + if args_effs.contains(&Effect::Interaction) { + // TODO: interaction span has to be more precise and point to an argument which performs interaction + warn_after_interaction(&fn_effs, &expr.span, &func.span, block_name, warnings) + } + + let mut result_effs = set_union(fn_effs, args_effs); + if selector.is_some() { + // external contract call (a.k.a. interaction) + result_effs.extend(HashSet::from([Effect::Interaction])) + }; + result_effs + } + IntrinsicFunction(intrinsic) => { + let intr_effs = effects_of_intrinsic(&intrinsic.kind); + // assuming left-to-right arguments evaluation + let args_effs = + analyze_expressions(intrinsic.arguments.iter().collect(), block_name, warnings); + if args_effs.contains(&Effect::Interaction) { + // TODO: interaction span has to be more precise and point to an argument which performs interaction + warn_after_interaction(&intr_effs, &expr.span, &expr.span, block_name, warnings) + } + set_union(intr_effs, args_effs) + } + Tuple { fields: exprs } | Array { contents: exprs } => { + // assuming left-to-right fields/elements evaluation + analyze_expressions(exprs.iter().collect(), block_name, warnings) + } + StructExpression { fields, .. } => { + // assuming left-to-right fields evaluation + analyze_expressions( + fields.iter().map(|e| &e.value).collect(), + block_name, + warnings, + ) + } + StructFieldAccess { prefix: expr, .. } + | TupleElemAccess { prefix: expr, .. } + | Return(expr) + | EnumTag { exp: expr } + | UnsafeDowncast { exp: expr, .. } + | AbiCast { address: expr, .. } => analyze_expression(expr, block_name, warnings), + EnumInstantiation { contents, .. } => match contents { + Some(expr) => analyze_expression(expr, block_name, warnings), + None => HashSet::new(), + }, + IfExp { + condition, + then, + r#else, + } => { + let cond_then_effs = analyze_two_expressions(condition, then, block_name, warnings); + let cond_else_effs = match r#else { + Some(else_exp) => { + analyze_two_expressions(condition, else_exp, block_name, warnings) + } + None => HashSet::new(), + }; + set_union(cond_then_effs, cond_else_effs) + } + WhileLoop { condition, body } => { + let cond_effs = analyze_expression(condition, block_name, warnings); + let body_effs = analyze_code_block(body, block_name, warnings); + if cond_effs.contains(&Effect::Interaction) { + warn_after_interaction( + &body_effs, + &condition.span, + &expr.span, + block_name, + warnings, + ); + }; + if body_effs.contains(&Effect::Interaction) { + warn_after_interaction( + &cond_effs, + &expr.span, + &condition.span, + block_name, + warnings, + ); + }; + set_union(cond_effs, body_effs) + } + AsmExpression { + registers, body, .. + } => { + let init_exprs = registers + .iter() + .filter_map(|rdecl| rdecl.initializer.as_ref()) + .collect(); + let init_effs = analyze_expressions(init_exprs, block_name, warnings); + let asmblock_effs = analyze_asm_block(body, block_name, warnings); + if init_effs.contains(&Effect::Interaction) { + // TODO: improve locations accuracy + warn_after_interaction(&asmblock_effs, &expr.span, &expr.span, block_name, warnings) + } + set_union(init_effs, asmblock_effs) + } + } +} + +fn analyze_two_expressions( + first: &ty::TyExpression, + second: &ty::TyExpression, + block_name: &Ident, + warnings: &mut Vec, +) -> HashSet { + let first_effs = analyze_expression(first, block_name, warnings); + let second_effs = analyze_expression(second, block_name, warnings); + if first_effs.contains(&Effect::Interaction) { + warn_after_interaction( + &second_effs, + &first.span, + &second.span, + block_name, + warnings, + ) + } + set_union(first_effs, second_effs) +} + +// Analyze a sequence of expressions +// TODO: analyze_expressions, analyze_codeblock and analyze_asm_block (see below) are very similar in structure +// looks like the algorithm implementation should be generalized +fn analyze_expressions( + expressions: Vec<&ty::TyExpression>, + block_name: &Ident, + warnings: &mut Vec, +) -> HashSet { + let mut interaction_span: Span = Span::dummy(); + let mut accumulated_effects = HashSet::new(); + let mut analysis_state: CEIAnalysisState = CEIAnalysisState::LookingForInteraction; + + for expr in expressions { + let expr_effs = analyze_expression(expr, block_name, warnings); + match analysis_state { + CEIAnalysisState::LookingForInteraction => { + if expr_effs.contains(&Effect::Interaction) { + analysis_state = CEIAnalysisState::LookingForStorageReadOrWrite; + interaction_span = expr.span.clone(); + } + } + CEIAnalysisState::LookingForStorageReadOrWrite => warn_after_interaction( + &expr_effs, + &interaction_span, + &expr.span, + block_name, + warnings, + ), + }; + accumulated_effects.extend(expr_effs) + } + accumulated_effects +} + +// No need to worry about jumps because they are not allowed in `asm` blocks. +fn analyze_asm_block( + asm_block: &Vec, + block_name: &Ident, + warnings: &mut Vec, +) -> HashSet { + let mut interaction_span: Span = Span::dummy(); + let mut accumulated_effects = HashSet::new(); + let mut analysis_state: CEIAnalysisState = CEIAnalysisState::LookingForInteraction; + + for asm_op in asm_block { + let asm_op_effs = effects_of_asm_op(asm_op); + match analysis_state { + CEIAnalysisState::LookingForInteraction => { + if asm_op_effs.contains(&Effect::Interaction) { + analysis_state = CEIAnalysisState::LookingForStorageReadOrWrite; + interaction_span = asm_op.span.clone(); + } + } + CEIAnalysisState::LookingForStorageReadOrWrite => warn_after_interaction( + &asm_op_effs, + &interaction_span, + &asm_op.span, + block_name, + warnings, + ), + }; + accumulated_effects.extend(asm_op_effs) + } + accumulated_effects +} + +fn warn_after_interaction( + ast_node_effects: &HashSet, + interaction_span: &Span, + effect_span: &Span, + block_name: &Ident, + warnings: &mut Vec, +) { + 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) { + warnings.push(CompileWarning { + span: Span::join(interaction_span.clone(), effect_span.clone()), + warning_content: Warning::StorageWriteAfterInteraction { + block_name: block_name.clone(), + }, + }); + } +} + +fn effects_of_codeblock_entry(ast_node: &ty::TyAstNode) -> HashSet { + match &ast_node.content { + ty::TyAstNodeContent::Declaration(decl) => effects_of_codeblock_decl(decl), + ty::TyAstNodeContent::Expression(expr) + | ty::TyAstNodeContent::ImplicitReturnExpression(expr) => effects_of_expression(expr), + ty::TyAstNodeContent::SideEffect => HashSet::new(), + } +} + +fn effects_of_codeblock_decl(decl: &ty::TyDeclaration) -> HashSet { + use crate::ty::TyDeclaration::*; + match decl { + VariableDeclaration(var_decl) => effects_of_expression(&var_decl.body), + // Declarations (except variable declarations) are not allowed in the body of a function + _ => HashSet::new(), + } +} + +fn effects_of_expression(expr: &ty::TyExpression) -> HashSet { + use crate::ty::TyExpressionVariant::*; + match &expr.expression { + Literal(_) + | VariableExpression { .. } + | FunctionParameter + | Break + | Continue + | AbiName(_) => HashSet::new(), + // this type of assignment only mutates local variables and not storage + Reassignment(reassgn) => effects_of_expression(&reassgn.rhs), + StorageAccess(_) => HashSet::from([Effect::StorageRead]), + StorageReassignment(storage_reassign) => { + let mut effs = HashSet::from([Effect::StorageWrite]); + effs.extend(effects_of_expression(&storage_reassign.rhs)); + effs + } + LazyOperator { lhs, rhs, .. } + | ArrayIndex { + prefix: lhs, + index: rhs, + } => { + let mut effs = effects_of_expression(lhs); + let rhs_effs = effects_of_expression(rhs); + effs.extend(rhs_effs); + effs + } + Tuple { fields: exprs } | Array { contents: exprs } => effects_of_expressions(exprs), + StructExpression { fields, .. } => effects_of_struct_expressions(fields), + CodeBlock(codeblock) => effects_of_codeblock(codeblock), + IfExp { + condition, + then, + r#else, + } => { + let mut effs = effects_of_expression(condition); + effs.extend(effects_of_expression(then)); + let else_effs = match r#else { + Some(expr) => effects_of_expression(expr), + None => HashSet::new(), + }; + effs.extend(else_effs); + effs + } + StructFieldAccess { prefix: expr, .. } + | TupleElemAccess { prefix: expr, .. } + | EnumTag { exp: expr } + | UnsafeDowncast { exp: expr, .. } + | Return(expr) => effects_of_expression(expr), + EnumInstantiation { contents, .. } => match contents { + Some(expr) => effects_of_expression(expr), + None => HashSet::new(), + }, + AbiCast { address, .. } => effects_of_expression(address), + IntrinsicFunction(intr_fn) => effects_of_expressions(&intr_fn.arguments) + .union(&effects_of_intrinsic(&intr_fn.kind)) + .cloned() + .collect(), + WhileLoop { condition, body } => effects_of_expression(condition) + .union(&effects_of_codeblock(body)) + .cloned() + .collect(), + FunctionApplication { + function_decl_id, + arguments, + selector, + .. + } => { + use crate::declaration_engine::de_get_function; + let fn_body = de_get_function(function_decl_id.clone(), &expr.span) + .unwrap() + .body; + let mut effs = effects_of_codeblock(&fn_body); + let args_effs = map_hashsets_union(arguments, |e| effects_of_expression(&e.1)); + effs.extend(args_effs); + if selector.is_some() { + // external contract call (a.k.a. interaction) + effs.extend(HashSet::from([Effect::Interaction])) + }; + effs + } + AsmExpression { + registers, + body, + whole_block_span: _, + .. + } => effects_of_register_initializers(registers) + .union(&effects_of_asm_ops(body)) + .cloned() + .collect(), + } +} + +fn effects_of_intrinsic(intr: &sway_ast::Intrinsic) -> HashSet { + use sway_ast::Intrinsic::*; + match intr { + StateStoreWord | StateStoreQuad => HashSet::from([Effect::StorageWrite]), + StateLoadWord | StateLoadQuad | GetStorageKey => HashSet::from([Effect::StorageRead]), + Revert | IsReferenceType | SizeOfType | SizeOfVal | Eq | Gtf | AddrOf | Log | Add | Sub + | Mul | Div | PtrAdd | PtrSub => HashSet::new(), + } +} + +fn effects_of_asm_op(op: &AsmOp) -> HashSet { + match op.op_name.as_str().to_lowercase().as_str() { + "sww" | "swwq" => HashSet::from([Effect::StorageWrite]), + "srw" | "srwq" | "bal" => HashSet::from([Effect::StorageRead]), + "call" => HashSet::from([Effect::Interaction]), + // the rest of the assembly instructions are considered to not have effects + _ => HashSet::new(), + } +} + +fn set_union(set1: HashSet, set2: HashSet) -> HashSet +where + E: std::hash::Hash + Eq + Clone, +{ + set1.union(&set2).cloned().collect() +} + +fn map_hashsets_union(elems: &[T], to_set: F) -> HashSet +where + F: Fn(&T) -> HashSet, + E: std::hash::Hash + Eq + Clone, +{ + elems + .iter() + .fold(HashSet::new(), |set, e| set_union(to_set(e), set)) +} + +fn effects_of_codeblock(codeblock: &ty::TyCodeBlock) -> HashSet { + map_hashsets_union(&codeblock.contents, effects_of_codeblock_entry) +} + +fn effects_of_expressions(exprs: &[ty::TyExpression]) -> HashSet { + map_hashsets_union(exprs, effects_of_expression) +} + +fn effects_of_struct_expressions(struct_exprs: &[ty::TyStructExpressionField]) -> HashSet { + map_hashsets_union(struct_exprs, |se| effects_of_expression(&se.value)) +} + +fn effects_of_asm_ops(asm_ops: &[AsmOp]) -> HashSet { + map_hashsets_union(asm_ops, effects_of_asm_op) +} + +fn effects_of_register_initializers( + initializers: &[ty::TyAsmRegisterDeclaration], +) -> HashSet { + map_hashsets_union(initializers, |asm_reg_decl| { + asm_reg_decl + .initializer + .as_ref() + .map_or(HashSet::new(), effects_of_expression) + }) +} diff --git a/sway-error/src/warning.rs b/sway-error/src/warning.rs index 3cb028fadda..1e7ba4bbc15 100644 --- a/sway-error/src/warning.rs +++ b/sway-error/src/warning.rs @@ -91,6 +91,12 @@ pub enum Warning { UnrecognizedAttribute { attrib_name: Ident, }, + StorageWriteAfterInteraction { + block_name: Ident, + }, + StorageReadAfterInteraction { + block_name: Ident, + }, } impl fmt::Display for Warning { @@ -216,6 +222,10 @@ 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"), } } } diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation/Forc.lock b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation/Forc.lock new file mode 100644 index 00000000000..747b638b7c7 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation/Forc.lock @@ -0,0 +1,13 @@ +[[package]] +name = 'cei_pattern_violation' +source = 'member' +dependencies = ['std'] + +[[package]] +name = 'core' +source = 'path+from-root-DF35851ECC937459' + +[[package]] +name = 'std' +source = 'path+from-root-DF35851ECC937459' +dependencies = ['core'] diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation/Forc.toml b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation/Forc.toml new file mode 100644 index 00000000000..d63837e418c --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation/Forc.toml @@ -0,0 +1,8 @@ +[project] +name = "cei_pattern_violation" +authors = ["Fuel Labs "] +entry = "main.sw" +license = "Apache-2.0" + +[dependencies] +std = { path = "../../../../../../../sway-lib-std" } diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation/src/main.sw b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation/src/main.sw new file mode 100644 index 00000000000..f92044bef63 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation/src/main.sw @@ -0,0 +1,21 @@ +contract; + +use std::storage::store; + +abi TestAbi { + #[storage(write)] + fn deposit(); +} + +impl TestAbi for Contract { + #[storage(write)] + fn deposit() { + let other_contract = abi(TestAbi, 0x3dba0a4455b598b7655a7fb430883d96c9527ef275b49739e7b0ad12f8280eae); + + // interaction + other_contract.deposit(); + // effect -- therefore violation of CEI where effect should go before interaction + let storage_key = 0x3dba0a4455b598b7655a7fb430883d96c9527ef275b49739e7b0ad12f8280eae; + store(storage_key, ()); + } +} diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation/test.toml b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation/test.toml new file mode 100644 index 00000000000..bd3dfe65d20 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation/test.toml @@ -0,0 +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 diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block/Forc.lock b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block/Forc.lock new file mode 100644 index 00000000000..1f1358dd63a --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block/Forc.lock @@ -0,0 +1,3 @@ +[[package]] +name = 'cei_pattern_violation_in_asm_block' +source = 'member' diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block/Forc.toml b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block/Forc.toml new file mode 100644 index 00000000000..c2dac6f008d --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block/Forc.toml @@ -0,0 +1,6 @@ +[project] +name = "cei_pattern_violation_in_asm_block" +authors = ["Fuel Labs "] +entry = "main.sw" +license = "Apache-2.0" +implicit-std = false diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block/src/main.sw b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block/src/main.sw new file mode 100644 index 00000000000..22112bac422 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block/src/main.sw @@ -0,0 +1,22 @@ +contract; + +const KEY = 0x0000000000000000000000000000000000000000000000000000000000000000; + +abi TestAbi { + #[storage(write)] + fn deposit(); +} + +impl TestAbi for Contract { + #[storage(write)] + fn deposit() { + let other_contract = abi(TestAbi, 0x3dba0a4455b598b7655a7fb430883d96c9527ef275b49739e7b0ad12f8280eae); + + // interaction + other_contract.deposit(); + // effect -- therefore violation of CEI where effect should go before interaction + asm(key: KEY, is_set, v: 42) { + sww key is_set v; + } + } +} diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block/test.toml b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block/test.toml new file mode 100644 index 00000000000..bd3dfe65d20 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block/test.toml @@ -0,0 +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 diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block_read/Forc.lock b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block_read/Forc.lock new file mode 100644 index 00000000000..6c75d48cac1 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block_read/Forc.lock @@ -0,0 +1,3 @@ +[[package]] +name = 'cei_pattern_violation_in_asm_block_read' +source = 'member' diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block_read/Forc.toml b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block_read/Forc.toml new file mode 100644 index 00000000000..5ee48b22436 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block_read/Forc.toml @@ -0,0 +1,6 @@ +[project] +name = "cei_pattern_violation_in_asm_block_read" +authors = ["Fuel Labs "] +entry = "main.sw" +license = "Apache-2.0" +implicit-std = false diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block_read/src/main.sw b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block_read/src/main.sw new file mode 100644 index 00000000000..e7a58fd45ca --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block_read/src/main.sw @@ -0,0 +1,23 @@ +contract; + +const KEY = 0x0000000000000000000000000000000000000000000000000000000000000000; + +abi TestAbi { + #[storage(read)] + fn deposit() -> u64; +} + +impl TestAbi for Contract { + #[storage(read)] + fn deposit() -> u64{ + let other_contract = abi(TestAbi, 0x3dba0a4455b598b7655a7fb430883d96c9527ef275b49739e7b0ad12f8280eae); + + // interaction + other_contract.deposit(); + // effect -- therefore violation of CEI where effect should go before interaction + asm(key: KEY, is_set, res) { + srw res is_set key; + res: u64 + } + } +} diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block_read/test.toml b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block_read/test.toml new file mode 100644 index 00000000000..635693c67cb --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_asm_block_read/test.toml @@ -0,0 +1,3 @@ +category = "compile" + +# check: $()Storage read after external contract interaction in function or method "deposit". Consider making all storage reads before calling another contract diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_codeblocks_other_than_in_functions/Forc.lock b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_codeblocks_other_than_in_functions/Forc.lock new file mode 100644 index 00000000000..53a3b0c36c5 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_codeblocks_other_than_in_functions/Forc.lock @@ -0,0 +1,13 @@ +[[package]] +name = 'cei_pattern_violation_in_codeblocks_other_than_in_functions' +source = 'member' +dependencies = ['std'] + +[[package]] +name = 'core' +source = 'path+from-root-374ED50208B00A57' + +[[package]] +name = 'std' +source = 'path+from-root-374ED50208B00A57' +dependencies = ['core'] diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_codeblocks_other_than_in_functions/Forc.toml b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_codeblocks_other_than_in_functions/Forc.toml new file mode 100644 index 00000000000..9925152dbfb --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_codeblocks_other_than_in_functions/Forc.toml @@ -0,0 +1,8 @@ +[project] +name = "cei_pattern_violation_in_codeblocks_other_than_in_functions" +authors = ["Fuel Labs "] +entry = "main.sw" +license = "Apache-2.0" + +[dependencies] +std = { path = "../../../../../../../sway-lib-std" } diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_codeblocks_other_than_in_functions/src/main.sw b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_codeblocks_other_than_in_functions/src/main.sw new file mode 100644 index 00000000000..6e502d06b8f --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_codeblocks_other_than_in_functions/src/main.sw @@ -0,0 +1,25 @@ +contract; + +use std::storage::store; + +abi TestAbi { + #[storage(write)] + fn deposit(amount: u64); +} + +impl TestAbi for Contract { + #[storage(write)] + fn deposit(amount: u64) { + // a code block inside the function body: a simpler version of the CEI analysis does not catch this + { + let other_contract = abi(TestAbi, 0x3dba0a4455b598b7655a7fb430883d96c9527ef275b49739e7b0ad12f8280eae); + + // interaction + other_contract.deposit(amount); + + // effect -- therefore violation of CEI where effect should go before interaction + let storage_key = 0x3dba0a4455b598b7655a7fb430883d96c9527ef275b49739e7b0ad12f8280eae; + store(storage_key, ()); + } + } +} diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_codeblocks_other_than_in_functions/test.toml b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_codeblocks_other_than_in_functions/test.toml new file mode 100644 index 00000000000..bd3dfe65d20 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_codeblocks_other_than_in_functions/test.toml @@ -0,0 +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 diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_if_statement/Forc.lock b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_if_statement/Forc.lock new file mode 100644 index 00000000000..61f9de3f384 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_if_statement/Forc.lock @@ -0,0 +1,13 @@ +[[package]] +name = 'cei_pattern_violation_in_if_statement' +source = 'member' +dependencies = ['std'] + +[[package]] +name = 'core' +source = 'path+from-root-E0ED002AB6D4EBB7' + +[[package]] +name = 'std' +source = 'path+from-root-E0ED002AB6D4EBB7' +dependencies = ['core'] diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_if_statement/Forc.toml b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_if_statement/Forc.toml new file mode 100644 index 00000000000..b4cb9b9e7d4 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_if_statement/Forc.toml @@ -0,0 +1,8 @@ +[project] +name = "cei_pattern_violation_in_if_statement" +authors = ["Fuel Labs "] +entry = "main.sw" +license = "Apache-2.0" + +[dependencies] +std = { path = "../../../../../../../sway-lib-std" } diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_if_statement/src/main.sw b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_if_statement/src/main.sw new file mode 100644 index 00000000000..8bad15ef342 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_if_statement/src/main.sw @@ -0,0 +1,24 @@ +contract; + +use std::storage::store; + +abi TestAbi { + #[storage(write)] + fn deposit(amount: u64); +} + +impl TestAbi for Contract { + #[storage(write)] + fn deposit(amount: u64) { + let other_contract = abi(TestAbi, 0x3dba0a4455b598b7655a7fb430883d96c9527ef275b49739e7b0ad12f8280eae); + + // interaction + other_contract.deposit(amount); + + if amount == 42 { + // effect -- therefore violation of CEI where effect should go before interaction + let storage_key = 0x3dba0a4455b598b7655a7fb430883d96c9527ef275b49739e7b0ad12f8280eae; + store(storage_key, ()); + } + } +} diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_if_statement/test.toml b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_if_statement/test.toml new file mode 100644 index 00000000000..bd3dfe65d20 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_if_statement/test.toml @@ -0,0 +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 diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_standalone_function/Forc.lock b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_standalone_function/Forc.lock new file mode 100644 index 00000000000..dbaa97f6d3a --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_standalone_function/Forc.lock @@ -0,0 +1,13 @@ +[[package]] +name = 'cei_pattern_violation_in_standalone_function' +source = 'member' +dependencies = ['std'] + +[[package]] +name = 'core' +source = 'path+from-root-7DCC831BB261963F' + +[[package]] +name = 'std' +source = 'path+from-root-7DCC831BB261963F' +dependencies = ['core'] diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_standalone_function/Forc.toml b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_standalone_function/Forc.toml new file mode 100644 index 00000000000..bbf4dd5da60 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_standalone_function/Forc.toml @@ -0,0 +1,8 @@ +[project] +name = "cei_pattern_violation_in_standalone_function" +authors = ["Fuel Labs "] +entry = "main.sw" +license = "Apache-2.0" + +[dependencies] +std = { path = "../../../../../../../sway-lib-std" } diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_standalone_function/src/main.sw b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_standalone_function/src/main.sw new file mode 100644 index 00000000000..9ec8ad73c50 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_standalone_function/src/main.sw @@ -0,0 +1,26 @@ +contract; + +use std::storage::store; + +abi TestAbi { + #[storage(write)] + fn deposit(); +} + +#[storage(write)] +fn standalone_function() { + let other_contract = abi(TestAbi, 0x3dba0a4455b598b7655a7fb430883d96c9527ef275b49739e7b0ad12f8280eae); + + // interaction + other_contract.deposit(); + // effect -- therefore violation of CEI where effect should go before interaction + let storage_key = 0x3dba0a4455b598b7655a7fb430883d96c9527ef275b49739e7b0ad12f8280eae; + store(storage_key, ()); +} + +impl TestAbi for Contract { + #[storage(write)] + fn deposit() { + standalone_function(); + } +} diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_standalone_function/test.toml b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_standalone_function/test.toml new file mode 100644 index 00000000000..0996a3a4213 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_in_standalone_function/test.toml @@ -0,0 +1,3 @@ +category = "compile" + +# check: $()Storage modification after external contract interaction in function or method "standalone_function". Consider making all storage writes before calling another contract diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_more_complex_logic/Forc.lock b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_more_complex_logic/Forc.lock new file mode 100644 index 00000000000..4706d0627bc --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_more_complex_logic/Forc.lock @@ -0,0 +1,13 @@ +[[package]] +name = 'cei_pattern_violation_more_complex_logic' +source = 'member' +dependencies = ['std'] + +[[package]] +name = 'core' +source = 'path+from-root-C4D8ECCE530DF143' + +[[package]] +name = 'std' +source = 'path+from-root-C4D8ECCE530DF143' +dependencies = ['core'] diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_more_complex_logic/Forc.toml b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_more_complex_logic/Forc.toml new file mode 100644 index 00000000000..458b2928a44 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_more_complex_logic/Forc.toml @@ -0,0 +1,8 @@ +[project] +name = "cei_pattern_violation_more_complex_logic" +authors = ["Fuel Labs "] +entry = "main.sw" +license = "Apache-2.0" + +[dependencies] +std = { path = "../../../../../../../sway-lib-std" } diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_more_complex_logic/src/main.sw b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_more_complex_logic/src/main.sw new file mode 100644 index 00000000000..a2ae95323dc --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_more_complex_logic/src/main.sw @@ -0,0 +1,125 @@ +contract; + +pub enum AuctionAsset { + NFTAsset: u64, + TokenAsset: u64, +} + +abi EnglishAuction { + #[storage(read, write)] + fn bid(auction_id: u64, bid_asset: AuctionAsset); + + #[storage(read, write)] + fn create(bid_asset: AuctionAsset, duration: u64, inital_price: u64, reserve_price: Option, seller: Identity, sell_asset: AuctionAsset) -> u64; +} + +abi NFT { + fn owner_of(token_id: u64) -> Identity; + fn transfer_from(from: Identity, to: Identity, token_id: u64); +} + +use std::{ + block::height, + auth::msg_sender, + call_frames::{ + contract_id, + msg_asset_id, + }, + context::msg_amount, + storage::StorageMap, +}; + +storage { + auctions: StorageMap> = StorageMap {}, + deposits: StorageMap<(Identity, u64), Option> = StorageMap {}, + total_auctions: u64 = 0, +} + +const ADDR: b256 = 0x0000000000000000000000000000000000000000000000000000000000000000; + +pub fn transfer_nft(asset: u64, from: Identity, to: Identity) { + let nft_abi = abi(NFT, ADDR); + + nft_abi.transfer_from(from, to, asset); + + let owner = nft_abi.owner_of(asset); + require(owner == to, 42); +} + +impl EnglishAuction for Contract { + #[storage(read, write)] + fn bid(auction_id: u64, bid_asset: AuctionAsset) { + let auction = storage.auctions.get(auction_id); + require(auction.is_some(), 42); + + let mut auction = auction.unwrap(); + let sender = msg_sender().unwrap(); + + let sender_deposit = storage.deposits.get((sender, auction_id)); + let total_bid: AuctionAsset = match sender_deposit { + Option::Some(_) => { + AuctionAsset::TokenAsset(42) + }, + Option::None => { + AuctionAsset::NFTAsset(42) + } + }; + + match total_bid { + AuctionAsset::NFTAsset(nft_asset) => { + transfer_nft(nft_asset, sender, Identity::ContractId(contract_id())); + }, + AuctionAsset::TokenAsset(token_asset) => { + require(token_asset == 42, 42); + } + } + + storage.deposits.insert((sender, auction_id), Option::Some(AuctionAsset::TokenAsset(42))); + } + + #[storage(read, write)] + fn create( + bid_asset: AuctionAsset, + duration: u64, + initial_price: u64, + reserve_price: Option, + seller: Identity, + sell_asset: AuctionAsset, + ) -> u64 { + require(reserve_price.is_none() || (reserve_price.is_some() && reserve_price.unwrap() >= initial_price), 42); + require(duration != 0, 42); + + match bid_asset { + AuctionAsset::TokenAsset(asset) => { + require(asset == 0, 42); + }, + AuctionAsset::NFTAsset(asset) => { + require(asset == 0, 42); + } + } + + match sell_asset { + AuctionAsset::TokenAsset(asset) => { + require(initial_price != 0, 42); + require(msg_amount() == asset, 42); + }, + AuctionAsset::NFTAsset(asset) => { + // Selling NFTs + let sender = msg_sender().unwrap(); + // TODO: Remove this when StorageVec in structs is supported + require(initial_price == 1, 42); + transfer_nft(asset, sender, Identity::ContractId(contract_id())); + } + } + + let auction = 42; + + let total_auctions = storage.total_auctions; + storage.deposits.insert((seller, total_auctions), Option::Some(sell_asset)); + storage.auctions.insert(total_auctions, Option::Some(auction)); + + storage.total_auctions += 1; + total_auctions + } + +} diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_more_complex_logic/test.toml b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_more_complex_logic/test.toml new file mode 100644 index 00000000000..dbf0652871f --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_more_complex_logic/test.toml @@ -0,0 +1,9 @@ +category = "compile" + +# check: $()Storage read after external contract interaction in function or method "bid". Consider making all storage reads before calling another contract + +# check: $()Storage modification after external contract interaction in function or method "bid". Consider making all storage writes before calling another contract + +# check: $()Storage read after external contract interaction in function or method "create". Consider making all storage reads before calling another contract + +# check: $()Storage modification after external contract interaction in function or method "create". Consider making all storage writes before calling another contract diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_storage_var_read/Forc.lock b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_storage_var_read/Forc.lock new file mode 100644 index 00000000000..ea8e4907a57 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_storage_var_read/Forc.lock @@ -0,0 +1,3 @@ +[[package]] +name = 'cei_pattern_violation_storage_var_read' +source = 'member' diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_storage_var_read/Forc.toml b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_storage_var_read/Forc.toml new file mode 100644 index 00000000000..261c013024b --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_storage_var_read/Forc.toml @@ -0,0 +1,6 @@ +[project] +name = "cei_pattern_violation_storage_var_read" +authors = ["Fuel Labs "] +entry = "main.sw" +license = "Apache-2.0" +implicit-std = false diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_storage_var_read/src/main.sw b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_storage_var_read/src/main.sw new file mode 100644 index 00000000000..7bca58f5f38 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_storage_var_read/src/main.sw @@ -0,0 +1,22 @@ +contract; + +storage { + var: u64 = 0, +} + +abi TestAbi { + #[storage(read)] + fn deposit() -> u64; +} + +impl TestAbi for Contract { + #[storage(read)] + fn deposit() -> u64 { + let other_contract = abi(TestAbi, 0x3dba0a4455b598b7655a7fb430883d96c9527ef275b49739e7b0ad12f8280eae); + + // interaction + other_contract.deposit(); + // effect -- therefore violation of CEI where effect should go before interaction + storage.var + } +} diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_storage_var_read/test.toml b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_storage_var_read/test.toml new file mode 100644 index 00000000000..635693c67cb --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_storage_var_read/test.toml @@ -0,0 +1,3 @@ +category = "compile" + +# check: $()Storage read after external contract interaction in function or method "deposit". Consider making all storage reads before calling another contract diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_storage_var_update/Forc.lock b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_storage_var_update/Forc.lock new file mode 100644 index 00000000000..54cef5bcd4d --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_storage_var_update/Forc.lock @@ -0,0 +1,3 @@ +[[package]] +name = 'cei_pattern_violation_storage_var_update' +source = 'member' diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_storage_var_update/Forc.toml b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_storage_var_update/Forc.toml new file mode 100644 index 00000000000..c514e30b52c --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_storage_var_update/Forc.toml @@ -0,0 +1,6 @@ +[project] +name = "cei_pattern_violation_storage_var_update" +authors = ["Fuel Labs "] +entry = "main.sw" +license = "Apache-2.0" +implicit-std = false diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_storage_var_update/src/main.sw b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_storage_var_update/src/main.sw new file mode 100644 index 00000000000..4c734fce69e --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_storage_var_update/src/main.sw @@ -0,0 +1,22 @@ +contract; + +storage { + var: u64 = 0, +} + +abi TestAbi { + #[storage(write)] + fn deposit(); +} + +impl TestAbi for Contract { + #[storage(write)] + fn deposit() { + let other_contract = abi(TestAbi, 0x3dba0a4455b598b7655a7fb430883d96c9527ef275b49739e7b0ad12f8280eae); + + // interaction + other_contract.deposit(); + // effect -- therefore violation of CEI where effect should go before interaction + storage.var = 42; + } +} diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_storage_var_update/test.toml b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_storage_var_update/test.toml new file mode 100644 index 00000000000..bd3dfe65d20 --- /dev/null +++ b/test/src/e2e_vm_tests/test_programs/should_pass/static_analysis/cei_pattern_violation_storage_var_update/test.toml @@ -0,0 +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