zkSync Era - Audittens's results

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/bootloader/bootloader.yul#L1667-L1669 https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/bootloader/bootloader.yul#L1681 https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/bootloader/bootloader.yul#L991-L996

Vulnerability details

Description

The issue arises from the following sequence in the bootloader's behavior:

1. The ZKSYNC_NEAR_CALL_executeL1Tx function is responsible for executing an L1->L2 transaction.
2. Within this function, if the L1 transaction fails (success = 0), the nearCallPanic() function is invoked.
3. The nearCallPanic() function, when called, exhausts all the remaining gas for the current frame.
4. Consequently, in getExecuteL1TxAndGetRefund function, potentialRefund is equal to zero.
5. As a result, user doesn't receive the expected gas refund, diverging from Ethereum's behavior where unused gas is returned to the caller upon a transaction failure.

Impact

• Users end up paying more than expected for failed transactions as they do not receive the expected gas refunds.
• Ethereum incompatibility: the described behavior is inconsistent with the Ethereum standards, leading to potential compatibility issues for dApps and systems expecting Ethereum-like behavior.

Recommended Mitigation Steps

Modify the getExecuteL1TxAndGetRefund function. First, calculate and store the value of gasSpentOnExecution in memory. Only after storing this value should the nearCallPanic() function be invoked. By doing this, even if nearCallPanic() exhausts all the remaining gas, the gasSpentOnExecution stored in memory can be used to correctly compute the potentialRefund, ensuring compatibility with Ethereum's behavior.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/Constants.sol#L35 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/AccountCodeStorage.sol#L93-L95 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/AccountCodeStorage.sol#L131-L137

Vulnerability details

Description

CURRENT_MAX_PRECOMPILE_ADDRESS is set to uint256(uint160(SHA256_SYSTEM_CONTRACT)) which is address(0x02), while the real maximal used precompile address is ECMUL_SYSTEM_CONTRACT = address(0x07).

The AccountCodeStorage::getCodeHash and AccountCodeStorage::getCodeSize functions rely on this constant to emulate the Ethereum behavior of the corresponding opcodes.

Impact

The wrong value is returned in the AccountCodeStorage::getCodeHash and AccountCodeStorage::getCodeSize functions for the precompiles that were added with addresses greater than address(0x02).

Recommended Mitigation Steps

Set the CURRENT_MAX_PRECOMPILE_ADDRESS constant to be equal to ECMUL_SYSTEM_CONTRACT or rename the mentioned constant to be MAXIMAL_POSSIBLE_PRECOMPILE_ADDRESS (and set it to the upper bound for the possible precompiles addresses).

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/bootloader/bootloader.yul#L1244-L1245 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/bootloader/bootloader.yul#L1255-L1259 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/bootloader/bootloader.yul#L1365 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/bootloader/bootloader.yul#L1542 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/scripts/process.ts#L123 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/Compressor.sol#L54-L82

Vulnerability details

Description

There is enforcement in the bootloader::sendCompressedBytecode function that the data to be used for the call of the Compressor::publishCompressedBytecode function is encoded in a standard way.

But this check contains bugs:

1. There is no check that the originalBytecodeOffset is equal to afterSelectorPtr + 64. This leads to the possibility of passing a great number of unused bytes as calldata of the call of the Compressor::publishCompressedBytecode function. In this case, the calldata will have the following format:

4                               bytes : `publishCompressedBytecode` selector
32                              bytes : offset for `_bytecode` parameter                                                  = V
32                              bytes : offset for `_rawCompressedData` parameter                                         = V + 32 + rounded_len(_bytecode)
(V - 64)                        bytes : any bytes that will be ignored in the `publishCompressedBytecode` function
32                              bytes : length of `_bytecode` parameter                                                   = len(_bytecode)
rounded_len(_bytecode)          bytes : `_bytecode` parameter                                                             = _bytecode 
32                              bytes : length of `_rawCompressedData` parameter                                          = len(_rawCompressedData)
rounded_len(_rawCompressedData) bytes : `_rawCompressedData` parameter                                                    = _rawCompressedData

2. There are used unsafe "raw" ariptmetic operations in the calculations of the offsets, while the safe analogs should be used. This leads to the possibility of passing absolutely wrong data so the call of the Compressor::publishCompressedBytecode function will revert. As this bug does not provide any strong attack vector I will present only the idea of how to achieve this, not the whole calldata: the offset of the _bytecode parameter should be equal (2**256) - 64 (so the originalBytecodeOffset will pass the checkOffset check), rest of the parameters is easy to determine over this. The same happens in the bootloader::validateBytes function, as in the bootloader::lengthRoundedByWords function the overflow happens, leading to the same outcome.

Impact

The ability of the operator to pass up to ~COMPRESSED_BYTECODES_SLOTS * 32 = ~1048576 additional bytes into the call of the Compressor::publishCompressedBytecode function. Such possibility of spending part of the transaction gas limit makes the operator able to actually decrease the L2 transaction gas limit, which leads to the possibility of manipulation of the amount of gas used for the transaction execution.

The ability of the operator to pass encoding-related checks of the bootloader::sendCompressedBytecode function, which leads to the possibility of forcing the Compressor::publishCompressedBytecode function call to revert.

Recommended Mitigation Steps

1. You can add the following check into the bootloader::sendCompressedBytecode function:

if iszero(eq(add(afterSelectorPtr, 64), originalBytecodeOffset)) {
    assertionError("Compression calldata incorrect")
}

2. You can use safeAdd instead of add in the corresponding calculations in the bootloader::sendCompressedBytecode function.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/bootloader/bootloader.yul#L1244-L1245 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/bootloader/bootloader.yul#L1255-L1259 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/bootloader/bootloader.yul#L1365 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/bootloader/bootloader.yul#L1562-L1579 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/Compressor.sol#L54-L82

Vulnerability details

Description

In the bootloader::sendCompressedBytecode function the nearCallPanic() is used for the case of unsuccessfull call of the Compressor::publishCompressedBytecode function. In practice, such situation could happen because of invalid data provided by the operator, as any of the internal checks of this function could be ignored by the operator to make the call to revert: decoding of the _rawCompressedData (link), the checks on dictionary length (link1, link2), the check on the encodedData length (link), etc.

Such possibility of forcing the overall bootloader::ZKSYNC_NEAR_CALL_markFactoryDepsL2 to near call revert, makes the operator able to burn all the gas of the L2 transaction if there is at least one bytecode to be published at the start of transaction processing.

Impact

The ability of the operator to burn all the gas of the L2 transaction in the call of the bootloader::ZKSYNC_NEAR_CALL_markFactoryDepsL2 function, if at least one bytecode is to be published.

Recommended Mitigation Steps

Use revertWithReason instead of nearCallPanic for the cases of unsuccessful calls of the Compressor::publishCompressedBytecode function. The cases of out of gas error on such calls operator can process as standard bytecode publications through the bootloader::markFactoryDepsForTx functionality.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/zksync/facets/Mailbox.sol#L318 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/zksync/facets/Mailbox.sol#L305-L306 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/zksync/facets/Mailbox.sol#L323 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/bootloader/bootloader.yul#L1656-L1659 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/bootloader/bootloader.yul#L2321 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/bootloader/bootloader.yul#L2328-L2333 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/DefaultAccount.sol#L122-L129

Vulnerability details

Description

ETH on L2 stored on EOAs is inaccessible through L1->L2 transactions, as for such transactions msg.value is generated only "from L1 ETH", not from the active balance of the tx.origin on L2. This is so because of the logic in the Malibox::_requestL2Transaction function, which enforces msg.value of the L1->L2 transaction to be only part of ETH to be minted inside of transaction processing on L2. Bootloader maintains this system invariant as well.

DefaultAccount incorrectly assumes that L1->L2 transactions initiated by EOA cover the needed functionality of the executeTransactionFromOutside function, so it is empty.

Users will be unable to access their ETH on L2 in the case of a malicious operator that ignores L2 transactions (so if it processes only L1->L2 transactions). Also, and importantly, this violates the security invariant that enforces the ability of the users to access L2 through L1->L2 transactions, and, therefore, withdraw funds from the rollup before the upgrade in case of a scheduled malicious upgrade.

Impact

No access to ETH on L2 that is controlled by EOAs in case of malicious operator.

Violation of the "guarantee of withdraw of funds before malicious upgrade execution" security invariant.

Recommended Mitigation Steps

Add a possibility to create L1->L2 transactions that can use ETH from the L2 balance as part of the msg.value. As an alternative, you can implement the DefaultAccount::executeTransactionFromOutside function so it will contain the expected behavior, as stated in the comment above its declaration.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/governance/Governance.sol#L70-L76 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/governance/Governance.sol#L154-L158 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/governance/Governance.sol#L186

Vulnerability details

Description

According to the implementation of the Governance::cancel function, the security council can cancel any operation that was scheduled by the governance owner.

While this feature creates the right for the security council to cancel malicious operations scheduled by the owner, it does not create additional protection for users of the rollup -- protection against a scheduled malicious operation (in practice, rather, a malicious upgrade) comes from the minimum delay for such operations, and only such protection can be considered as guaranteed by the smart contract and sufficient (in practice it is sufficient due to invariant that a user will be able to enforce the execution of an L1->L2 operation using L1 priority queue while the upgrade has not yet been completed).

At the same time, this feature creates unnecessary security assumptions about the security council.

Attack scenario

As a demonstrative example of the problem (with a realistic application of the governance functionality -- rollup upgrades management), one can consider the following scenario:

1. There is a bug in the rollup system where it is possible to corrupt the rollup state invariants/system contracts invariants/some DeFi project on zkSync with a very great TVL/...
2. The bug allows the attacker to break the system only over a relatively long period (for example, 3 times the minimum delay for the governance operation)
3. After some entity initiated the attack no other entity can "take the attacker's place" (for example, the attacker's address has been stored at the storage, and, due to some specifics of the vulnerability, no other entity can override it -- the only way to stop the attacker is to conduct the upgrade)
4. The attacker can be a smart contract entity, with the following code implemented: I) Smart contract automatically performs all necessary actions to break the system -- anyone can call necessary functions to process the attack and finalize it at the end of the scenario II) When the attack is over, the smart contract automatically splits all the profit from the attack between hardcoded security council accounts and attacker account using the hardcoded ratio

In such a situation, the owner will schedule an upgrade to fix the vulnerability and protect the system. But there is a very powerful economic incentive to cancel such a scheduled upgrade (and in general all owner's attempts to upgrade the rollup).

Also, the system does not have any (at least publicly mentioned) incentive/enforcement that the security council will execute an instant upgrade in case of the analogical scenario but with a smaller required time delay for the attacker.

Impact

A very powerful economic incentive for the security council to cancel upgrades proposed by the owner, with almost zero practical benefit to the system from such a possibility.

Lack of system incentive/enforcement that the security council will execute instant upgrades in case of need.

Recommended Mitigation Steps

Allow only the governance owner to cancel scheduled governance operations.

Create additional incentives for the security council to execute instant upgrades if necessary.

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/contracts/DefaultAccount.sol#L222-L227 https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/contracts/DefaultAccount.sol#L45

Vulnerability details

Description

The DefaultAccount contract's fallback function is designed to prevent any calls from the bootloader using an assertion check against the bootloader's formal address. However, the fallback function doesn't consider the scenario where another account, when invoked by the bootloader, uses a delegatecall to call the DefaultAccount contract's fallback. In such a scenario, the assertion check will fail, causing the contract to panic instead of behaving neutrally.

Attack scenario

Someone can create a contract that, when invoked by the bootloader, utilizes a delegatecall to the DefaultAccount's fallback function. This will trigger the assert statement, causing the DefaultAccount to panic, which is an unintended behavior.

Impact

The vulnerability may lead to unexpected behavior in the contract operation, potentially causing disruptions or, in worst-case scenarios, loss of funds or other unintended consequences.

Recommended Mitigation Steps

Add ignoreInDelegateCall modifier to DefaultAccount's fallback function.

Assessed type

call/delegatecall

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/bootloader/bootloader.yul#L1244-L1245 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/bootloader/bootloader.yul#L1255-L1259 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/bootloader/bootloader.yul#L1365 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/scripts/process.ts#L123 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/Compressor.sol#L54-L82 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/Compressor.sol#L62

Vulnerability details

Description

There is no enforcement in the system that the compressed bytecode to be published was compressed optimally. Moreover, there is no enforcement that the compressed data to be published is not longer than the original bytecode. The only upper bounds for the unused data of "_rawCompressedData" (specifically, dictionary) are 524288 bytes from the Compressor::publishCompressedBytecode function and ~32768 slots == ~1048576 bytes from the bootloader memory restriction.

This gives the operator the ability to spend a great amount of gas on the emulation of the publication of a fake "compressed bytecode" (with length up to ~524288 bytes) in any case, even when the real bytecode is relatively short. This can cost for the transaction initiator the same as publication of ~524288 non-zero bytes in L1 calldata (which is ~524288 * 16 = ~8'388'608 gas on L1). Please note, that the operator actually will not publish this data on L1 in most of such cases, as in practice users will not initiate transactions with so a great L2 gas limit. And even for the cases when the gas limit of the transaction enforces publication of such data on L1, this attack is profitable for the operator.

Impact

Ability of the operator to perform publication of up to ~524288 bytes of the "compressed bytecode", for each of the bytecodes that are to be published at the transaction preparation.

In most of the practical cases, the gas for such operation will be burned (the cost of it will be paid by the user and the "body" of the L2 transaction will be not executed), while the real publication of the bytecode will be skipped due to revertion of the bootloader::ZKSYNC_NEAR_CALL_markFactoryDepsL2 function.

Also, and importantly, such possibility of spending part of the transaction gas limit makes the operator able to actually decrease the L2 transaction gas limit, which leads to the possibility of manipulation of the amount of gas used for the transaction execution.

Recommended Mitigation Steps

Enforce that the length of the compressed data in the bytecode publication is not greater than the original bytecode length. Namely, you can add the following check to the Compressor::publishCompressedBytecode function:

require(_rawCompressedData.length < _bytecode.length, "Compressed data should be shorter than the original bytecode");

To make this check more accurate, there should be a counting of non-zero bytes in both arrays and the corresponding weighted check.

Assessed type

Other