0xc0ffEE - Can not create a pool by cloning strategies on zkSync network

[sherlock-admin2]

0xc0ffEE

high

Can not create a pool by cloning strategies on zkSync network

Can not create pool by cloning strategies on zkSync network because of different behaviors from EVM instructions between zkSync and Ethereum

Vulnerability Detail

When creating pool by cloning strategies, logics inside Clone::createClone(address,uint256) is executed, which calls to ClonesUpgradeable::cloneDeterministic(). Here, OpenZeppelin implements ClonesUpgradeable::cloneDeterministic() to deploy minimal proxy using almost assembly and using create2.

So far, cloning a strategy using library Clone would get failed as zksync docs pointed out

PoC: Check this deployed address on zksync era testnet. Try clone(), we would get failed

Impact

Protocol does not work properly on zksync era: could not create pool by cloning strategies

Code Snippet

https://github.com/allo-protocol/allo-v2/blob/851571c27df5c16f6586ece2a1cb6fd0acf04ec9/contracts/core/Allo.sol#L190 https://github.com/allo-protocol/allo-v2/blob/851571c27df5c16f6586ece2a1cb6fd0acf04ec9/contracts/core/libraries/Clone.sol#L26-L35

Tool used

Manual Review

Recommendation

Consider implementing different pattern for deploying/cloning strategies, such as: Using Factory for each kind of strategy and allow Allo contract to calls to Factory to deploy/clone strategies

Duplicate of #862

[thelostone-mc]

This is understood. We are working with the zksync team to see when Clones would be supported and until then it is understood that the createPool function will not be usable and instead pool creators would have to rely on createPoolWithCustomStartegy

[codenamejason]

It doesn't look like they are going to support this any time soon https://github.com/zkSync-Community-Hub/zkync-developers/discussions/91 and https://github.com/zkSync-Community-Hub/zkync-developers/discussions/97

[nevillehuang]

Escalate

deploy minimal proxy using almost assembly and using create2.

Duplicate of #862.

[sherlock-admin2]

Escalate

deploy minimal proxy using almost assembly and using create2.

Duplicate of #862.

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.

[jkoppel]

Escalate.

This is a duplicate of #862. The issue with #862 is that zkSync Era does not support CREATE2.The issue here is also that zkSync Era does not support CREATE2.

Edit: I misspoke. I meant that it does not support CREATE2 in an EVM-compatible way.

[sherlock-admin2]

Escalate.

This is a duplicate of #862. The issue with #862 is that zkSync Era does not support CREATE2.The issue here is also that zkSync Era does not support CREATE2.

Edit: I misspoke. I meant that it does not support CREATE2 in an EVM-compatible way.

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.

[imsrybr0]

This is a duplicate of #862. The issue with #862 is that zkSync Era does not support CREATE2.The issue here is also that zkSync Era does not support CREATE2.

CREATE2 is supported on zkSync Era.

Difference between this and #862 is that in #862, if the bytecode hash of the anchor is known, you can use CREATE2 to successfully deploy an anchor (check #107).

In this case however, the minimal clone bytecode hash is not known and can't be known (as far as my testing goes) due to the differences in compiling.

Regards

[jkoppel]

I misspoke. I meant that it does not support CREATE2 in an EVM-compatible way.

I am not at all understanding what makes this different from #862. You're saying that the issue could be patched in one case but not the other, but that doesn't make them separate issues.

Compare:

"Issue: You're writing uint128(foo()) in 10 places, and in each of those places it could overflow!" "No, these are 2 separate issues! In 5 of those places, you could rewrite the surrounding function to accomodate a uint256, but in 5 of them you can't."

Still the same issue.

Note further that neither this nor #100 mention this "can't be patched" claim that allegedly makes this different.

[imsrybr0]

Not arguing about patchability, both can be patched, they are just stemming from two different intents and have different impacts / solutions.

862 uses CREATE3 (which uses CREATE2 to deploy a proxy contract) to make sure addresses are similar across multiple chain which is not even possible due to prefixing in address computation in zkSync.

