IIP-2: Limit contracts with a wildcard selector to one other message

[ascjones]

Abstract

Wildcard selectors used in proxy patterns are vulnerable to malicious or accidental selector clashing, causing the Proxy contract to intercept a message intended for a Logic contract. This proposal aims to eliminate this vulnerability by restricting an ink! contract with a wildcard selector to only having one other message with a well-known reserved selector.

Motivation

Wildcard selectors can be used to annotate a so called "fallback" function which handles any message with a selector which does not match any of the other selectors in an ink! smart contract. These can be used to implement common proxy patterns for contract upgradeability. Users send messages to a Proxy contract which forwards/delegates messages to a Logic contract via the fallback message handler e.g.

/// Handles any message with no matching selector in this proxy contract
#[ink(message, payable, selector = _)]
pub fn fallback(&self) {
  // forward call to the "logic" contract which actually executes the call
}

However this introduces a vulnerability whereby the Proxy contract may define a message with a selector identical to one from the Logic contract. In this case the Proxy would handle the message rather than it being passed on to the Logic contract. This is known as Proxy Selector Clashing, described in detail in this OpenZeppelin post.

Because the Logic contract can be upgraded dynamically, there is no way to guarantee at compile time whether a selector clash exists.

Specification

The proposal is to restrict contracts with a wildcard selector to only have one other message with a reserved/well known selector. This guarantees that there are no selector clashes, either by chance or malicious intent, and that the Proxy will only handle messages intended for it.

If a contract uses a wildcard selector #[ink(message, payable, selector = _)] it MAY define one other message. This message MUST use the reserved selector ::ink::IIP2_HANDLER_SELECTOR. This selector MUST only be used in conjunction with a wildcard selector.

/// Handles any message with no matching selector in this proxy contract
#[ink(message, selector = _)]
pub fn fallback(&self) {
  // forward call to the "logic" contract which actually executes the call
}

#[derive(Decode)]
pub enum ProxyMessage {
    UpgradeContract(Hash),
}

/// One other message allowed to handle messages.
/// Fails to compile unless `IIP1_HANDLER_SELECTOR` is used.
#[ink(message, selector = ::ink::IIP2_HANDLER_SELECTOR)]
pub fn handler(&self, msg: ProxyMessage) {
    match msg {
        ProxyMessage(hash) => ...
    }
}

/// An additional message. Fails to compile when uncommented.
// #[ink(message)]
// pub fn additional_message(&self, msg: ProxyMessage) {
//    match msg {
//        ProxyMessage(hash) => ...
//    }
// }

Considerations

The goal is to introduce this restriction in order to eliminate the selector clashing vulnerability, but at the same time retain enough flexibility for contract authors to use the wildcard selector for different proxy patterns and any other usage.

The tradeoff is that contracts using wildcard selectors won't look exactly like idiomatic ink! contracts, because the other non-handler message will have to implement it's own inner dispatch logic most likely using an enum (see example).

Rationale

By constraining an ink! contract using a wildcard selector to only having a single well known selector, we can provide compile-time guarantees for that a Proxy and Logic contract written in ink! will not have clashing selectors.

[SkymanOne]

Closing issue as completed by #1708

[forgetso]

Why is the proxy contract only allowed one other function? In our proxy contract we have several useful functions (below) that allow us to withdraw from the proxy, set the code hash of the proxy, terminate the proxy, etc.. Are we to handle these in one giant function now?

https://github.com/prosopo/captcha/blob/da880924b5cc1a05ee0f9bd62d28bef502a352f1/protocol/contracts/proxy/src/lib.rs#L70-L141

[forgetso]

The tradeoff is that contracts using wildcard selectors won't look exactly like idiomatic ink! contracts, because the other non-handler message will have to implement it's own inner dispatch logic most likely using an enum (see example).

I get it now. We have one public dispatching function that calls private functions that implement withdraw, set_code_hash, terminate, etc. Makes sense!

[ascjones]

The tradeoff is that contracts using wildcard selectors won't look exactly like idiomatic ink! contracts, because the other non-handler message will have to implement it's own inner dispatch logic most likely using an enum (see example).

I get it now. We have one public dispatching function that calls private functions that implement withdraw, set_code_hash, terminate, etc. Makes sense!

Precisely: define an enum with variants that can then dispatch to the other private fns. As I said it is a tradeoff, but I think a minor inconvenience to close the vulnerability, and most contracts will not be proxy contracts with a wildcard.

[forgetso]

Thanks for confirming. I've made the changes in our proxy contract and got nearly all of the tests passing using the new method. There is one bug in that terminate now fails during testing.

https://github.com/prosopo/captcha/blob/0be68d87a89f5add26579078cf57e64dc66fc656/protocol/contracts/proxy/src/lib.rs#L306-L336

thread 'proxy::tests::test_proxy_terminate' panicked at 'Box<dyn Any>', /home/user/.cargo/git/checkouts/ink_sr25519-548109b1b8e8003f/8866fdb/crates/engine/src/ext.rs:318:9
stack backtrace:
   0: std::panicking::begin_panic
             at /rustc/8ede3aae28fe6e4d52b38157d7bfe0d3bceef225/library/std/src/panicking.rs:625:12
   1: std::panic::panic_any
             at /rustc/8ede3aae28fe6e4d52b38157d7bfe0d3bceef225/library/std/src/panic.rs:63:5

I know terminate is meant to panic but it seems the introduction of the function dispatch enum and the closure has confused ink::env::test::assert_contract_termination.