Bugfix struct member read access (#1563)

Fixes a bug: `StorageLoad` after a struct member access is a storage
read, this case was not considered in the mutability check.

The regression in the EVM test is fine; it's caused by a [problem in
solc](https://github.com/ethereum/solidity/issues/11573)

Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Sean Young <sean@mess.org>

src/sema/mutability.rs

@@ -5,6 +5,7 @@ use super::{
         Builtin, CallTy, DestructureField, Diagnostic, Expression, Function, Mutability, Namespace,
         RetrieveType, Statement, Type,
     },
+    diagnostics::Diagnostics,
     yul::ast::{YulExpression, YulStatement},
     Recurse,
 };
@@ -16,7 +17,7 @@ use bitflags::bitflags;
 use solang_parser::pt::Loc;
 use solang_parser::{helpers::CodeLocation, pt};
 
-#[derive(PartialEq, PartialOrd)]
+#[derive(Clone, Copy, Hash, Eq, PartialEq, PartialOrd)]
 enum Access {
     None,
     Read,
@@ -49,16 +50,16 @@ pub fn mutability(file_no: usize, ns: &mut Namespace) {
                 continue;
             }
 
-            let mut diagnostics = check_mutability(func, ns);
+            let diagnostics = check_mutability(func, ns);
 
-            ns.diagnostics.append(&mut diagnostics);
+            ns.diagnostics.extend(diagnostics);
         }
     }
 }
 
 /// While we recurse through the AST, maintain some state
 struct StateCheck<'a> {
-    diagnostics: Vec<Diagnostic>,
+    diagnostic: Diagnostics,
     declared_access: Access,
     required_access: Access,
     func: &'a Function,
@@ -69,91 +70,65 @@ struct StateCheck<'a> {
 
 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.check_level(loc, Access::Value);
         self.required_access.increase_to(Access::Value);
     }
 
     fn write(&mut self, loc: &pt::Loc) {
-        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.check_level(loc, Access::Write);
         self.required_access.increase_to(Access::Write);
     }
 
     fn read(&mut self, loc: &pt::Loc) {
-        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.check_level(loc, Access::Read);
+        self.required_access.increase_to(Access::Read);
+    }
+
+    /// Compare the declared access level to the desired access level.
+    /// If there is an access violation, it'll be reported to the diagnostics.
+    fn check_level(&mut self, loc: &pt::Loc, desired: Access) {
+        if self.declared_access >= desired {
+            return;
         }
 
-        self.required_access.increase_to(Access::Read);
+        let (message, note) = match desired {
+            Access::Read => ("reads from state", "read to state"),
+            Access::Write => ("writes to state", "write to state"),
+            Access::Value => (
+                "accesses value sent, which is only allowed for payable functions",
+                "access of value sent",
+            ),
+            Access::None => unreachable!("desired access can't be None"),
+        };
+
+        let diagnostic = self
+            .modifier
+            .map(|modifier_loc| {
+                let message = format!(
+                    "function declared '{}' but modifier {}",
+                    self.func.mutability, message
+                );
+                Diagnostic::error_with_note(modifier_loc, message, *loc, note.into())
+            })
+            .unwrap_or_else(|| {
+                let message = format!(
+                    "function declared '{}' but this expression {}",
+                    self.func.mutability, message
+                );
+                Diagnostic::error(*loc, message)
+            });
+
+        self.diagnostic.push(diagnostic);
     }
 }
 