This uses CREATE2 to deploy a minimal clone.

I think these could fall under duplication for unknown bytecode hashes deployment failure (one for the CREATE3 proxy, the other for the minimal clone) but that would be equivalent to marking every overflow as a duplicate no matter where they happen (uint128(foo()), uint64(bar()), ...) what their impact is in your example, no ?

[jkoppel]

I don't see where you're seeing that distinction.

862 just says "CREATE3 not available" and the description says that it's not available because CREATE2 requires statically-known bytecode. Nothing about "to make sure addresses are similar across multiple chains."

Let's look at the things marked as duplicates too:

437: "Can not create profile and update profile on zkSync network because of different behaviors from EVM instructions between zkSync and Ethereum"

866: "There a number of differences between the two chains, and the relevant one here is the calculatio of create2 based addresses."

956: "create, create2, call opcodes mainly work differently on zksync and also create require the bytecode in advance. And the solady create3 utility was not made keeping these things in mind."

387: "However, zkSync Era requires that the bytecode be statically known when CREATE or CREATE2 is used. This protocol has multiple places where it deploys a contract whose bytecode is not statically known"

The last one BTW also points to the same code highlighted by this issue.

I see nothing to support drawing this distinction between #862 and this present issue.

This as best sounds like it's talking about a distinction in the purpose of the code included in this report (which is also included in some variants of #862 such as #387), and the code cited in #862.

"Issue: You're writing uint128(foo()) in 10 places, and in each of those places it could overflow!" "No, these are 2 separate issues! 5 of these places call foo() for reason A, but the other 5 call it for reason B."

Again, this argument does not support marking this as not a duplicate issue.

[imsrybr0]

Quoting from the issue I shared earlier which was marked as duplicate of 862 ;

Recommendation CREATE3 hole purpose is to make contracts deployments on the same addresses easy.

In zkSync Era case, contracts address will be different anyway due to the use of prefixes when computing CREATE and CREATE2 addresses.

https://github.com/matter-labs/era-system-contracts/blob/main/contracts/Constants.sol#L79 https://github.com/matter-labs/era-system-contracts/blob/main/contracts/ContractDeployer.sol#L91

CREATE3 can be replaced on zkSync Era deployments.

As I said, I would understand all of those issues being marked as duplicates under a larger "unknown bytecode hashes deployment failures" root cause. But again, applied to something else like your example, every overflow, no matter the intent / impact, would be marked as a duplicate.

[jkoppel]

I see that you made a comment in your issue about the purpose of the underlying code. That does not magically limit the scope of 5 other people's submissions.

But again, applied to something else like your example, every overflow, no matter the intent / impact, would be marked as a duplicate.

My example is about a specific recurring code pattern that has an overflow. This is about a specific recurring code pattern which does not behave correctly on zkSync Era.

[imsrybr0]

Simply trying to answer your questions. You asked where that distinction was made sharing sharing a couple of examples where it wasn't, I shared one where it was. But if I understand you correctly, all of the submissions needed to mention that or nothing.

The overflow itself can come from different scenarios (user input in foo, a computation in bar, ...) and have different impacts (funds locked, funds stolen, DOS, ...) and have different solutions (validate user input, fix the computation, accommodate for uint256,...).

I highly doubt submissions for these would be marked as duplicates because they are all coming from a uint128 overflow.

[nevillehuang]

Not arguing about patchability, both can be patched, they are just stemming from two different intents and have different impacts / solutions.

862 uses CREATE3 (which uses CREATE2 to deploy a proxy contract) to make sure addresses are similar across multiple chain which is not even possible due to prefixing in address computation in zkSync.

This uses CREATE2 to deploy a minimal clone.

I think these could fall under duplication for unknown bytecode hashes deployment failure (one for the CREATE3 proxy, the other for the minimal clone) but that would be equivalent to marking every overflow as a duplicate no matter where they happen (uint128(foo()), uint64(bar()), ...) what their impact is in your example, no ?

