Remove wildcard selectors

[ascjones]

One of two possible approaches to solving the proxy selector clashing vulnerability

Motivation

The proxy selector clashing vulnerability depends on the usage of wildcard selectors/fallback functions in proxy contracts. By removing wildcard selectors this vulnerability is eliminated. Contract authors can still implement upgradeability using set_code_hash.

Open questions

• What other common patterns depend on this feature?
• If they cannot be replicated by using set_code_hash, are there alternatives?
• Even if we prevent those patterns being implemented is it worth it to close this vulnerability?

Diamonds/Multi-Facet proxy

As described in https://eips.ethereum.org/EIPS/eip-2535, this pattern of delegating to many "facet" contracts routed by the selector would no longer be possible if we removed wildcard selectors.

[HCastano]

I'm of the opinion that we don't need to have wildcard/fallback functions in ink!. We
should be able to do everything with explicit message calls.

Upgradeable Contracts

The only reason we introduced wildcard selectors was to enable upgradeable contracts
through the proxy pattern (see #739 for more context/history).

This use case has been made obsolete with set_code_hash.

Accepting Native Token Transfers

Unlike Solidity, we also don't use the fallback (or in their case receive) to handle
incoming native token transfers. If we do decide that we want some special mechanics
around this then we can allow a top level payable attribute for contracts.

Extensibility

Lastly, using the fallback as a form of extensibility - like in the Diamond Pattern.

Ignoring the fact that Substrate chains can configure a maximum contract size
much more easily than Ethereum's mainnet (albeit with a performance tradeoff),
I think we can replicate this pattern and while also being more explicit.

Instead of having to implicitly pass call data through the fallback function like we do
now with the use of set_forward_input we can pass this data in explicitly.

For example, we can do something like this:

/// Represents an ink! message.
#[derive(scale::Encode, scale::Decode)]
struct Message {
    selector: Selector,
    input: Input,
    value: Value,
    gas_limit: Gas,
}

/// A regular message which will forward the given `message` to the extension address.
#[ink(message)]
fn forward(&mut self, ext_address: AccountId, message: Message) {
    let code_hash = self.env().code_hash(&ext_address).unwrap();

    // Option 1: Use existing `CallBuilder` pattern
    let Message {selector, input, value, gas_limit} = message;

    build_call::<DefaultEnvironment>()
        .delegate(code_hash)
            .exec_input(
                ExecutionInput::new(Selector::new(selector))
                 .push_arg(input)
        )
        .transferred_value(value)
        .gas_limit(gas_limit)
        .call_flags(
            ink::env::CallFlags::default()
                .set_tail_call(false),
        )
        .returns::<()>()
        .invoke();

    // Option 2: Add support for passing an encoded `Message` directly
    //
    // This would be similar to what how we forward input right now. We'd try and run
    // the provided `Message` with the `code_hash`.
    //
    // Would trap if `Message` failed to decode correctly on dispatch.
    build_call::<DefaultEnvironment>()
        .delegate(code_hash)
        .call_buffer(&message.encode())
        .call_flags(
            ink::env::CallFlags::default()
                .set_tail_call(false),
        )
        .invoke();
}

Pros:

• No "special" ink! messages
• Can still keep state in single contract
• No selector clashing since the forward message is always executed independently of
  the selector in Message

Cons:

• UIs would need to manually encode the call they're interested in
• Nesting calls to extension contracts might get messy
• Kinda looks like a poor man's version of actual Library support

[codecov-commenter]

Codecov Report

Merging #1706 (0a4dcc0) into master (6fb975d) will increase coverage by 0.21%.
The diff coverage is 85.00%.

@@            Coverage Diff             @@
##           master    #1706      +/-   ##
==========================================
+ Coverage   70.76%   70.98%   +0.21%     
==========================================
  Files         206      204       -2     
  Lines        6455     6359      -96     
==========================================
- Hits         4568     4514      -54     
+ Misses       1887     1845      -42     

Impacted Files	Coverage Δ
crates/ink/codegen/src/generator/dispatch.rs	99.50% <ø> (+6.54%)	⬆️
crates/ink/ir/src/ir/item_impl/callable.rs	91.66% <ø> (-0.18%)	⬇️
crates/ink/ir/src/ir/item_mod.rs	91.27% <ø> (+2.13%)	⬆️
crates/ink/ir/src/ir/trait_def/item/trait_item.rs	100.00% <ø> (+1.78%)	⬆️
...tes/ink/tests/ui/contract/pass/message-selector.rs	32.00% <0.00%> (-1.34%)	⬇️
crates/ink/ir/src/ir/attrs.rs	82.82% <87.50%> (+2.05%)	⬆️
crates/ink/ir/src/ir/item_impl/constructor.rs	91.66% <100.00%> (-0.53%)	⬇️
crates/ink/ir/src/ir/item_impl/message.rs	92.30% <100.00%> (-0.48%)	⬇️
crates/ink/ir/src/ir/trait_def/item/mod.rs	88.52% <100.00%> (-0.10%)	⬇️

... and 5 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[ascjones]

Instead of having to implicitly pass call data through the fallback function like we do
now with the use of set_forward_input we can pass this data in explicitly.

As you note in your cons list, this solution means it is no longer a "transparent" proxy, that is clients cannot simply take the metadata of a Logic contract and send messages to the Proxy as if it is the Logic contract (which as you say is solved by set_code_hash for a single Logic contract). So this solution would not be easily be compatible with existing clients.

Ignoring the fact that Substrate chains can configure a maximum contract size
much more easily than Ethereum's mainnet (albeit with a performance tradeoff),

That's true, but for larger contracts you need to pay the proof_size cost, which may be disproportionate if the message you are calling is only very small. So there may be some cost saving benefits to splitting a large contract into many smaller pieces.

Another possible way of implementing extensibility without wildcards/fallbacks using set_code_hash would be to statically define each proxiable message and just forward the call onto the specified code hash. We might need to build that into the language to avoid the payload being decoded/encoded.

#[ink(storage)]
struct Contract {
  logic: Hash,
}

/// generates code to 
#[ink(message(delegate_to = self.logic))]
pub fn transfer(&self, _to: AccountId, _value: Balance) {
  // no body allowed
}

Of course this pattern could introduce other unknown issues, and lacks some of the benefits such as auditability.

In short I don't think we should easily dismiss the Diamond Pattern and similar, from what I understand in my research it is extremely popular on Ethereum and we want to attract those devs.

I agree that it is probably an insecure pattern in general, but I don't think it is up to us to decide what type of contracts should be deployed. We should aim to provide as flexible as possible a language while guiding users to the pit of safety and success.

[ascjones]

Closing in favour of #1708