Sender-aware deterministic deployment

[guidanoli]

📚 Context

EIP-1014 introduced the CREATE2 instruction to the EVM. It enables a very powerful mechanism called deterministic deployment, which lets contracts deploy other contracts to a deterministically computed address. In fact, you can calculate this address even before deploying the contract. This instruction takes the following parameters as source of entropy for the address of deployment:

• the address of the contract executing the CREATE2 instruction (usually a factory contract)
• a hash salt, which must be pushed onto the stack before executing the function
• the hash of the to-be-deployed contract initialization code

We have been using this mechanism for all of our factories since v0.9.0, and in v1.1.0 we decide to compose factories together to further reduce the number of transactions needed to deploy intertwined contract pairs, while keeping our architecture as modular as possible.

One guarantee that we would like to be true about these factories is the following: given a set of parameters and salt, you are guaranteed that either (1) all contracts were deployed successively, or (2) none have been deployed yet. Recently, however, we discovered a bug that allows anyone to break this guarantee, after knowing the set of parameters.

:lady_beetle: Understanding the attack

Let me demonstrate the attack with an example of the "sunny path" and the "rainy path". We'll use the AuthorityHistoryPairFactory contract as an example. Feel free to consult the source code to fully understand the details.

Below, you can see Alice wants to deploy an Authority-History pair deterministically, with owner as Authority owner, and salt as hash salt. The AuthorityHistoryPairFactory then calls the AuthorityFactory to create a new Authority contract, with the AuthorityHistoryPairFactory being the owner and h(owner, salt) as being the hash salt.

sequenceDiagram
    actor Alice
    participant AHPF as AuthorityHistoryPairFactory (AHPF)
    participant HF as HistoryFactory
    participant AF as AuthorityFactory
    Alice->>AHPF: newAuthorityHistoryPair(owner, salt)
    activate AHPF
    AHPF->>AF: newAuthority(AHPF, H(owner, salt))
    AF-->>AHPF: ✅ authority
    AHPF->>HF: newHistory(authority, salt)
    HF-->>AHPF: ✅ history
    AHPF->>Authority: setHistory(history)
    AHPF->>Authority: transferOwnership(owner)
    deactivate AHPF
    AHPF-->>Alice: ✅ (authority, history)

The attack consists of deploying this Authority contract not through the AuthorityHistoryPairFactory but through the AuthorityFactory directly. As a result, when Alice tries to deploy it through AuthorityHistoryPairFactory, it fails because the address is already occupied.

sequenceDiagram
    actor Alice
    actor Bob
    participant AHPF as AuthorityHistoryPairFactory (AHPF)
    participant HF as HistoryFactory
    participant AF as AuthorityFactory
    Bob->>AF: newAuthority(AHPF, H(owner, salt))
    AF-->>Bob: ✅ authority
    Alice->>AHPF: newAuthorityHistoryPair(owner, salt)
    activate AHPF
    AHPF->>AF: newAuthority(AHPF, H(owner, salt))
    AF-->>AHPF: ❌ Error: address already occupied!!

✔️ Solution

The root of the problem lies on the fact that deterministic deployment is not aware of who called the factory, because the address of the to-be-deployed contract is fully determined by the function parameters.

One solution would be to modify this entry point to add the msg.sender to the entropy of the compound salt, but then we would miss a very powerful guarantee, which is that anyone can call the factory with the right arguments and deploy the contract at the same address. Let's keep this truly permissionless entry point then.

A better, backwards-compatible solution is to add a new entry point that is similar to the current deterministic deployment entry point, but also uses the msg.sender to calculate the hash salt for CREATE2. This entry point would be used by other factory contracts, in order to guarantee that any contract it can deploy can only be deployed through it. This solves the aforementioned problem.

But wait, how do we do it technically? Well, let's say that the hash salt provided to CREATE2 is calculated like this:

bytes32 create2salt = keccak256(abi.encodePacked(deployerAddress, providedSalt));

...where deployerAddress is...

• address(0) for permissionless deterministic deployment.
• msg.sender for permissioned deterministic dpeloyment.

📈 Subtasks

Adapt the contracts, interfaces, and tests of:

• [ ] HistoryFactory
• [ ] AuthorityFactory
• [ ] AuthorityHistoryPairFactory
• [ ] CartesiDAppFactory

[tuler]

If Bob created the same Authority that Alice needs, why doesn't Alice just use it? Bob is making Alice a favor by paying for the deployment.

[guidanoli]

If Bob created the same Authority that Alice needs, why doesn't Alice just use it? Bob is making Alice a favor by paying for the deployment.

Because Bob just created an Authority whose owner is the factory, which is useless for Bob, for Alice, or for anyone, since the factory does not have an entry point for transferring ownership of other contracts. The sole purpose for Bob doing that would be to jam the factory to not go past that point, as the CREATE2 instruction would always revert when it sees there is already a contract in that address.

[guidanoli]

@tuler Your observation is true for "simple" factories, that actually deploy the applications, but for factories that just call other factories, this is a problem, because anyone can call the underlying factory with the same arguments that the factory would.

[tuler]

But the owner param of the Authority also impacts the contract address, right? So if the owner Bob uses is A, and the owner Alice wants is A, use the same contract.

[guidanoli]

Ok, let me go through the attack in greater detail with you. First, let's see the newAuthorityHistoryPair function.

https://github.com/cartesi/rollups-contracts/blob/5f1fccd72cb3fd6bf98ae9f29450bfb2fab85882/onchain/rollups/contracts/consensus/authority/AuthorityHistoryPairFactory.sol#L62-L69

Here, you can see that the AuthorityHistoryPairFactory makes the following call:

authorityFactory.newAuthority(authorityHistoryPairFactory, someSalt);

Don't care too much about how someSalt is calculated here. This salt can be calculated deterministically through _authorityOwner and _salt. Let's suppose Bob knows these two values and can, therefore, calculate someSalt easily.

Imagine that these parameters are agreed upon by a few interested parties, so that, if necessary, they can instantiate these contracts and initiate a dispute on-chain. So, Bob wouldn't have to front-run any tx to get this attack to work.

Now, Bob can call the AuthorityFactory himself directly, and not through AuthorityHistoryPairFactory. By doing so, Bob has created an Authority with no history and owned by the AuthorityHistoryPairFactory (see the first argument passed to newAuthority function).

When Alice tries to call newAuthorityHistoryPair, the function call will revert because the AuthorityFactory will not be able to deploy a contract to an address that already has code in it. This check is performed under-the-hood by the EVM.

What is left is an Authority contract owned by the AuthorityHistoryPairFactory contract. The problem is that the AuthorityHistoryPairFactory contract can never call transferOwnership on it, because control flow will never reach that point due to the call to newAuthorityHistoryPair.

As a result, that set of parameters (_authorityOwner and _salt) are useless. They cannot be used anymore.

[tuler]

the function call will revert because the AuthorityFactory will not be able to deploy a contract to an address that already has code in it. This check is performed under-the-hood by the EVM.

The deployed bytecode + constructor args on that address is exactly the same as if the one created by newAuthorityHistoryPair after line 69, right?

[guidanoli]

The deployed bytecode + constructor args on that address is exactly the same as if the one created by newAuthorityHistoryPair, right?

Yes. I think it's better to think in another way: the newAuthority function from the AuthorityFactory doesn't care who calls it. It will deploy the same contract for the same arguments provided, right?

Now, the AuthorityHistoryPairFactory has no power in gatekeeping a given address for itself, right? Because any call that it does to AuthorityFactory could be replicated by anyone else, given that they provide the exact same arguments, right?

With this, anyone can call newAuthority before AuthorityHistoryPairFactory and now the salt provided to AuthorityFactory is unusable.

[tuler]

Making the deterministic deployment sender-aware doesn't look like the way to go. The design of these factories is strange. The circular dependency of Authority and History is also strange. Don't you think?

[ZzzzHui]

Now, Bob can call the AuthorityFactory himself directly, and not through AuthorityHistoryPairFactory. By doing so, Bob has created an Authority with no history and owned by the AuthorityHistoryPairFactory (see the first argument passed to newAuthority function).

When Alice tries to call newAuthorityHistoryPair, the function call will revert because the AuthorityFactory will not be able to deploy a contract to an address that already has code in it. This check is performed under-the-hood by the EVM.

I don't think this case will revert. The sender addresses for CREATE2 are different, one is AuthorityFactory and the other is AuthorityHistoryPairFactory, so the deterministic deployed addresses are different

[ZzzzHui]

Just compared with the current implementation of compound salt, keccak(msg.sender, _salt) seems like a better compound salt

[ZzzzHui]

Making the deterministic deployment sender-aware doesn't look like the way to go. The design of these factories is strange. The circular dependency of Authority and History is also strange. Don't you think?

To remove this inter-dependency in the factory, maybe we can modify the constructor in the Authority to something like:

    constructor(address _owner, address _history) {
        if( _history == address(0)) { _history = new History(address(this));}
        history = _history;
        ......
    }

Can be modified to deterministically deploy history.

[guidanoli]

The sender addresses for CREATE2 are different

That is my point: msg.sender is currently not being taken into account by factories. For the CREATE2 instruction, only the address of the deployer contract matters for calculating the address of the to-be-deployed contract.

[guidanoli]

Just compared with the current implementation of compound salt, keccak(msg.sender, _salt) seems like a better compound salt

For this purpose, yes. But the current implementation is also interesting, for other reasons! That is why I think we should have two deterministic deployment entry points for factories: one that takes msg.sender into account and another that doesn't.

[guidanoli]

To remove this inter-dependency in the factory, maybe we can modify the constructor in the Authority to something like:

    constructor(address _owner, address _history) {
        if( _history == address(0)) { _history = new History(address(this));}
        history = _history;
        ......
    }

Can be modified to deterministically deploy history.

I don't like this solution. The purpose of the History contract was to decouple the format and storage of claims from the type of consensus. I'd like these things to stay decoupled. If we want to avoid this interdependency, but keep things decoupled, we could have a permissionless History contract, which indexes claims by consensus and by DApp, like this:

//       consensus           dapp       claims
mapping (address => mapping (address => Claim[])) claims;

[pedroargento]

If we have a global History contract shared among all consensus we remove the need of all consensus-history pair factories.

[guidanoli]

Let's list the most relevant changes incurred by this:

IHistory

• :warning: getClaim function will need an extra parameter, the submitter address
• :warning: migrateToConsensus will be removed

History

• :warning: no longer inherits from Ownable contract
• :warning: NewClaimToHistory event will have an extra parameter, the submitter address
• :warning: constructor will no longer have owner parameter
• :warning: Remove migrateToConsensus function
• Change numClaims and claims state variables to be indexed by submitter address as well
• Change submitClaim function to index the aforementioned state variables by msg.sender

IHistoryFactory

• :warning: Removed interface

HistoryFactory

• :warning: Removed contract

Authority

• :warning: Add _history to constructor
• :warning: Remove migrateHistoryToConsensus function
• Call getClaim on History with address(this).

AuthorityFactory

• :warning: Add _history to all functions

AuthorityHistoryPairFactory

• :warning: Removed contract

IAuthorityHistoryPairFactory

• :warning: Removed contract