-fn check_mutability(func: &Function, ns: &Namespace) -> Vec<Diagnostic> {
+fn check_mutability(func: &Function, ns: &Namespace) -> Diagnostics {
     if func.is_virtual {
-        return Vec::new();
+        return Default::default();
     }
 
     let mut state = StateCheck {
-        diagnostics: Vec::new(),
+        diagnostic: Default::default(),
         declared_access: match func.mutability {
             Mutability::Pure(_) => Access::None,
             Mutability::View(_) => Access::Read,
@@ -213,13 +188,13 @@ fn check_mutability(func: &Function, ns: &Namespace) -> Vec<Diagnostic> {
             match func.mutability {
                 Mutability::Payable(_) | Mutability::Pure(_) => (),
                 Mutability::Nonpayable(_) => {
-                    state.diagnostics.push(Diagnostic::warning(
+                    state.diagnostic.push(Diagnostic::warning(
                         func.loc,
                         "function can be declared 'pure'".to_string(),
                     ));
                 }
                 _ => {
-                    state.diagnostics.push(Diagnostic::warning(
+                    state.diagnostic.push(Diagnostic::warning(
                         func.loc,
                         format!(
                             "function declared '{}' can be declared 'pure'",
@@ -232,7 +207,7 @@ fn check_mutability(func: &Function, ns: &Namespace) -> Vec<Diagnostic> {
 
         // 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(
+            state.diagnostic.push(Diagnostic::warning(
                 func.loc,
                 "function can be declared 'view'".to_string(),
             ));
@@ -271,7 +246,7 @@ fn check_mutability(func: &Function, ns: &Namespace) -> Vec<Diagnostic> {
         );
     }
 
-    state.diagnostics
+    state.diagnostic
 }
 
 fn recurse_statements(stmts: &[Statement], ns: &Namespace, state: &mut StateCheck) {
@@ -359,12 +334,15 @@ fn recurse_statements(stmts: &[Statement], ns: &Namespace, state: &mut StateChec
 
 fn read_expression(expr: &Expression, state: &mut StateCheck) -> bool {
     match expr {
+        Expression::StorageLoad { loc, .. } => {
+            state.data_account |= DataAccountUsage::READ;
+            state.read(loc)
+        }
         Expression::PreIncrement { expr, .. }
         | Expression::PreDecrement { expr, .. }
         | Expression::PostIncrement { expr, .. }
         | Expression::PostDecrement { expr, .. } => {
             expr.recurse(state, write_expression);
-            return false;
         }
         Expression::Assign { left, right, .. } => {
             right.recurse(state, read_expression);
@@ -374,14 +352,6 @@ fn read_expression(expr: &Expression, state: &mut StateCheck) -> bool {
         Expression::StorageArrayLength { loc, .. } => {
             state.data_account |= DataAccountUsage::READ;
             state.read(loc);
-            return false;
-        }
-        Expression::Subscript { loc, array_ty, .. } if array_ty.is_contract_storage() => {
-            state.data_account |= DataAccountUsage::READ;
-            state.read(loc);
-        }
-        Expression::Variable { ty, .. } if ty.is_contract_storage() => {
-            state.data_account |= DataAccountUsage::READ;
         }
         Expression::StorageVariable { loc, .. } => {
             state.data_account |= DataAccountUsage::READ;

src/sema/statements.rs

@@ -419,7 +419,7 @@ fn statement(
         pt::Statement::Block {
             statements,
             unchecked,
-            ..
+            loc,
         } => {
             symtable.new_scope();
             let mut reachable = true;
@@ -427,6 +427,8 @@ fn statement(
             let mut context = context.clone();
             context.unchecked |= *unchecked;
 
+            let mut resolved_stmts = Vec::new();
+
             for stmt in statements {
                 if !reachable {
                     ns.diagnostics.push(Diagnostic::error(
@@ -435,11 +437,25 @@ fn statement(
                     ));
                     return Err(());
                 }
-                reachable = statement(stmt, res, &mut context, symtable, loops, ns, diagnostics)?;
+                reachable = statement(
+                    stmt,
+                    &mut resolved_stmts,
+                    &mut context,
+                    symtable,
+                    loops,
+                    ns,
+                    diagnostics,
+                )?;
             }
 
             symtable.leave_scope();
 
+            res.push(Statement::Block {
+                loc: *loc,
+                unchecked: *unchecked,
+                statements: resolved_stmts,
+            });
+
             Ok(reachable)
         }
         pt::Statement::Break(loc) => {
@@ -2001,7 +2017,7 @@ fn return_with_values(
                     diagnostics,
                     ResolveTo::Type(&return_ty),
                 )?;
-                let expr = expr.cast(loc, &return_ty, true, ns, diagnostics)?;
+                let expr = expr.cast(&expr_return.loc(), &return_ty, true, ns, diagnostics)?;
                 used_variable(ns, &expr, symtable);
                 exprs.push(expr);
             }

tests/contract_testcases/evm/builtins/address_code_01.sol

@@ -7,4 +7,4 @@ contract UpgradeableProxy {
 }
 
 // ---- Expect: diagnostics ----
-// error: 5:9-38: conversion from bytes to uint256 not possible
+// error: 5:16-38: conversion from bytes to uint256 not possible

tests/contract_testcases/polkadot/enums/test_cast_errors.sol

@@ -5,4 +5,4 @@ contract test {
             }
         }
 // ---- Expect: diagnostics ----
-// error: 4:17-33: implicit conversion from enum test.state to uint8 not allowed
+// error: 4:24-33: implicit conversion from enum test.state to uint8 not allowed

tests/contract_testcases/polkadot/functions/mutability_13.sol

@@ -0,0 +1,21 @@
+library StorageSlot {
+    struct AddressSlot {
+        address value;
+    }
+
+    function getAddressSlot(
+        bytes32 slot
+    ) internal pure returns (AddressSlot storage r) {
+        assembly {
+            r.slot := slot
+        }
+    }
+}
+
+contract StorageKeys {
+    function get(bytes32 key) public view returns (address a) {
+        a = StorageSlot.getAddressSlot(key).value;
+    }
+}
+
+// ---- Expect: diagnostics ----

tests/contract_testcases/polkadot/functions/mutability_14.sol

@@ -0,0 +1,15 @@
+contract Foo {
+    address a;
+
+    function r() internal view returns (address) {
+        return msg.sender;
+    }
+
+    function foo() public pure {
+        a = r();
+    }
+}
+
+// ---- Expect: diagnostics ----
+// error: 9:9-10: function declared 'pure' but this expression writes to state
+// error: 9:13-16: function declared 'pure' but this expression reads from state

tests/contract_testcases/polkadot/functions/mutability_15.sol

@@ -0,0 +1,16 @@
+contract C {
+    string s;
+
+    function test(string calldata x) public pure returns (string memory) {
+        s = "foo";
+        s = x;
+        print(s);
+        return s;
+    }
+}
+
+// ---- Expect: diagnostics ----
+// error: 5:9-10: function declared 'pure' but this expression writes to state
+// error: 6:9-10: function declared 'pure' but this expression writes to state
+// error: 7:15-16: function declared 'pure' but this expression reads from state
+// error: 8:16-17: function declared 'pure' but this expression reads from state

tests/contract_testcases/polkadot/value/this_address_01.sol

@@ -5,4 +5,4 @@
             }
         }
 // ---- Expect: diagnostics ----
-// error: 4:17-28: implicit conversion to address from contract b not allowed
+// error: 4:24-28: implicit conversion to address from contract b not allowed

tests/contract_testcases/solana/negative_exponent.sol

@@ -12,4 +12,4 @@ contract c {
 
 // ---- Expect: diagnostics ----
 // warning: 6:16-26: ethereum currency unit used while targeting Solana
-// error: 9:2-20: conversion to uint256 from rational not allowed
+// error: 9:9-20: conversion to uint256 from rational not allowed

tests/evm.rs

@@ -198,6 +198,9 @@ fn ethereum_solidity_tests() {
             // FIXME: others listed explicitly cause panics and need fixing
             if !file_name.ends_with("max_depth_reached_4.sol")
                 && !file_name.ends_with("invalid_utf8_sequence.sol")
+                // Bug in solc: https://github.com/ethereum/solidity/issues/11573
+                && !file_name
+                    .ends_with("internal_library_function_attached_to_string_accepting_storage.sol")
                 && file_name.ends_with(".sol")
             {
                 Some(entry)