Submitted by anon
There is an inconsistency between zkSync Era and Ethereum Virtual Machine (EVM) in handling deployment nonce leading to significant consequences. In EVM, the factorys nonce is increased at the beginning of child contract creation, and if the child contracts creation fails, the nonce is not rolled back. In zkSync Era, the deployment nonce increases during the deployment process, and if the child contract constructor reverts, the entire transaction, including the increased deployment nonce, also reverts.
In zkSync Era, when a factory contract deploys another contract using low-level creation operations (i.e. using CREATE or CREATE2 in assembly, instead of using new), it is expected that the deployment nonce of the factory will increment by one, akin to the behavior in the Ethereum Virtual Machine (EVM). However, a notable issue arises when the constructor of the child contract encounters a revert.
In the EVM, the nonce of the factory increases as soon as the CREATE operation begins, regardless of whether the child contract creation is ultimately successful. This can be seen in GETH codebase, where the nonce of the factory is increased at line 431. Then, the EVM captures a snapshot, at line 443, to manage any potential failures, allowing for a graceful revert, at line 492, in case of unsuccessful child contract creation. In such cases, the returned address is address(0), indicating a failed contract creation attempt.
https://github.com/ethereum/go-ethereum/blob/dc34fe8291bfcaefbce97f559e9610beffb2e470/core/vm/evm.go#L431
https://github.com/ethereum/go-ethereum/blob/dc34fe8291bfcaefbce97f559e9610beffb2e470/core/vm/evm.go#L443
https://github.com/ethereum/go-ethereum/blob/dc34fe8291bfcaefbce97f559e9610beffb2e470/core/vm/evm.go#L492
However, zkSync Era differs in its approach. When the CREATE opcode is employed, it invokes the create or create2 function within the ContractDeployer contract.
https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/ContractDeployer.sol#L146
https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/ContractDeployer.sol#L130
This action results in the incremented deployment nonce of the factory, and, in the final stages of contract creation, it calls the constructor of the child contract.
https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/ContractDeployer.sol#L189
https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/ContractDeployer.sol#L344
In the event of a revert in the child contracts constructor, the entire transaction in the ContractDeployer is reverted. Consequently, the incremented deployment nonce of the factory is also rolled back, and the returned address signifies an unsuccessful contract creation attempt by being set to address(0).
This divergence in nonce management between zkSync Era and the EVM could have far-reaching implications, especially for factories that are deployed on zkSync Era and assume that the deployment nonce behaves in a manner consistent with the EVM.
Calling the function test triggers the internal function deploy three times. In each call, the child contract is deployed, and two values are returned: the predicted address of the to-be-deployed child and the address of the deployed child. The first and third deploy calls provide false as an argument to ensure that the constructor of the child doesnt revert. In the second deploy call, true is used as an argument, causing the constructor of the child to revert.
Comparing the predicted addresses with the deployed addresses reveals the following:
Its worth noting that the initial value of the variable nonce is set to one for the EVM PoC, while it is set to zero for the zkSync Era PoC. This difference is unrelated to this report and has been explained in a separate report.
In EVM:
The result for EVM is:
In zkSync Era:
The result for zkSync Era is:
As you see, the deployedAddress3 is equal to predictedAddress2 which is not correct.
A potential solution might involve invoking the _constructContract function externally within a try/catch block. By doing so, if _constructContract reverts (as occurred with the child contract), the deployment nonce wont be rolled back. However, implementing this kind of modification would necessitate reviewing and changing other parts of the code as well.
Context
miladpiri (zkSync) acknowledged, but disagreed with severity and commented:
Alex the Entreprenerd (judge) decreased severity to Medium and commented:
anon (warden) commented:
unforgiven (warden) commented:
anon (warden) commented:
miladpiri (zkSync) commented:
Alex the Entreprenerd (judge) commented:
