Custom Selectors and Proxy Selector Clashing Attacks

[ascjones]

I have opened #1634 to remove the custom selector functionality. Edit: now closed pending further decision.

As I understand it, the motivation for doing this is that it enables the following exploit when using the "proxy" contract pattern:

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

/// This method uses a selector the same as one in the "logic" contract,
/// thus intercepting a call intended to be forwarded.
#[ink(message, selector = 0xF23A6E61)]
pub fn innocuous_function_name(&self) {
  // do some malicious stuff
}

This is an instance of the "Proxy selector clashing" exploit raised first by Nomic and expanded upon in this OZ article: Beware of the proxy: learn how to exploit function clashing.

The main difference is that ink! allows custom selectors, whereas in Solidity they can only be derived from function names. The argument is that this makes the attack more likely to succeed because:

1. The malicious contract author does not need to do the extra step of brute forcing a function name with a clashing selector.
2. Can give a malicious function a plausible sounding name with a custom selector.
3. A contract user would be more likely to overlook a malicious function with a plausible sounding name (but a custom selector).

It is objectively easier for the malicious contract author to craft a clashing malicious method with a custom selector, but would not be too difficult to do the same with function names:

Armed with some rust code, we found that clash550254402() has the same selector as proxyOwner(). It took less than 15 minutes to find it in a newish Macbook Pro. A motivated hacker can optimize the process and dedicate much more resources to finding discrete-looking function names.

As for whether a contract user reviewing the proxy contract is more or less likely to notice a malicious method with a clashing selector, that is entirely subjective. One could argue that someone familiar with ink! may notice the use of a custom selector in combination with a wildcard selector and immediately be suspicious, at least enough to read what the function is doing.

So now we have to ask:

• Does this feature make this exploit more likely to succeed? If so by how much?
• Does such an increase in the possibility of this exploit warrant removing the feature entirely?
• How can we mitigate and prevent this attack?

Applying the "precautionary principle" (remove it just in case), doesn't take into account the effect of unintended consequences e.g.:

• What use cases would we break that are already dependent on this feature? (see ERC1155 example)
• Does this feature in other cases allow for "better" security e.g. explicit non-opaque selectors?

Alternatives

Just a couple of ideas as alternatives to removing the feature entirely:

1. Tooling for validating proxy contracts, requires metadata for both proxy and logic contracts, can check for clashing selectors. Can be implemented in cargo-contract or a block explorer e.g. https://etherscan.io/proxyContractChecker
2. Language level safety (brainstorming):

• allow marking a contract as proxy, will only allow two methods: one with wildcard, one for upgrading the contract.
• don't allow custom selectors when a wildcard selector is present.
• remove fallback selectors entirely, they are inherently dangerous!!!!

4. Transparent Proxies: Transparent Proxy zeppelinos/zos-lib#36, maybe enforce that somehow on a language level

[Neopallium]

Why are custom Proxy contracts needed? What extra features would each developer need to add to a Proxy contract?

If there are only a few common Proxy patterns (multi-sig owner, upgrade voting, etc...), and they are published with audits. Then it should be easy to check for clashes between the Proxy and Impl.

Auditing of a deployed contract that uses the Proxy pattern, should include both the Proxy and Implementation code/metadata.

Removing custom selectors wouldn't fix the issue with possible clashing.

Adding language support for Transparent proxy support, could make auditing easier.

[nicolasgarcia214]

@ascjones, the description of the issue is fantastic. I have a couple of things I would like to add:

• The benefits of using custom selectors are minimal. The first one is the ability to change the name of a function while maintaining the same selector. This capability is non-relevant. The second one is to enable the creation of language-agnostic contract standards/interfaces. For this, it might be beneficial to consider alternative solutions to the current approach of allowing the contract developer to set the selector.
• The possibility that there are other issues not yet discovered related to this feature is not minimal.
• Third-party applications that monitor transactions by looking for specific selectors may also have problems with this functionality.

The solutions, in my opinion, can be these:

• Remove the feature and look for an alternative to handle language-agnostic contract standards.
• Require the metadata of the implementation contract to build the proxy and, if selector clashing occurs between the proxy and the implementation, prevent the contract from being compiled.
• Block explorers will need to add a flag in the proxy if they detect unusual functionalities for a proxy and custom selectors in the contract.

Why are custom Proxy contracts needed? What extra features would each developer need to add to a Proxy contract?

Malicious developers can create backdoors or honeypot functionalities to scam average users. The thing with custom selectors is that they enable the possibility to conceal the functionality in a way that is difficult to detect, even for experienced users.

[Neopallium]

Require the metadata of the implementation contract to build the proxy and, if selector clashing occurs between the proxy and the implementation, prevent the contract from being compiled.

This might not cover possible clashing if the implementation is upgraded to a new version that adds new callable functions, unless the set of public callable functions is locked across all versions.

If the implementation contract is not allowed to add new functions in newer versions, then the Proxy contract could be built using the first implementation version. In that case any clashing could give priority to the implementation over the Proxy's functions.

Why are custom Proxy contracts needed? What extra features would each developer need to add to a Proxy contract?

Malicious developers can create backdoors or honeypot functionalities to scam average users. The thing with custom selectors is that they enable the possibility to conceal the functionality in a way that is difficult to detect, even for experienced users.

I meant what are the non-malicious use-cases for a custom Proxy. If there are only a few limited Proxy contract patterns, then those could be audited and any custom Proxy contract could have a big red warning on it (requiring extra auditing).

[yjhmelody]

BTW, I think ink! should support custom function path for selector(at least pin the prefix path). If you rename contract struct or move to new mod(it will happen when code line gets longer), the selector value will be changed.

[ascjones]

only allow two methods: one with wildcard, one for upgrading the contract.

I have fleshed out this idea in IIP-2

[ascjones]

We can remove wildcard selectors entirely, draft PR here: #1706.

So for me it is a choice between

1. IIP-2: Limit contracts with a wildcard selector to one other message #1676 (Restrict wildcard selectors to have exactly one other message #1708): keeps wildcard selectors but limits their use to prevent exploit. A bit more work, but maintains the flexibility of having the feature. Reasonably sure it closes the vulnerability.
2. Remove wildcard selectors #1706: removing them completely. Less work, definitely closes vulnerability. But possibly removes important feature from the language that smart contract devs require? However there is still the possibility of bringing back this feature with 1. in the future.

[ascjones]

One big question we need to answer before moving forward with this, is do we want to support multi-faceted proxies as per https://eips.ethereum.org/EIPS/eip-1538?

The single contract proxy is covered by set_code_hash, but multiple logic contracts with the proxy acting as a router would not work if we removed wildcard selectors. Though there may be workarounds.

[ascjones]

The consultation period is over on this issue and we need to make a decision. To restate the choices are:

1. Remove wildcard selectors Remove wildcard selectors #1706
2. Restrict wildcard selectors Restrict wildcard selectors to have exactly one other message #1708
3. Keep wildcard selectors as is, use tooling and documentation to prevent vulnerability.

I am supporting #1708, on the grounds that:

• it solves the selector clashing vulnerability, which in our case is the main danger of the wildcard selectors.
• it retains the flexibility of allowing to use the wildcard selector/fallback pattern for bypassing the default static dispatch logic, and allow the contract to perform it's own routing.

ink! should be flexible and safe, and I think this solution strikes the correct balance. The cost is a bit of extra complexity.

[ascjones]

Closed by #1708