QA Report

[c4-submissions]

See the markdown file with the details of this report here.

[bytes032]

9 l 4 r 20 nc

[QA-1] Unsafe addition in unchecked block may overflow in NonceHolder.sol
l

[QA-2] Missing checks for address(0) when updating address state variables
bot

[QA-3] Consider adding address verifyingContract to EIP721_DOMAIN_TYPEHASH in TransactionHelper.sol
r

[QA-4] Violation of EIP-712 in TransactionHelper.sol
l

[QA-5] Functions in Governance.sol do not check if new values are the same as current state
bot

[QA-6] Incorrect type of blockGasLimit in SystemContext.sol
low

[QA-7] L1ERC20Bridge won't work with rebasing tokens
r

[QA-8] getZkSyncMeta() from SystemContractHelper.sol does not include some fields from ZkSyncMeta
r

[QA-9] Lack of cleaning of variables before call in inline assembler
nc

[QA-10] Lack of 0-value check in NonceHolder.sol
nc

[QA-11] processPaymasterInput() may revert for some ERC-20 tokens
x

[QA-12] Lack of address(0) check in L2ERC20Bridge means that finalizeDeposit() will mint tokens to address(0)
l

[QA-13] storedBatchHash() might return batch which was reverted by Executor's revertBatches()
r

[QA-14] DiamondProxy does not check if contract exists
l

[QA-15] Lack of input validation in AccountCodeStorage.sol may lead to overflow
l

[QA-16] Incorrect assumption may lead to revert in _burnMsgValue() in system-contracts/contracts/L2EthToken.sol
l

[QA-17] The current gas price might be set to 0 in SystemContext
l

[QA-18] MsgValueSimulator ignores MAX_MSG_VALUE
l

[N-1] In ContractDeployer.sol - ensure that new value is different than the current one
bot

[N-2] Update name of updateNonceOrdering to better reflect its usage
nc

[N-3] In NonceHolder.sol - function getRawNonce() can be used inside more functions
nc

[N-4] Unused code / code for local testing should be removed
nv

[N-5] ContractDeployer.sol events do not emit parameters from old values
bot

[N-6] NatSpec does not properly describes _setAllowList() in ethereum/contracts/upgrades/BaseZkSyncUpgrade.sol
nc

[N-7] Redundant check in DefaultAccount.sol
nc

[N-8] incrementDeploymentNonce() from NonceHolder.sol doubles access checks:
nc

[N-9] In NonceHolder.sol - the order of getters and setters are mixed
nc

[N-10] Incorrect comment in SystemContext about baseFee
nc

[N-11] The behavior of _markBytecodeAsPublished() may be misleading
nc

[N-12] Incorrect comment in TransactionHelper.sol
nc

[N-13] Improper NatSpec for _isValidSignature() in DefaultAccount.sol
nc

[N-14] Some functions define named returns, but still return value via return statement
bot

[N-15] Inconsistency in representing hex 0x00
nc

[N-16] Misleading variable naming in SystemContext.sol
nc

[N-17] Constants in NonceHolder.sol does not have visibility set
nc

[N-18] Define constants, instead of using magic numbers:
bot

[N-19] Use ternary operators to make if/else statements more readable
nc

[N-20] ForceDeployment struct can be moved from ConstractDeployer.sol
nc

[N-21] Repeated checks can be implemented as modifiers
bot

[N-22] ReentrancyGuard.sol contains links to non-existing webpages
nc

[N-23] Comments are not grammatically correct:
nc

[N-24] Messages in require are not descriptive
nc

[lsaudit]

Hey, firstly - thank you for the access to the repo and for judging it so quickly!
I've reviewed your remarks and I'd like to ask you to take another look at some of my issues.

[QA-13] storedBatchHash() might return batch which was reverted by Executor's revertBatches()

The exactly same issue was evaluated as Low in #1124 (QA-02), while my issue as Refactor. I also think, that this one should be evaluated as Low, instead of Refactor, since it's the same issue.

[QA-2] Missing checks for address(0) when updating address state variables

As mentioned in my report, this issue was not included in the bot-report, thus I've reported it here.

