Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restrict wildcard selectors to have exactly one other message #1708

Merged
merged 36 commits into from
Apr 20, 2023

Conversation

ascjones
Copy link
Collaborator

@ascjones ascjones commented Mar 10, 2023

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 ascjones marked this pull request as ready for review March 30, 2023 10:31
@ascjones
Copy link
Collaborator Author

CI failures fixed in #1733

Copy link
Contributor

@SkymanOne SkymanOne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

codecov-commenter commented Mar 30, 2023

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

@HCastano HCastano added the E-on-ice The issue or PR is currently on ice and not further discussed or developed. label Apr 14, 2023
@ascjones ascjones removed the E-on-ice The issue or PR is currently on ice and not further discussed or developed. label Apr 20, 2023
@ascjones ascjones merged commit 137d18d into master Apr 20, 2023
@ascjones ascjones deleted the aj/restrict-wildcard-selectors branch April 20, 2023 09:00
@SkymanOne SkymanOne mentioned this pull request Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants