Messages with an immutable receiver should not be allowed to change state via `ink_env`

[xermicus]

As per our docs:

An ink! message with a &self receiver may only read state whereas an ink! message with a &mut self receiver may mutate the contract's storage.

However this isn't enforced: Via the Environment, supposedly immutable contracts can change the state in many ways. Things that are currently possible in immutable contracts that should fail to compile:

• writing to storage
• emitting events
• calling chain extensions
• call_runtime
• instantiating contracts
• calling contracts
• terminate
• transfer value

Minimal reproducer

While direct storage writes are enforced by the &self receiver, this can easily be circumvented.

#![cfg_attr(not(feature = "std"), no_std, no_main)]

#[ink::contract]
mod mutable {
    use ink::storage::traits::ManualKey;
    use ink::storage::Lazy;

    /// A contract for testing `Mapping` functionality.
    #[ink(storage)]
    #[derive(Default)]
    pub struct Mutable {}

    impl Mutable {
        #[ink(constructor)]
        pub fn new() -> Self {
            Self {}
        }

        /// This function manipulates state despite receiving `&self`
        #[ink(message)]
        pub fn read_only(&self, a: u8) {
            ink::env::set_contract_storage(&a, &a);
            // The same just with an added layer of inderiction
            Lazy::<u8, ManualKey<127>>::new().set(&127);
        }

        #[ink(message)]
        pub fn get(&self, a: u8) -> (Option<u8>, Option<u8>) {
            (
                ink::env::get_contract_storage(&a).unwrap(),
                ink::env::get_contract_storage(&127).unwrap(),
            )
        }
    }
}

[SkymanOne]

Not a bug but a feature XD

[xermicus]

Not a bug but a feature XD

As discussed elsewhere.

I disagree. Because it is hard to impossible to know inside our macros whether any state mutating API is called downstream. We should not pretend that we actually know this. And talk about it in the docs, and even include a mutates property in the metadata, pretending that we have knowledge about the mutability of our messages, when in fact, we can't reliably determine this.

solc even prohibits state changes in view functions on an assembly level. It makes us look bad if we pretend to have the concept of view functions.

We simply shouldn't sell guarantees we can't fulfill, especially if they can have security implications. What do we gain from having this in the metadata when it is in fact completely meaningless? In the current state, the only thing "immutable message" means is that the storage can't be written directly (but via a plethora of other ways, some of them intended).

Which is very missleading.

However, while currently not implemented, we could enforce this in the runtime. Akin to a staticcall in the EVM: If the message dispatcher (the dispatcher knows which messages are supposed to be immutable) could flag to the runtime that from that point on, the contract want's the execution to revert upon calling into any state changing runtime API. The contract would still compile, however not work which would be an improvement and nullify the security concerns. Lints around this could additionally be provided.

[SkymanOne]

As discussed before, I think we should restrict the direct usage of ink::env crate, and encapsulate Lazy and Mapping storage mutations with a higher-level interface that can infer mutability.

[xermicus]

As discussed before, I think we should restrict the direct usage of ink::env crate,

This doesn't solve the issue fully, but would be helpful in a lint.

Bottom line is we rely on the clients to do this correctly. The contracts-ui already honors it and fixed it in cargo contract. Additionally I am going to implement a static call flag in the contracts pallet so that contract to contract calls can have this guarantee.