In the bot report, I see only one "missing checks for address(0x0)" issues: L-21 (which mentions ValidatorTimelock.sol, Admin.sol and SystemContext.sol - it does not mention Governance.sol which I've reported).

The other issue from bot report is L-20 - which reports missing checks for address(0x0) in the constructor/initializer. It does indeed report Governance.sol, however, the bot-report mentions, lack of address(0) check in the constructor. In my issue - QA-2 - I'm reporting that there's no address(0) check in updateSecurityCouncil() - this is separate function which was not included in the bot report.
Even when the developer would fix the bot's L-20 (and checks for address(0) in the constructor) - it still will be possible to set securityCouncil_ to address(0) by calling updateSecurityCouncil(). IMHO, this makes this issue not a duplicate, but a valid Low finding.

[QA-3] Consider adding address verifyingContract to EIP721_DOMAIN_TYPEHASH in TransactionHelper.sol

EIP-721 explicitly states, that verifyingContract is used for phishing prevention. It has a real value for the security of the contract itself. I think this issue should be considered as Low, instead of Refactor, since missing verifyingContract has an impact on overall security of EIP721_DOMAIN_TYPEHASH.

[QA-9] Lack of cleaning of variables before call in inline assembler

Solidity docs straightforwardly mentions that "To be safe, always clear the data properly before you use it in a context where this is important". Since in all instances, the issue affects the address variable, and then we call, staticcall and delegatecall to that address - a potential impact might be much higher than NC. I've reported it as Low.

[N-1] In ContractDeployer.sol - ensure that new value is different than the current one

I don't see this entry in the bot-report. I think it was confused with bot's N‑43 - "Events that mark critical parameter changes should contain both the old and the new value".
The bot reports, that when we're updating nonce ordering, the AccountNonceOrderingUpdated() event should emit both previous and new value of nonce ordering.
My report states, that we should verify if updateNonceOrdering() really updates the ordering. In other words, if the current version of nonce ordering is AccountNonceOrdering.Sequential and we're calling updateNonceOrdering(AccountNonceOrdering.Sequential), we're basically don't update anything. However, function will still emit an event which might be confusing (as it suggests, that the update had taken place).
The recommendation for this issue is to add additional check: require(_nonceOrdering != currentInfo.nonceOrdering, "You are not updating anything"). As mentioned above, this is not bot's duplicate and probably should be evaluated as Refactoring issue.

[QA-5] Functions in Governance.sol do not check if new values are the same as current state

Same as above, bot reports that event should contain both the old and the new value. May issue states that there should be an additional check, to make sure we're not updating the same value.
E.g. if minDelay = 123 and we call updateDelay(123), function will still emit an event, even thought, nothing will be changed (minDelay will still be 123). There should be additional check: require(minDelay != _newDelay, "You are not updating anything!").
I've reported this issue in two functions updateDelay() and updateSecurityCouncil().

[N-21] Repeated checks can be implemented as modifiers

I've grepped the whole bot-report section and I do not see anything which suggests it's already set in a bot report. Could you please point me out to the bot's issue?

Additional question

Moreover, would this degraded report (from Medium to QA) be also included in my QA report: #614

[lsaudit]

Hey @GalloDaSballo

I've one more additional issue related to my QA report.

[QA-6] Incorrect type of blockGasLimit in SystemContext.sol

I've noticed, that multiple of issues were evaluated as Medium, whenever inconsistent behavior of the EVM with the zkSyncVM was demonstrated:

E.g., some of your comments:

#92 (comment)

We have an historical record of awarding non-evm equivalence that can cause damage as med, so I'm inclined to maintain the severity

#426 (comment) #175 (comment) #174 (comment)

(...)
Because the goal of the zkSync EVM is to be the EVM compatible, Medium Severity seems appropriate

#133 (comment)

(...)
I believe Medium Severity is most appropriate as the finding demonstrates an inconsistent behaviour of the EVM with the zkSyncVM

I agree with the impact, but believe that such finding belongs to the Medium Severity classification

#25 (comment)

Because the behaviour differs from the EVM, I believe that Medium Severity is appropriate

Since I was not aware that proving inconsistency between zkSync and EVM would be evaluated as Medium - I wrote [QA-6] Incorrect type of blockGasLimit in SystemContext.sol in QA Report, instead of reporting it as a separate Medium issue.

Could you plese verify, if above issue (QA-6) should also be treated as inconsistency between EVM and zkSync and be classified as Medium?

I've redesigned the structure of my finding (added Impact and Recommended Mitigation Steps, PoC remained the same and was copy-pasted from my QA Report).

---

Impact

The gas limit of a block defines the maximum amount of gas that all the transactions inside the block are allowed to consume.
The EVM defines it as uint64. However, in zkSync, the gas limit is defined as uint256. This discrepancy implies, that the zkSync gas limit can be changed to a higher value (since it's uint256) than EVM (in EVM the max value needs to fit in uint64).

The goal of the zkSync EVM is to be the EVM compatible. This goal is not fulfilled. zkSync implementation allows for setting higher gas limit (because gas limit is defined as uint256) than EVM.

Proof of Concept

EVM defines gasLimit as uint64:

https://github.com/ethereum/go-ethereum/blob/c1d5a012ea4b824e902db14e47bf147d727c2657/core/types/tx_legacy.go#L30

Gas      uint64          // gas limit

However, the current implementation of zkEVM implements blockGasLimit as uint256:

File: system-contracts/contracts/SystemContext.sol#L37

    uint256 public blockGasLimit = type(uint32).max;

There shoudn't be any inconsistency between variable datatypes. The variable type should be the same as the EVM, so uint64.

Recommended Mitigation Steps

uint256 public blockGasLimit = type(uint32).max; should be changed to uint64 public blockGasLimit = type(uint32).max;

[GalloDaSballo]

I disagree with the escalation for 2 reasons:
Both values will never overfllow
The size of the value is still one word

[lsaudit]

escalation: thank you, that explanation seems fair!

rest of my QA: I've missed the google docs link when I was escalating, but now I see that my report is already graded as A. Upgrading QA-6 was disputed, thus my QA score won't be affected. Therefore, please feel free to ignore the rest of my comments. If this QA is already graded as A, further escalation won't change a thing, so I don't wanna unnecessary waste judge's time!

[c4-judge]

GalloDaSballo marked the issue as grade-a