Agree with @jkoppel. Both of the issues root cause is the non-support in a evm compatible way for CREATE2 when deploying contracts.

[imsrybr0]

Agree with @jkoppel. Both of the issues root cause is the non-support for CREATE2 when deploying contracts

CREATE2 is supported though.

[nevillehuang]

Agree with @jkoppel. Both of the issues root cause is the non-support for CREATE2 when deploying contracts

CREATE2 is supported though.

Sorry misspoked, what i meant was non-support based on create2 differences. In fact i argue that in ur submission #100 you are simply stating it does not work on zksync, without identifying the actual root case of the differences in create2.

[imsrybr0]

No worries.

Yep, you can use CREATE2 to deploy contracts in a similar way, the only condition is that the bytecode hash has to be known.

I don't necessarily disagree with this being a duplicate under a single root cause (unknown bytecode hashes deployment failure) but it means discarding the intent / impact / solution.

If that was the case, the line drawn has to be consistent, every issue caused by an overflow should be marked as a duplicate for example, no matter the intent / impact / solution.

[nevillehuang]

What you’re saying is very extreme and incorrect. All it comes down to is root cause —> scenarios. See here

[imsrybr0]

What you’re saying is very extreme and incorrect. All it comes down to is root cause —> scenarios. See here

Which part of it is extreme and incorrect exactly? The overflow section? Yes, I agree, it is extreme and in my opinion incorrect too.

But for me there is no difference between that and marking every unknown bytecode hash deployment failure as a duplicate.

Also, those docs also mention this :

In case the same vulnerability appears across multiple places in different contracts, they can be considered duplicates. The exception to this would be if underlying code implementations, impact, and the fixes are different, then they can be treated separately.

[nevillehuang]

To add, #387 mentions both cases.

Unless other watsons have factual statements to make, my discussion is done here and will leave it to @hrishibhat to decide.

[thongtrungtran]

It seems clear to me that there is a distinction. https://github.com/sherlock-audit/2023-09-Gitcoin-judging/issues/862 is an issue about deterministically deploying contracts This issue is an issue about cloning a contract by using minimal proxies These two issues may be caused by create2's different behaviors in zksync, but they are different in purposes and impacts

[neeksec]

Hi, all. please see the Escalation in https://github.com/sherlock-audit/2023-09-Gitcoin-judging/issues/862#issuecomment-1765822917 and https://github.com/sherlock-audit/2023-09-Gitcoin-judging/issues/862#issuecomment-1766009249.

After re-reading the docs. I think I can agree that this one and #862 should be low because of no fund loss. If you have any objections, please leave comments in #862.

[neeksec]

For the Escalations in this issue, I sugguest to keep the original judgment and maintaining them as non-duplicates.

I agree that the root cause of this issue is identical to #862. And I made them duplicates in my first round sorting.

Later I found only #387 mentions both CREATE3 and Clones among all the findings. Although I wanted to select #387 as the best issue, its report quality is comparatively lower than the others. I was concerned that the sponsor might not fully comprehend the problem and also wanted to ensure that they are aware of all the occurrences. That's why I made them seperated.

[nevillehuang]

If this issues are valid, root cause is the same and should be dupped. If not i am in support of this issues being low severity too to be consistent with the kyberswap judging mentioned.

[jkoppel]

Sherlock rules are clear on this. "In case the same vulnerability appears across multiple places in different contracts, they can be considered duplicates. "

[imsrybr0]

From the same section :

The exception to this would be if underlying code implementations, impact, and the fixes are different, then they can be treated separately.

which is the case for those two issues.

[Evert0x]

Both escalations suggest this issue needs to be duplicate with #862. this judging comment will be limited to that case.

Both issues describe that create2 on zksync is not supported in an EVM-compatible way.

This issue uses the strategy cloning as an example, 862 uses the _generateAnchor as an example.

I don't see a strong argument to consider this a different rootcause.

Planning to accept both escalations (as second escalation added more context) and duplicate with 862

[Evert0x]

Result: Medium Duplicate of #862

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• nevillehuang: accepted
• jkoppel: accepted