[verifier] Fix verifier parameter rules. [adapter] Fix visibility che…

…ck (MystenLabs#2051)

* [verifier] Fix verifier parameter rules

- Fixed rules to statically reject entry functions whose signatures are *always* invalid

crates/sui-adapter-transactional-tests/tests/entry_points/wrong_visibility.exp

@@ -0,0 +1,14 @@
+processed 5 tasks
+
+task 1 'publish'. lines 8-24:
+created: object(103)
+written: object(102)
+
+task 2 'run'. lines 26-26:
+Error: Function visibility is invalid for an entry point to execution: "Can only call functions with 'public(script)' visibility".
+
+task 3 'run'. lines 28-28:
+Error: Function visibility is invalid for an entry point to execution: "Can only call functions with 'public(script)' visibility".
+
+task 4 'run'. lines 30-30:
+Error: Function visibility is invalid for an entry point to execution: "Can only call functions with 'public(script)' visibility".

crates/sui-adapter-transactional-tests/tests/entry_points/wrong_visibility.move

@@ -0,0 +1,30 @@
+// Copyright (c) 2022, Mysten Labs, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+// invalid, the adapter should yell that the invoked functions have the wrong visibility
+
+//# init --addresses Test=0x0
+
+//# publish
+module Test::M {
+    use Sui::TxContext::TxContext;
+
+    public fun t1(_: &mut TxContext) {
+        abort 0
+    }
+
+    public(friend) fun t2(_: &mut TxContext) {
+        abort 0
+    }
+
+    fun t3(_: &mut TxContext) {
+        abort 0
+    }
+
+}
+
+//# run Test::M::t1
+
+//# run Test::M::t2
+
+//# run Test::M::t3

crates/sui-adapter/src/adapter.rs

@@ -6,8 +6,9 @@ use anyhow::Result;
 use crate::bytecode_rewriter::ModuleHandleRewriter;
 use move_binary_format::{
     access::ModuleAccess,
+    binary_views::BinaryIndexedView,
     errors::PartialVMResult,
-    file_format::{CompiledModule, LocalIndex, SignatureToken, StructHandleIndex},
+    file_format::{CompiledModule, LocalIndex, SignatureToken, StructHandleIndex, Visibility},
 };
 use sui_framework::EventType;
 use sui_types::{
@@ -73,13 +74,14 @@ pub fn execute<E: Debug, S: ResourceResolver<Error = E> + ModuleResolver<Error =
         })
         .collect();
     let module = vm.load_module(&module_id, state_view)?;
+    let is_genesis = ctx.digest() == TransactionDigest::genesis();
     let TypeCheckSuccess {
         module_id,
         args,
         object_data,
         by_value_objects,
         mutable_ref_objects,
-    } = resolve_and_type_check(&objects, &module, function, &type_args, args)?;
+    } = resolve_and_type_check(&objects, &module, function, &type_args, args, is_genesis)?;
 
     let mut args = args;
     args.push(ctx.to_vec());
@@ -626,6 +628,7 @@ pub fn resolve_and_type_check(
     function: &Identifier,
     type_args: &[TypeTag],
     args: Vec<CallArg>,
+    is_genesis: bool,
 ) -> Result<TypeCheckSuccess, SuiError> {
     // Resolve the function we are calling
     let function_str = function.as_ident_str();
@@ -644,6 +647,16 @@ pub fn resolve_and_type_check(
             })
         }
     };
+    // Check for script visibility, but ignore for genesis.
+    // Genesis calls private functions, and bypasses this rule. This is helpful for ensuring the
+    // functions are not called again later.
+    // In other words, this is an implementation detail that we are using `execute` for genesis
+    // functions, and as such need to bypass this check.
+    if fdef.visibility != Visibility::Script && !is_genesis {
+        return Err(SuiError::InvalidFunctionVisibility {
+            error: "Can only call functions with 'public(script)' visibility".to_string(),
+        });
+    }
     let fhandle = module.function_handle_at(fdef.function);
 
     // check arity of type and value arguments
