Restrict wildcard selectors to have exactly one other message

[ascjones]

Implementation of IIP-2 , one of the possible solutions to #1643.

This PR tightens the usage of wildcard selectors, requiring exactly one other message with a well-known reserved selector to be defined.

/// 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
}

/// One other message allowed to handle messages.
/// Fails to compile unless `selector = @` is used.
/// This MUST be specified along with a wildcard selector.
/// I've just used `@` for now as a complement to `_`
#[ink(message, selector = @)]
pub fn handler(&self, msg: ProxyMessage) {
    match msg {
        ProxyMessage(hash) => ...
    }
}

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

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

It is instructive to visualise the message dispatch as a match statement (indeed this is what is generated by ink!). Currently when no wildcard is used it looks like:

match msg.selector {
  0x00000001 => /* decode args then invoke msg_1 */
  0x00000002 => /* decode args then invoke msg_2 */ 
}

This is an exhaustive match statement, so we can guarantee that the message will be handled by the target contract.

Wildcard selectors (aka fallback functions) are often used to bypass this built in static message dispatch, and forward/delegate messages to other contracts. So in the presence of a wildcard the match statement will become non-exhaustive:

match msg.selector {
  0x00000001 => /* decode args then invoke msg_1 */
  0x00000002 => /* decode args then invoke msg_2 */ 
  _ => /* no matching selector, invoke wildcard */
}

It is easy to see here, that the contract will first try and match a local selector, before falling back to the wildcard selector. This is where the selector clashing vulnerability manifests, the proxy contract can intercept a message intended to be routed by the wildcard.

What this PR does, for the second example when using a wildcard selector, the match statement will always have only two branches:

match msg.selector {
  ::ink::IIP2_WILDCARD_COMPLEMENT_SELECTOR => /* decode args then invoke wildcard complement*/
  _ => /* invoke wildcard */
}

By restricting the wildcard selector to having exactly one other message with a fixed selector, we have the guarantee that a message intended for the wildcard handler cannot be intercepted by another locally defined message.

todo

• decide whether to require the user to always define the other message complementing the wildcard, or to default to throwing an error or passing through to the wildcard handler.
• define well-known reserved constant for the other message required when using a wildcard.
• prevent this well-known reserved selector being used for normal messages.

[ascjones]

CI failures fixed in #1733

[SkymanOne]

I personally believe that we should favour this PR in contrast to #1706
The reason is that emerging contracts pattern like diamond proxy pattern may rely on wildcard selector. While we do solve the problem of upgradability with set_code_hash by removing the selector we would harm the modularity of ink! smart contracts.

Restricting wildcard selector ensures that we close the vulnerability while maintaining the support for proxy patterns.

[codecov-commenter]

Codecov Report

Merging #1708 (5e9b2fc) into master (9dfc631) will increase coverage by 0.16%.
The diff coverage is 91.17%.

❗ Current head 5e9b2fc differs from pull request most recent head f9a6901. Consider uploading reports for the commit f9a6901 to get more accurate results

@@            Coverage Diff             @@
##           master    #1708      +/-   ##
==========================================
+ Coverage   70.74%   70.90%   +0.16%     
==========================================
  Files         207      206       -1     
  Lines        6617     6493     -124     
==========================================
- Hits         4681     4604      -77     
+ Misses       1936     1889      -47     

Impacted Files	Coverage Δ
crates/e2e/src/lib.rs	0.00% <ø> (ø)
crates/ink/ir/src/ast/attr_args.rs	100.00% <ø> (ø)
crates/ink/ir/src/ir/item_impl/callable.rs	91.83% <ø> (-0.17%)	⬇️
...tes/ink/tests/ui/contract/pass/message-selector.rs	33.33% <ø> (+1.19%)	⬆️
...ests/ui/contract/pass/message-wildcard-selector.rs	16.66% <ø> (+4.16%)	⬆️
crates/ink/ir/src/ir/item_impl/constructor.rs	89.06% <33.33%> (-3.48%)	⬇️
crates/ink/ir/src/ir/attrs.rs	80.69% <84.61%> (+0.13%)	⬆️
crates/ink/ir/src/ast/meta.rs	86.66% <87.50%> (+1.18%)	⬆️
crates/e2e/macro/src/config.rs	84.84% <100.00%> (ø)
crates/ink/ir/src/ir/config.rs	100.00% <100.00%> (ø)
... and 5 more

... and 63 files with indirect coverage changes

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