Restrict external function callable scope (#1312)

Signed-off-by: Lucas Steuernagel <lucas.tnagel@gmail.com>

docs/language/functions.rst

@@ -19,14 +19,31 @@ which are provided in the return statement, the values of the return variables a
 of the function is returned. It is still possible to explicitly return some values
 with a return statement.
 
-Functions which are declared ``public`` will be present in the ABI and are callable
-externally. If a function is declared ``private`` then it is not callable externally,
-but it can be called from within the contract. If a function is defined outside a
-contract, then it cannot have a visibility specifier (e.g. ``public``).
-
 Any DocComment before a function will be include in the ABI. Currently only Substrate
 supports documentation in the ABI.
 
+Function visibility
+___________________
+
+Solidity functions have a visibility specifier that restricts the scope in which they can be called.
+Functions can be declared public, private, internal or external with the following definitions:
+
+    - ``public`` functions can be called inside and outside a contract (e.g. by an RPC). They are
+      present in the contract's ABI or IDL.
+    - ``private`` functions can only be called inside the contract they are declared.
+    - ``internal`` functions can only be called internally within the contract or by any contract
+      inherited contract.
+    - ``external`` functions can exclusively be called by other contracts or directly by an RPC. They
+      are also present in the contract's ABI or IDL.
+
+Both public and external functions can be called using the syntax ``this.func()``. In this case, the
+arguments are ABI encoded for the call, as it is treated like an external call. This is the only way to
+call an external function from inside the same contract it is defined. This method, however, should be avoided
+for public functions, as it will be more costly to call them than simply using ``func()``.
+
+If a function is defined outside a contract, it cannot have a visibility specifier (e.g. ``public``).
+
+
 Arguments passing and return values
 ___________________________________
 

src/sema/contracts.rs

@@ -259,10 +259,10 @@ impl ast::Namespace {
 }
 
 // Is a contract a base of another contract
-pub fn is_base(base: usize, parent: usize, ns: &ast::Namespace) -> bool {
-    let bases = &ns.contracts[parent].bases;
+pub fn is_base(base: usize, derived: usize, ns: &ast::Namespace) -> bool {
+    let bases = &ns.contracts[derived].bases;
 
-    if base == parent || bases.iter().any(|e| e.contract_no == base) {
+    if base == derived || bases.iter().any(|e| e.contract_no == base) {
         return true;
     }
 

src/sema/expression/function_call.rs

@@ -17,7 +17,7 @@ use crate::sema::{builtin, using};
 use crate::Target;
 use solang_parser::diagnostics::Diagnostic;
 use solang_parser::pt;
-use solang_parser::pt::CodeLocation;
+use solang_parser::pt::{CodeLocation, Loc, Visibility};
 use std::collections::HashMap;
 
 /// Resolve a function call via function type
@@ -278,27 +278,8 @@ pub fn function_call_pos_args(
         for (i, arg) in args.iter().enumerate() {
             let ty = ns.functions[*function_no].params[i].ty.clone();
 
-            let arg = match expression(
-                arg,
-                context,
-                ns,
-                symtable,
-                &mut errors,
-                ResolveTo::Type(&ty),
-            ) {
-                Ok(e) => e,
-                Err(_) => {
-                    matches = false;
-                    continue;
-                }
-            };
-
-            match arg.cast(&arg.loc(), &ty, true, ns, &mut errors) {
-                Ok(expr) => cast_args.push(expr),
-                Err(_) => {
-                    matches = false;
-                }
-            }
+            matches &=
+                evaluate_argument(arg, context, ns, symtable, &ty, &mut errors, &mut cast_args);
         }
 
         if !matches {
@@ -309,37 +290,19 @@ pub fn function_call_pos_args(
             continue;
         }
 
-        let func = &ns.functions[*function_no];
-
-        if func.contract_no != context.contract_no && func.is_private() {
-            errors.push(Diagnostic::error_with_note(
-                *loc,
-                format!("cannot call private {}", func.ty),
-                func.loc,
-                format!("declaration of {} '{}'", func.ty, func.name),
-            ));
-
-            continue;
+        match resolve_internal_call(
+            loc,
+            *function_no,
+            context,
+            resolve_to,
+            virtual_call,
+            cast_args,
+            ns,
+            &mut errors,
+        ) {
+            Some(resolved_call) => return Ok(resolved_call),
+            None => continue,
         }
-
-        let returns = function_returns(func, resolve_to);
-        let ty = function_type(func, false, resolve_to);
-
-        return Ok(Expression::InternalFunctionCall {
-            loc: *loc,
-            returns,
-            function: Box::new(Expression::InternalFunction {
-                loc: *loc,
-                ty,
-                function_no: *function_no,
-                signature: if virtual_call && (func.is_virtual || func.is_override.is_some()) {
-                    Some(func.signature.clone())
-                } else {
-                    None
-                },
-            }),
-            args: cast_args,
-        });
     }
 
     match name_matches {
@@ -465,27 +428,8 @@ pub(super) fn function_call_named_args(
 
             let ty = param.ty.clone();
 
-            let arg = match expression(
-                arg,
-                context,
-                ns,
-                symtable,
-                &mut errors,
-                ResolveTo::Type(&ty),
-            ) {
-                Ok(e) => e,
-                Err(()) => {
-                    matches = false;
-                    continue;
-                }
-            };
-
-            match arg.cast(&arg.loc(), &ty, true, ns, &mut errors) {
-                Ok(expr) => cast_args.push(expr),
-                Err(_) => {
-                    matches = false;
-                }
-            }
+            matches &=
+                evaluate_argument(arg, context, ns, symtable, &ty, &mut errors, &mut cast_args);
         }
 
         if !matches {
@@ -495,37 +439,19 @@ pub(super) fn function_call_named_args(
             continue;
         }
 
-        let func = &ns.functions[*function_no];
-
-        if func.contract_no != context.contract_no && func.is_private() {
-            errors.push(Diagnostic::error_with_note(
-                *loc,
-                "cannot call private function".to_string(),
-                func.loc,
-                format!("declaration of function '{}'", func.name),
-            ));
-
-            continue;
+        match resolve_internal_call(
+            loc,
+            *function_no,
+            context,
+            resolve_to,
+            virtual_call,
+            cast_args,
+            ns,
+            &mut errors,
+        ) {
+            Some(resolved_call) => return Ok(resolved_call),
+            None => continue,
         }
-
-        let returns = function_returns(func, resolve_to);
-        let ty = function_type(func, false, resolve_to);
-
-        return Ok(Expression::InternalFunctionCall {
-            loc: *loc,
-            returns,
-            function: Box::new(Expression::InternalFunction {
-                loc: *loc,
-                ty,
-                function_no: *function_no,
-                signature: if virtual_call && (func.is_virtual || func.is_override.is_some()) {
-                    Some(func.signature.clone())
-                } else {
-                    None
-                },
-            }),
-            args: cast_args,
-        });
     }
 
     match function_nos.len() {
@@ -2620,3 +2546,76 @@ pub(crate) fn function_type(func: &Function, external: bool, resolve_to: Resolve
         }
     }
 }
+
+/// This function evaluates the arguments of a function call with either positional arguments or
+/// named arguments.
+fn evaluate_argument(
+    arg: &pt::Expression,
+    context: &ExprContext,
+    ns: &mut Namespace,
+    symtable: &mut Symtable,
+    arg_ty: &Type,
+    errors: &mut Diagnostics,
+    cast_args: &mut Vec<Expression>,
+) -> bool {
+    expression(arg, context, ns, symtable, errors, ResolveTo::Type(arg_ty))
+        .and_then(|arg| arg.cast(&arg.loc(), arg_ty, true, ns, errors))
+        .map(|expr| cast_args.push(expr))
+        .is_ok()
+}
+
+/// This function finishes resolving internal function calls. It returns None if it is not
+/// possible to resolve the function.
+fn resolve_internal_call(
+    loc: &Loc,
+    function_no: usize,
+    context: &ExprContext,
+    resolve_to: ResolveTo,
+    virtual_call: bool,
+    cast_args: Vec<Expression>,
+    ns: &Namespace,
+    errors: &mut Diagnostics,
+) -> Option<Expression> {
+    let func = &ns.functions[function_no];
+
+    if func.contract_no != context.contract_no && func.is_private() {
+        errors.push(Diagnostic::error_with_note(
+            *loc,
+            format!("cannot call private {}", func.ty),
+            func.loc,
+            format!("declaration of {} '{}'", func.ty, func.name),
+        ));
+
+        return None;
+    } else if let (Some(base_no), Some(derived_no)) = (func.contract_no, context.contract_no) {
+        if is_base(base_no, derived_no, ns) && matches!(func.visibility, Visibility::External(_)) {
+            errors.push(Diagnostic::error_with_note(
+                *loc,
+                "functions declared external cannot be called via an internal function call"
+                    .to_string(),
+                func.loc,
+                format!("declaration of {} '{}'", func.ty, func.name),
+            ));
+            return None;
+        }
+    }
+
+    let returns = function_returns(func, resolve_to);
+    let ty = function_type(func, false, resolve_to);
+
+    Some(Expression::InternalFunctionCall {
+        loc: *loc,
+        returns,
+        function: Box::new(Expression::InternalFunction {
+            loc: *loc,
+            ty,
+            function_no,
+            signature: if virtual_call && (func.is_virtual || func.is_override.is_some()) {
+                Some(func.signature.clone())
+            } else {
+                None
+            },
+        }),
+        args: cast_args,
+    })
+}

tests/contract_testcases/solana/functions/external_functions.sol

@@ -0,0 +1,50 @@
+
+function doThis(bar1 bb) returns (int) {
+    // This is allwoed
+    return bb.this_is_external(1, 2);
+}
+
+contract bar1 {
+    constructor() {}
+
+    function this_is_external(int a, int b) external pure returns (int) {
+        return a-b;
+    }
+}
+
+
+contract bar2 is bar1 {
+    constructor() {}
+
+    function hello(int a, int b) external pure returns (int) {
+        return a - b;
+    }
+
+    function test2(int c, int d) external returns (int) {
+        // Not allowed
+        return hello(c, d);
+    }
+
+    function test3(int f, int g) external returns (int) {
+        // Not allowed
+        return hello({b: g, a: f});
+    }
+
+    function test4(int c, int d) public returns (int) {
+        // This is allowed
+        return this.this_is_external(c, d) + this.hello(d, c);
+    }
+
+    function test5(int f, int g) external returns (int) {
+        // Not allowed
+        return this_is_external(f, g);
+    }
+}
+
+// ---- Expect: diagnostics ----
+// error: 25:16-27: functions declared external cannot be called via an internal function call
+// 	note 19:5-61: declaration of function 'hello'
+// error: 30:16-35: functions declared external cannot be called via an internal function call
+// 	note 19:5-61: declaration of function 'hello'
+// error: 40:16-38: functions declared external cannot be called via an internal function call
+// 	note 10:5-72: declaration of function 'this_is_external'

tests/evm.rs

@@ -249,7 +249,7 @@ fn ethereum_solidity_tests() {
         })
         .sum();
 
-    assert_eq!(errors, 1084);
+    assert_eq!(errors, 1079);
 }
 
 fn set_file_contents(source: &str, path: &Path) -> (FileResolver, Vec<String>) {