@@ -922,7 +935,10 @@ fn type_check_struct(
         Err(SuiError::TypeError {
             error: format!(
                 "Expected argument of type {}, but found type {}",
-                sui_verifier::format_signature_token(module, param_type),
+                sui_verifier::format_signature_token(
+                    &BinaryIndexedView::Module(module),
+                    param_type
+                ),
                 arg_type
             ),
         })
@@ -994,7 +1010,8 @@ fn struct_tag_equals_struct_inst(
     param_type: StructHandleIndex,
     param_type_arguments: &[SignatureToken],
 ) -> bool {
-    let (address, module_name, struct_name) = sui_verifier::resolve_struct(module, param_type);
+    let view = BinaryIndexedView::Module(module);
+    let (address, module_name, struct_name) = sui_verifier::resolve_struct(&view, param_type);
 
     // same address, module, name, and type parameters
     &arg_type.address == address

crates/sui-types/src/error.rs

@@ -213,6 +213,8 @@ pub enum SuiError {
     ModuleNotFound { module_name: String },
     #[error("Function signature is invalid: {error:?}.")]
     InvalidFunctionSignature { error: String },
+    #[error("Function visibility is invalid for an entry point to execution: {error:?}.")]
+    InvalidFunctionVisibility { error: String },
     #[error("Type error while binding function arguments: {error:?}.")]
     TypeError { error: String },
     #[error("Execution aborted: {error:?}.")]

crates/sui-verifier-transactional-tests/tests/entry_points/non_key_struct.exp

@@ -0,0 +1,4 @@
+processed 1 task
+
+task 0 'publish'. lines 6-17:
+Error: Failed to verify the Move module, reason: "Invalid entry point parameter type. Expected primitive or object type. Got: _::M::S".

crates/sui-verifier-transactional-tests/tests/entry_points/non_key_struct.mvir

@@ -0,0 +1,17 @@
+// Copyright (c) 2022, Mysten Labs, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+// invalid, non key structs are not supported
+
+//# publish
+module 0x0.M {
+    import 0x2.TxContext;
+
+    struct S has copy, drop, store { value: u64 }
+
+    public(script) no(s: Self.S, ctx: &mut TxContext.TxContext) {
+        label l0:
+        abort 0;
+    }
+
+}

crates/sui-verifier-transactional-tests/tests/entry_points/non_key_struct_generic.exp

@@ -0,0 +1,7 @@
+processed 2 tasks
+
+task 0 'publish'. lines 6-21:
+Error: Failed to verify the Move module, reason: "Invalid entry point parameter type. Expected primitive or object type. Got: _::M::Obj<_::M::NoStore>".
+
+task 1 'publish'. lines 23-35:
+Error: Failed to verify the Move module, reason: "Invalid entry point parameter type. Expected primitive or object type. Got: _::M::Obj<T0>".

crates/sui-verifier-transactional-tests/tests/entry_points/non_key_struct_generic.mvir

@@ -0,0 +1,35 @@
+// Copyright (c) 2022, Mysten Labs, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+// invalid as NoStore doesn't have store, so Obj doesn't have key
+
+//# publish
+module 0x0.M {
+    import 0x2.TxContext;
+    import 0x2.ID;
+
+    struct Obj<T> has key { id: ID.VersionedID }
+    struct NoStore has copy, drop { value: u64 }
+
+    public(script) no(s: Self.Obj<Self.NoStore>, ctx: &mut TxContext.TxContext) {
+        label l0:
+        abort 0;
+    }
+
+}
+
+// valid, while T doesn't have store, and might it later, we require it to be annotated
+
+//# publish
+module 0x0.M {
+    import 0x2.TxContext;
+    import 0x2.ID;
+
+    struct Obj<T> has key { id: ID.VersionedID }
+
+    public(script) no<T>(s: Self.Obj<T>, ctx: &mut TxContext.TxContext) {
+        label l0:
+        abort 0;
+    }
+
+}

crates/sui-verifier-transactional-tests/tests/entry_points/non_key_struct_generic_valid.exp

@@ -0,0 +1,5 @@
+processed 1 task
+
+task 0 'publish'. lines 6-18:
+created: object(103)
+written: object(102)

crates/sui-verifier-transactional-tests/tests/entry_points/non_key_struct_generic_valid.mvir

@@ -0,0 +1,18 @@
+// Copyright (c) 2022, Mysten Labs, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+// valid, T has store, thus Obj has key
+
+//# publish
+module 0x0.M {
+    import 0x2.TxContext;
+    import 0x2.ID;
+
+    struct Obj<T> has key { id: ID.VersionedID }
+
+    public(script) no<T: store>(s: Self.Obj<T>, ctx: &mut TxContext.TxContext) {
+        label l0:
+        abort 0;
+    }
+
+}

crates/sui-verifier-transactional-tests/tests/entry_points/non_key_struct_vector.exp

@@ -0,0 +1,4 @@
+processed 1 task
+
+task 0 'publish'. lines 6-17:
+Error: Failed to verify the Move module, reason: "Invalid entry point parameter type. Expected primitive or object type. Got: vector<_::M::S>".

crates/sui-verifier-transactional-tests/tests/entry_points/non_key_struct_vector.mvir

@@ -0,0 +1,17 @@
+// Copyright (c) 2022, Mysten Labs, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+// invalid, non key structs are not supported, even in vectors
+
+//# publish
+module 0x0.M {
+    import 0x2.TxContext;
+
+    struct S has copy, drop, store { value: u64 }
+
+    public(script) no(s: vector<Self.S>, ctx: &mut TxContext.TxContext) {
+        label l0:
+        abort 0;
+    }
+
+}

crates/sui-verifier-transactional-tests/tests/entry_points/option.exp

@@ -0,0 +1,4 @@
+processed 1 task
+
+task 0 'publish'. lines 6-16:
+Error: Failed to verify the Move module, reason: "Invalid entry point parameter type. Expected primitive or object type. Got: Std::Option::Option<u64>".

crates/sui-verifier-transactional-tests/tests/entry_points/option.mvir

@@ -0,0 +1,16 @@
+// Copyright (c) 2022, Mysten Labs, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+// invalid, non key structs are not supported
+
+//# publish
+module 0x0.M {
+    import 0x2.TxContext;
+    import 0x1.Option;
+
+    public(script) no(s: Option.Option<u64>, ctx: &mut TxContext.TxContext) {
+        label l0:
+        abort 0;
+    }
+
+}

crates/sui-verifier-transactional-tests/tests/entry_points/single_type_param_generic_object.mvir

@@ -8,7 +8,7 @@ module 0x0.M {
     struct Obj<T> has key {
         id: ID.VersionedID,
     }
-    public(script) foo<T>(l: Self.Obj<T>, ctx: &mut TxContext.TxContext) {
+    public(script) foo<T: store>(l: Self.Obj<T>, ctx: &mut TxContext.TxContext) {
         label l0:
         abort 0;
     }

crates/sui-verifier/src/entry_points_verifier.rs

@@ -57,7 +57,7 @@ pub fn verify_module(module: &CompiledModule) -> SuiResult {
 
 /// Checks if this module has a conformant `init`
 fn verify_init_function(module: &CompiledModule, fdef: &FunctionDefinition) -> Result<(), String> {
-    let view = BinaryIndexedView::Module(module);
+    let view = &BinaryIndexedView::Module(module);
 
     if fdef.visibility != Visibility::Private {
         return Err(format!(
@@ -88,7 +88,7 @@ fn verify_init_function(module: &CompiledModule, fdef: &FunctionDefinition) -> R
     match parameters.len() {
         0 => Ok(()),
         1 => {
-            if is_tx_context(&view, &parameters[0]) {
+            if is_tx_context(view, &parameters[0]) {
                 Ok(())
             } else {
                 Err(format!(
@@ -98,7 +98,7 @@ fn verify_init_function(module: &CompiledModule, fdef: &FunctionDefinition) -> R
                     SUI_FRAMEWORK_ADDRESS,
                     TX_CONTEXT_MODULE_NAME,
                     TX_CONTEXT_STRUCT_NAME,
-                    format_signature_token(module, &parameters[0]),
+                    format_signature_token(view, &parameters[0]),
                 ))
             }
         }
@@ -113,7 +113,7 @@ fn verify_entry_function_impl(
     module: &CompiledModule,
     func_def: &FunctionDefinition,
 ) -> Result<(), String> {
-    let view = BinaryIndexedView::Module(module);
+    let view = &BinaryIndexedView::Module(module);
     let handle = view.function_handle_at(func_def.function);
     let params = view.signature_at(handle.parameters);
 
@@ -124,8 +124,10 @@ fn verify_entry_function_impl(
             view.identifier_at(handle.name)
         ));
     }
+
     let last_param = params.0.last().unwrap();
-    if !is_tx_context(&view, last_param) {
+    let last_param_idx = params.0.len() - 1;
+    if !is_tx_context(view, last_param) {
         return Err(format!(
             "{}::{}. Expected last parameter of function signature to be &mut {}::{}::{}, but \
             found {}",
@@ -134,10 +136,14 @@ fn verify_entry_function_impl(
             SUI_FRAMEWORK_ADDRESS,
             TX_CONTEXT_MODULE_NAME,
             TX_CONTEXT_STRUCT_NAME,
-            format_signature_token(module, last_param),
+            format_signature_token(view, last_param),
         ));
     }
 
+    for param in &params.0[0..last_param_idx] {
+        verify_param_type(view, &handle.type_parameters, param)?;
+    }
+
     let return_ = view.signature_at(handle.return_);
     if !return_.is_empty() {
         return Err(format!(
@@ -149,6 +155,43 @@ fn verify_entry_function_impl(
     Ok(())
 }
 
+fn verify_param_type(
+    view: &BinaryIndexedView,
+    function_type_args: &[AbilitySet],
+    param: &SignatureToken,
+) -> Result<(), String> {
+    if is_primitive(param) {
+        return Ok(());
+    }
+
+    if is_object(view, function_type_args, param)? {
+        Ok(())
+    } else {
+        Err(format!(
+            "Invalid entry point parameter type. Expected primitive or object type. Got: {}",
+            format_signature_token(view, param)
+        ))
+    }
+}
+
+fn is_primitive(s: &SignatureToken) -> bool {
+    match s {
+        SignatureToken::Bool
+        | SignatureToken::U8
+        | SignatureToken::U64
+        | SignatureToken::U128
+        | SignatureToken::Address => true,
+        // optimistic
+        SignatureToken::TypeParameter(_) => true,
+        SignatureToken::Signer => false,
+        SignatureToken::Vector(inner) => is_primitive(inner),
+        SignatureToken::Struct(_)
+        | SignatureToken::StructInstantiation(_, _)
+        | SignatureToken::Reference(_)
+        | SignatureToken::MutableReference(_) => false,
+    }
+}
+
 fn is_tx_context(view: &BinaryIndexedView, p: &SignatureToken) -> bool {
     match p {
         SignatureToken::MutableReference(m) => match &**m {