[Bug][Compiler]The view tag does not strictly check global mutable variables

[blackyc]

🐛 Bug

I found that only tags are checked in the VM code, and whether the global variables of the smart contract are changed is not checked during compilation. The official documentation also does not clearly state the specific limitations of #[view]. It may cause ambiguity to all developers, and the view function of the smart contract is in an unexpected state.

https://github.com/aptos-labs/aptos-core/blob/main/aptos-move/aptos-vm/src/verifier/view_function.rs#L40-47

pub fn determine_is_view(
    module_metadata: Option<&RuntimeModuleMetadataV1>,
    fun_name: &IdentStr,
) -> bool {
    if let Some(data) = module_metadata {
        data.fun_attributes
            .get(fun_name.as_str())
            .map(|attrs| attrs.iter().any(|attr| attr.is_view_function()))
            .unwrap_or_default()
    } else {
        false
    }
}

/// Validate view function call. This checks whether the function is marked as a view
/// function, and validates the arguments.
pub(crate) fn validate_view_function(
    session: &mut SessionExt,
    module_storage: &impl AptosModuleStorage,
    args: Vec<Vec<u8>>,
    fun_name: &IdentStr,
    func: &LoadedFunction,
    module_metadata: Option<&RuntimeModuleMetadataV1>,
    struct_constructors_feature: bool,
) -> PartialVMResult<Vec<Vec<u8>>> {
    // Must be marked as view function.
    let is_view = determine_is_view(module_metadata, fun_name);
    if !is_view {
        return Err(
            PartialVMError::new(StatusCode::INVALID_MAIN_FUNCTION_SIGNATURE)
                .with_message("function not marked as view function".to_string()),
        );
    }

The following code snippet passed and executed in my test.

#[view]
    /// Calculates and returns the amount of rewards earned by a user for a specific pool and reward token that can be claimed
    public fun get_earned(user: address, pool: Object<StakingPool>, reward_token: Object<Metadata>): u64 acquires MultiRewardsModule, StakingPool, UserData {

        if (!exists<UserData>(user)) {
            return 0
        };

        let user_data = borrow_global<UserData>(user);

        if (!table::contains(&user_data.pool_rewards, pool)) {
            return 0
        };

        // NOTE: we don't need to check if the user's pool_rewards has the reward_token
        // as the total_earned() function will add it if it doesn't exist

        total_earned(&pool, user, reward_token)
}

    fun total_earned(
        pool_obj: &Object<StakingPool>, account: address, reward_token: Object<Metadata>
    ): u64 acquires MultiRewardsModule, StakingPool, UserData {
      ....Not important code
        // Check if user has reward data for this pool and token
        if (!table::contains(pool_rewards, reward_token)) {
            table::add(
                pool_rewards,
                reward_token,
                UserRewardData { reward_per_token_paid: 0, rewards: 0 }
            );
        };

Simple explanation: The current amount that can be claimed is returned in the defined #[view] function get_earned. However, if the user data does not exist, it is added to the table. In fact, the on-chain interface of the smart contract accesses the #[view] function to change the variable state and passes the test, which causes the function to be ambiguous.

Expected Behavior

In the view function, global variables will not be changed, only the contract status is read and a read-only return value is returned.

Additional context

Found during the smart contract related audit.

[davidiw]

We can amend the documentation to make it clear that anything executed via a view call does not affect persistent storage and all changes that occur during this call are dropped at the conclusion of the call.

[davidiw]

Note, leaving a view function as public and mutating state, may risk opening a backdoor into the contract. We use view functions in various modules to expose the innards of data structures, so one need not know the actual layout. Much like accessors in other languages.

[blackyc]

We can amend the documentation to make it clear that anything executed via a view call does not affect persistent storage and all changes that occur during this call are dropped at the conclusion of the call.

Note that there are indeed different meanings and deviations between the document and the actual implementation. From your response, it is described that the view function call will not affect any persistent storage, which may be a conflict. I look forward to your investigation and response.

[rahxephon89]

Hi @blackyc, this doc may give a better description of what #[view] means and when calling it will not change global state: https://aptos.dev/en/build/apis/fullnode-rest-api#reading-state-with-the-view-function.

[blackyc]

Thank you very much. I think this may help many developers avoid this risk in their aptos move contracts. Currently, it can only be guaranteed by the developers themselves. If compilation checks can be added to aptos move test in the future, such as compiling checks for state modification in view functions in solidity, it will be better. From a security perspective, if the contract cannot be guaranteed at the compilation level, when the developer does not comply with the specifications, it may lead to the risk of "read-only modification" contracts.