pengun - CREATE3 is not available in the zkSync Era.

[sherlock-admin]

pengun

high

CREATE3 is not available in the zkSync Era.

In the current code using CREATE3 with CREATE2, but in zkSync Era, CREATE2 for arbitrary bytecode is not available, so a revert occurs in the CREATE3.deploy process.

Vulnerability Detail

According to the contest README, the project can be deployed in zkSync Era. (https://github.com/sherlock-audit/2023-09-Gitcoin/blob/main/README.md?plain=1#L11)

The zkSync Era docs explain how it differs from Ethereum.

The description of CREATE and CREATE2 (https://era.zksync.io/docs/reference/architecture/differences-with-ethereum.html#create-create2) states that Create cannot be used for arbitrary code unknown to the compiler.

POC:

// SPDX-License-Identifier: Unlicensed
pragma solidity ^0.8.0;

import "./MiniContract.sol";
import "./CREATE3.sol";

contract DeployTest {
    address public deployedAddress;
    event Deployed(address);

    function generateContract() public returns(address, address) {
        bytes32 salt = keccak256("SALT");

        address preCalculatedAddress = CREATE3.getDeployed(salt);

        // check if the contract has already been deployed by checking code size of address
        bytes memory creationCode = abi.encodePacked(type(MiniContract).creationCode, abi.encode(777));

        // Use CREATE3 to deploy the anchor contract
        address deployed = CREATE3.deploy(salt, creationCode, 0);
        return (preCalculatedAddress, deployed);
    }
}

You can check sample POC code at zkSync Era Testnet(https://goerli.explorer.zksync.io/address/0x0f670f8AfcB09f4BC509Cb59D6e7CEC1A52BFA51#contract)

Also, the logic to compute the address of Create2 is different from Ethereum, as shown below, so the CREATE3 library cannot be used as it is.

This cause registry returns an incorrect preCalculatedAddress, causing the anchor to be registered to an address that is not the actual deployed address.

address ⇒ keccak256( 
    keccak256("zksyncCreate2") ⇒ 0x2020dba91b30cc0006188af794c2fb30dd8520db7e2c088b7fc7c103c00ca494, 
    sender, 
    salt, 
    keccak256(bytecode), 
    keccak256(constructorInput)
 ) 

Impact

generateAnchor doesn't work, so user can't do anything related to anchor.

Code Snippet

https://github.com/allo-protocol/allo-v2/blob/851571c27df5c16f6586ece2a1cb6fd0acf04ec9/contracts/core/Registry.sol#L350 https://github.com/allo-protocol/allo-v2/blob/851571c27df5c16f6586ece2a1cb6fd0acf04ec9/contracts/core/Registry.sol#L338

Tool used

Manual Review

Recommendation

This can be solved by implementing CREATE2 directly instead of CREATE3 and using type(Anchor).creationCode. Also, the compute address logic needs to be modified for zkSync.

[0xdeadbeef0x]

Escalate

I argue that this and #411 show be low since there is no loss of funds. While this impacts the functionality of the protocol on zkSync, no funds are at risk.

Also, createPoolWithCustomStrategy can be used instead - see comment https://github.com/sherlock-audit/2023-09-Gitcoin-judging/issues/411#issuecomment-1757462133

[sherlock-admin2]

Escalate

I argue that this and #411 show be low since there is no loss of funds. While this impacts the functionality of the protocol on zkSync, no funds are at risk.

Also, createPoolWithCustomStrategy can be used instead - see comment https://github.com/sherlock-audit/2023-09-Gitcoin-judging/issues/411#issuecomment-1757462133

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[neeksec]

Agree with Escalation that this one and #411 is low.

Re-read the docs. DoS without fund loss is not counted as medium.

Could Denial-of-Service (DOS), griefing, or locking of contracts count as a Medium (or High) issue? It would not count if the DOS, etc. lasts a known, finite amount of time <1 year. If it will result in funds being inaccessible for >=1 year, then it would count as a loss of funds and be eligible for a Medium or High designation.

Also checked a similar issue in Kyberswap. Can agree with the Escalation.

[imsrybr0]

Both of this and #411 break core contract functionality.

EDIT: Also, createPoolWithCustomStrategy cannot be used as it requires a valid profile which cannot be created because of this issue

[jacksanford1]

https://github.com/allo-protocol/allo-v2/pull/387

[Evert0x]

If I understand correctly the complete zksync era deployment will be useless because of this.

https://docs.sherlock.xyz/audits/judging/judging#v.-how-to-identify-a-medium-issue

Breaks core contract functionality, rendering the contract useless

I believe this creates a very strong argument for Medium severity.

Also checked https://github.com/sherlock-audit/2023-07-kyber-swap-judging/issues/26#issuecomment-1711944172.

KyberSwap didn't explicitly tag zksync era as an EVM chain they were planning to deploy on.

Planning to reject escalation and keep medium.

[0xdeadbeef0x]

@Evert0x

I would appreciate to not reject the escalation because up until now only issues that result in loss of funds would get accepted by Sherlock (hence my reasoning for escalating this)

It was based on previous contests and for the clear definition in Sherlock docs: https://docs.sherlock.xyz/audits/judging/judging#ii.-criteria-for-issue-severity There is a viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost.

Even in the DOS rule it mentions there has to be loss of funds

[0xRizwan]

KyberSwap didn't explicitly tag zksync era as an EVM chain they were planning to deploy on.

@Evert0x That kyberswap report was submitted by me. It is supposed to deploy and will be deployed on zkSync Era, I would just draft a detail response on kyberswap issue. To be noted, I am in discussion on the kyber swap issue with Hrishi and Jack from last 2 weeks.

[jack-the-pug]

https://github.com/allo-protocol/allo-v2/pull/387/files#diff-342c707b787edfadc9b07ed13856c8915c85b47c3bb10e7190b005f6fd177e50R348-R350

@0xKurt I believe that the way the address is computed in the updated version still returns an incorrect preCalculatedAddress on zkSync Era.

[0xKurt]

we will look into it @jack-the-pug. If you have any suggestions for us, feel free to drop them. cc @codenamejason @thelostone-mc

[Evert0x]

Result: Medium Has Duplicates

Even in the DOS rule it mentions there has to be loss of funds This not about the interpretation of the DOS rule but about the How to identify a medium issue rule.

Besides the reasons mentioned earlier. The following is also a key rule in the judging

Hierarchy of truth: Contest README > Sherlock rules for valid issues > Historical decisions.

Although the correct language is missing the intention of this sentence is that the protocol can thrive in the context defined by the protocol team. We will update to language to make this clear for future contests.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• 0xdeadbeef0x: rejected