MohammedRizwan - deploy() will not work on zkSync with current implementation

[sherlock-admin2]

MohammedRizwan

medium

deploy() will not work on zkSync with current implementation

Summary

Vulnerability Detail

Impact

Per the contest page, Kyber-swap protocol is intended to deployed on below chains

Mainnet, BNB Chain, Polygon, Arbitrum, Optimism, Avalanche, Fantom, Linea, Polygon zkEVM, Base, BitTorrent, Cronos, Velas, Oasis (and could be any EVM compatible chains)

As stated in contest Read, (and could be any EVM compatible chains) We are discussing an issue with zksync era with regards to implementation of deploy() in CodeDeployer.sol. Which can be seen below(comments not shown below due to simplicity)

File: ks-elastic-sc/contracts/libraries/CodeDeployer.sol

>>  function deploy(bytes memory code) internal returns (address destination) {
    bytes32 deployerCreationCode = _DEPLOYER_CREATION_CODE;

    assembly {
      let codeLength := mload(code)

      mstore(code, deployerCreationCode)

>>      destination := create(0, code, add(codeLength, 32))

      mstore(code, codeLength)
    }

    assert(destination != address(0));
  }

According to ZKSync Era's documentation, the bytecode of a contract must be known beforehand by the compiler for the CREATE/CREATE2 opcode to work:

To guarantee that create/create2 functions operate correctly, the compiler must be aware of the bytecode of the deployed contract in advance. The compiler interprets the calldata arguments as incomplete input for ContractDeployer, as the remaining part is filled in by the compiler internally.

However, in deploy() fucnction of CodeDeployer.sol contract, the contract's bytecode is provided by the caller as an argument. It can be seen the bytes code,

  function deploy(bytes memory code) internal returns (address destination) {

is directly passed in create()

      destination := create(0, code, add(codeLength, 32))

Therefore, deploy() function will not work on ZKSync due to the following official reason by zkSync.

It is to be noted here that deploy() has been used in BaseSplitCodeFactory.sol constructor here. Therefore the issue must be fixed.

Code Snippet

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/main/ks-elastic-sc/contracts/libraries/CodeDeployer.sol#L71

Tool used

Manual Review

Recommendation

Refer the zkSync documentation to take the create() design consideration for zkSync as explained above.

[sherlock-admin]

2 comment(s) were left on this issue during the judging contest.

Trumpero commented:

low, it will not causes loss of funds

jkoppel commented:

The outcome is that this would be discovered as soon as KyberSwap tried to deploy on zkSync. They would be unable to create a Pool if not unable to compile and would need to edit the code to do so. No pools would be created and no funds would be lost.

[0xRizwan]

@Evert0x @jacksanford1

With due respect, I am reopening this issue as i feel it judged incorrectly. Please refer below additional context.

This was probably the first ever issue submitted on sherlock platform on zksync Era create/create2 compiler issue. It was judged as low severity citing it will not causes loss of funds. I had dicussion with Trumpero kyber swap judge where i explained this issue fulfills the sherlock rule of Medium severity but he stressed on loss of funds despite the issue is causing Dos, Breaks core contract functionality, rendering the contract useless.

In Allo contest, similar issue on create2 was submitted as Medium severity, later it got escalated being low citing this issue as reference, now when the escalation is resolved by @Evert0x It is confirmed and concluded as Medium severity. Allo issue can be checked here

I found a comment of @Evert0x on Allo issue saying,

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

This is not completely true. The readme states,

Mainnet, BNB Chain, Polygon, Arbitrum, Optimism, Avalanche, Fantom, Linea, Polygon zkEVM, Base, BitTorrent, Cronos, Velas, Oasis (and could be any EVM compatible chains)

Now, see this line (and could be any EVM compatible chains). This is also the part of readme.

zksync Era is an EVM compatible chain.

When i checked the kyberswap docs, zksync chain can be seen as supported and deployed network. This can be checked here

I politely request to consider the issue as Medium severity. This is completely a judging mistake and this could be due to consider a Medium issue where only loss of funds happens but thanks to @jacksanford1 for clarifying it here.

[Trumpero]

@0xRizwan Hi, I believe you are misunderstanding. There were multiple reasons that lead me to categorize your issue in Kyberswap as low priority. First, it doesn't have any impact since all contracts of Kyberswap Elastic will be deployed by the 'deploy()' in your report. It will not work in zkSync, and the entire protocol is not able to be deployed in zkSync with the current codebase. Therefore, this issue can't cause any loss or harm to the protocol. Secondly, the readme of the contest doesn't specify zkSync in the list of chains. The phrase 'could be any EVM compatible chains' implies potential, not certainty, of other chains. Hence, I believe being unable to deploy the current codebase on zkSync doesn't have an impact on the protocol. The last reason is that I discussed this issue with Kyber's protocol team, and they informed me that they were already aware of the trouble of their current codebase in zkSync, so they removed zkSync from the expected chains.

If you compare your issue in the Kyber Elastic contest with the issue in the Allo v2 contest, there are several differences. The readme of the Allo contest explicitly specifies zkSync Era in the list of chains, so issues concerning the zkSync chain will have a significant impact. Additionally, I believe the issue in the Allo v2 contest affects the Registry contract's ability to generate an anchor for users, while the rest of the protocol is still able to work in zkSync. Although I don't have much context regarding the Allo v2 contest, I believe the issue in the Allo contest should hold higher validity.

[0xRizwan]

@Trumpero

Thanks for the comment, Really appreciate it.

With due respect, I don't agree with the comment.

First,

Therefore, this issue can't cause any loss or harm to the protocol

Loss of funds is not the core requirement of Medium severity at sherlock. I understand there is some misunderstanding in interpreting the Medium severity identification, however it is clarified by @jacksanford1 here

Further if you see @Evert0x comments on Allo issue, it states below reasoning on Medium severity of issue,

Breaks core contract functionality, rendering the contract useless

This can be checked here

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.

This can be checked here

Second,

Secondly, the readme of the contest doesn't specify zkSync in the list of chains. The phrase 'could be any EVM compatible chains' implies potential, not certainty, of other chains. Hence, I believe being unable to deploy the current codebase on zkSync doesn't have an impact on the protocol.

I have given the justification above on this point. The protocol indeed support zksync era and deployed on it.

Third,

The last reason is that I discussed this issue with Kyber's protocol team, and they informed me that they were already aware of the trouble of their current codebase in zkSync, so they removed zkSync from the expected chains.

This is not true. zksync chain can be seen as supported and deployed network here. Further to note since whatever the sponsor has said, it is not mentioned in readme.md as known issue Therefore a readme is superior over sherlock rules in this case.

Issues in both the protocol are important, later is judged as Medium severity after clarification on Medium severity identification. To be noted allo issue is not judged based on loss of funds and your reasoning on this issue is low, it will not causes loss of funds.

I believe this might have happend due to misunderstanding or wrong interpretation of sherlock rules.

But the issue can still be resolved and this issue was correctly submitted as Medium severity and it must be judged as Medium severity.

[Trumpero]

@0xRizwan Your arguement is based on the docs of Kyberswap website which includes many protocols of Kyberswap (Kyberswap exchange, Kyber Classic pools, Kyber Elastic pools, Kyber limit order, etc), not only KyberElastic (Elastic pools) of contest in Sherlock. The docs you mentioned in Kyber's website lists the chains they support exchanges, not the chains where KyberElastic were deployed on. Currently, they only support user exchanges and classic pools on the zkSync Era, without including KyberElastic pools. You can verify this by visiting this site https://kyberswap.com/pools/zksync. The protocol team was already aware of being unable to deploy the current codebase of KyberElastic on zkSync so they didn't put zkSync into the chains list of their contest. They listed all the chains that should be valid in the contest, and the phrase 'could be any EVM compatible chains' indicates the potential for future integrations, though not with certainty. Therefore, I considered this not to be a core functionality of the protocol, and the issue should be regarded as an informational issue.

Additionally, I believe this issue is very different from the issue you mentioned in Allo v2 contest. So I believe it doesn't make sense to use the validity of that issue to consider this issue medium.