functions that use .call() should be allowed to declared view or defa…

…ult mutability (#1338) Neither option should produce a diagnostic. Fixes #997 Signed-off-by: Sean Young <[email protected]>
hyperledger · Jun 16, 2023 · 4026d1c · 4026d1c
1 parent 1985aca
commit 4026d1c
Show file tree

Hide file tree

Showing 7 changed files with 167 additions and 74 deletions.
diff --git a/src/sema/mutability.rs b/src/sema/mutability.rs
@@ -1,19 +1,36 @@
 // SPDX-License-Identifier: Apache-2.0
 
-use super::ast::{
-    Builtin, DestructureField, Diagnostic, Expression, Function, Mutability, Namespace, Statement,
-    Type,
+use super::{
+    ast::{
+        Builtin, CallTy, DestructureField, Diagnostic, Expression, Function, Mutability, Namespace,
+        RetrieveType, Statement, Type,
+    },
+    yul::ast::{YulExpression, YulStatement},
+    Recurse,
 };
-use crate::sema::ast::RetrieveType;
-use crate::sema::yul::ast::{YulExpression, YulStatement};
-use crate::sema::Recurse;
-use solang_parser::pt;
+use solang_parser::{helpers::CodeLocation, pt};
+
+#[derive(PartialEq, PartialOrd)]
+enum Access {
+    None,
+    Read,
+    Write,
+    Value,
+}
+
+impl Access {
+    fn increase_to(&mut self, other: Access) {
+        if *self < other {
+            *self = other;
+        }
+    }
+}
 
 /// check state mutability
 pub fn mutability(file_no: usize, ns: &mut Namespace) {
     if !ns.diagnostics.any_errors() {
         for func in &ns.functions {
-            if func.loc.try_file_no() != Some(file_no) {
+            if func.loc.try_file_no() != Some(file_no) || func.ty == pt::FunctionTy::Modifier {
                 continue;
             }
 
@@ -27,41 +44,90 @@ pub fn mutability(file_no: usize, ns: &mut Namespace) {
 /// While we recurse through the AST, maintain some state
 struct StateCheck<'a> {
     diagnostics: Vec<Diagnostic>,
-    does_read_state: bool,
-    does_write_state: bool,
-    can_read_state: bool,
-    can_write_state: bool,
+    declared_access: Access,
+    required_access: Access,
     func: &'a Function,
+    modifier: Option<pt::Loc>,
     ns: &'a Namespace,
 }
 
 impl<'a> StateCheck<'a> {
+    fn value(&mut self, loc: &pt::Loc) {
+        if self.declared_access != Access::Value {
+            if let Some(modifier_loc) = &self.modifier {
+                self.diagnostics.push(Diagnostic::error_with_note(
+                    *modifier_loc,
+                    format!(
+                        "function declared '{}' but modifier accesses value sent, which is only allowed for payable functions",
+                        self.func.mutability
+                    ),
+                    *loc,
+                    "access of value sent".into()
+                ));
+            } else {
+                self.diagnostics.push(Diagnostic::error(
+                    *loc,
+                    format!(
+                        "function declared '{}' but this expression accesses value sent, which is only allowed for payable functions",
+                        self.func.mutability
+                    ),
+                ));
+            }
+        }
+
+        self.required_access.increase_to(Access::Value);
+    }
+
     fn write(&mut self, loc: &pt::Loc) {
-        if !self.can_write_state {
-            self.diagnostics.push(Diagnostic::error(
-                *loc,
-                format!(
-                    "function declared '{}' but this expression writes to state",
-                    self.func.mutability
-                ),
-            ));
+        if self.declared_access < Access::Write {
+            if let Some(modifier_loc) = &self.modifier {
+                self.diagnostics.push(Diagnostic::error_with_note(
+                    *modifier_loc,
+                    format!(
+                        "function declared '{}' but modifier writes to state",
+                        self.func.mutability
+                    ),
+                    *loc,
+                    "write to state".into(),
+                ));
+            } else {
+                self.diagnostics.push(Diagnostic::error(
+                    *loc,
+                    format!(
+                        "function declared '{}' but this expression writes to state",
+                        self.func.mutability
+                    ),
+                ));
+            }
         }
 
-        self.does_write_state = true;
+        self.required_access.increase_to(Access::Write);
     }
 
     fn read(&mut self, loc: &pt::Loc) {
-        if !self.can_read_state {
-            self.diagnostics.push(Diagnostic::error(
-                *loc,
-                format!(
-                    "function declared '{}' but this expression reads from state",
-                    self.func.mutability
-                ),
-            ));
+        if self.declared_access < Access::Read {
+            if let Some(modifier_loc) = &self.modifier {
+                self.diagnostics.push(Diagnostic::error_with_note(
+                    *modifier_loc,
+                    format!(
+                        "function declared '{}' but modifier reads from state",
+                        self.func.mutability
+                    ),
+                    *loc,
+                    "read to state".into(),
+                ));
+            } else {
+                self.diagnostics.push(Diagnostic::error(
+                    *loc,
+                    format!(
+                        "function declared '{}' but this expression reads from state",
+                        self.func.mutability
+                    ),
+                ));
+            }
         }
 
-        self.does_read_state = true;
+        self.required_access.increase_to(Access::Read);
     }
 }
 
@@ -72,25 +138,18 @@ fn check_mutability(func: &Function, ns: &Namespace) -> Vec<Diagnostic> {
 
     let mut state = StateCheck {
         diagnostics: Vec::new(),
-        does_read_state: false,
-        does_write_state: false,
-        can_write_state: false,
-        can_read_state: false,
+        declared_access: match func.mutability {
+            Mutability::Pure(_) => Access::None,
+            Mutability::View(_) => Access::Read,
+            Mutability::Nonpayable(_) => Access::Write,
+            Mutability::Payable(_) => Access::Value,
+        },
+        required_access: Access::None,
         func,
+        modifier: None,
         ns,
     };
 
-    match func.mutability {
-        Mutability::Pure(_) => (),
-        Mutability::View(_) => {
-            state.can_read_state = true;
-        }
-        Mutability::Payable(_) | Mutability::Nonpayable(_) => {
-            state.can_read_state = true;
-            state.can_write_state = true;
-        }
-    };
-
     for arg in &func.modifiers {
         if let Expression::InternalFunctionCall { function, args, .. } = &arg {
             // check the arguments to the modifiers
@@ -118,15 +177,19 @@ fn check_mutability(func: &Function, ns: &Namespace) -> Vec<Diagnostic> {
                 // modifiers do not have mutability, bases or modifiers itself
                 let func = &ns.functions[function_no];
 
+                state.modifier = Some(arg.loc());
+
                 recurse_statements(&func.body, ns, &mut state);
+
+                state.modifier = None;
             }
         }
     }
 
     recurse_statements(&func.body, ns, &mut state);
 
     if pt::FunctionTy::Function == func.ty && !func.is_accessor {
-        if !state.does_write_state && !state.does_read_state {
+        if state.required_access == Access::None {
             match func.mutability {
                 Mutability::Payable(_) | Mutability::Pure(_) => (),
                 Mutability::Nonpayable(_) => {
@@ -147,7 +210,8 @@ fn check_mutability(func: &Function, ns: &Namespace) -> Vec<Diagnostic> {
             }
         }
 
-        if !state.does_write_state && state.does_read_state && func.mutability.is_default() {
+        // don't suggest marking payable as view (declared_access == Value)
+        if state.required_access == Access::Read && state.declared_access == Access::Write {
             state.diagnostics.push(Diagnostic::warning(
                 func.loc,
                 "function can be declared 'view'".to_string(),
@@ -293,6 +357,19 @@ fn read_expression(expr: &Expression, state: &mut StateCheck) -> bool {
             kind: Builtin::PayableSend | Builtin::PayableTransfer | Builtin::SelfDestruct,
             ..
         } => state.write(loc),
+        Expression::Builtin {
+            loc,
+            kind: Builtin::Value,
+            ..
+        } => {
+            // internal/private functions cannot be declared payable, so msg.value is only checked
+            // as reading state in private/internal functions in solc.
+            if state.func.is_public() {
+                state.value(loc)
+            } else {
+                state.read(loc)
+            }
+        }
         Expression::Builtin {
             loc,
             kind: Builtin::ArrayPush | Builtin::ArrayPop,
@@ -315,13 +392,10 @@ fn read_expression(expr: &Expression, state: &mut StateCheck) -> bool {
             }
             _ => unreachable!(),
         },
-        Expression::ExternalFunctionCallRaw { loc, .. } => {
-            if state.ns.target.is_substrate() {
-                state.write(loc)
-            } else {
-                state.read(loc)
-            }
-        }
+        Expression::ExternalFunctionCallRaw { loc, ty, .. } => match ty {
+            CallTy::Static => state.read(loc),
+            CallTy::Delegate | CallTy::Regular => state.write(loc),
+        },
         _ => {
             return true;
         }

diff --git a/tests/contract_testcases/evm/call/call_02.sol b/tests/contract_testcases/evm/call/call_02.sol
@@ -4,8 +4,7 @@
                 (bool s, bytes memory bs) = a.call{value: 2}("");
             }
         }
-        
+
 // ---- Expect: diagnostics ----
-// warning: 3:13-49: function can be declared 'view'
 // warning: 4:23-24: destructure variable 's' has never been used
 // warning: 4:39-41: destructure variable 'bs' has never been used
diff --git a/tests/contract_testcases/evm/comment_tests.sol b/tests/contract_testcases/evm/comment_tests.sol
@@ -678,9 +678,7 @@ function _approve(
     //
 }/**//**//**//**//**//**//**///
 // ---- Expect: diagnostics ----
-// warning: 188:5-75: function can be declared 'view'
 // warning: 195:50-56: conversion truncates uint256 to uint128, as value is type uint128 on target evm
-// warning: 264:5-270:37: function can be declared 'view'
 // warning: 268:17-25: function parameter 'weiValue' is unused
 // warning: 269:23-35: function parameter 'errorMessage' is unused
 // warning: 276:70-78: conversion truncates uint256 to uint128, as value is type uint128 on target evm

diff --git a/tests/contract_testcases/substrate/builtins/msg_01.sol b/tests/contract_testcases/substrate/builtins/msg_01.sol
@@ -1,8 +1,8 @@
-
-        contract bar {
-            function test(uint128 v) public returns (bool) {
-                return msg.value > v;
-            }
-        }
+contract bar {
+	uint128 state;
+	function test(uint128 v) public returns (bool) {
+		return state > v;
+       }
+}
 // ---- Expect: diagnostics ----
-// warning: 3:13-59: function can be declared 'pure'
+// warning: 3:2-48: function can be declared 'view'
diff --git a/tests/contract_testcases/substrate/functions/mutability_12.sol b/tests/contract_testcases/substrate/functions/mutability_12.sol
@@ -0,0 +1,14 @@
+contract c {
+	// private functions cannot be payable and solc checks msg.value as
+	// state read in them
+	function foo() private view returns (uint) {
+		return msg.value;
+	}
+
+	function bar() public returns (uint) {
+		return foo();
+	}
+}
+
+// ---- Expect: diagnostics ----
+// warning: 8:2-38: function can be declared 'view'
diff --git a/tests/contract_testcases/substrate/modifier/mutability_01.sol b/tests/contract_testcases/substrate/modifier/mutability_01.sol
@@ -1,10 +1,18 @@
+contract c {
+    uint128 var;
+    modifier foo1() { uint128 x = var; _; }
+    modifier foo2() { var = 2; _; }
+    modifier foo3() { var = msg.value; _; }
 
-        contract c {
-            uint64 var;
-            modifier foo() { uint64 x = var; _; }
-
-            function bar() foo() public pure {}
-        }
+    function bar1() foo1() public pure {}
+    function bar2() foo2() public view {}
+    function bar3() foo3() public {}
+}
 // ---- Expect: diagnostics ----
-// warning: 4:37-38: local variable 'x' has been assigned, but never read
-// error: 4:41-44: function declared 'pure' but this expression reads from state
+// warning: 3:31-32: local variable 'x' has been assigned, but never read
+// error: 7:21-27: function declared 'pure' but modifier reads from state
+// 	note 3:35-38: read to state
+// error: 8:21-27: function declared 'view' but modifier writes to state
+// 	note 4:23-26: write to state
+// error: 9:21-27: function declared 'nonpayable' but modifier accesses value sent, which is only allowed for payable functions
+// 	note 5:29-38: access of value sent
diff --git a/tests/evm.rs b/tests/evm.rs
@@ -248,7 +248,7 @@ fn ethereum_solidity_tests() {
         })
         .sum();
 
-    assert_eq!(errors, 1048);
+    assert_eq!(errors, 1042);
 }
 
 fn set_file_contents(source: &str, path: &Path) -> (FileResolver, Vec<String>) {