zkSync Era - 0xsomeone's results

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/main/code/contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol#L342 https://github.com/code-423n4/2023-10-zksync/blob/main/code/contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol#L344-L349

Vulnerability details

Impact

The deposit limitations meant to be enforced by the L1ERC20Bridge on the Ethereum mainnet are done so incorrectly. In detail, the deposit limitation enforcement function L1ERC20Bridge::_verifyDepositLimit will enforce the deposit limit solely when the depositLimitation flag is true.

Per the AllowList::setDepositLimit implementation, the depositLimitation of a token can be toggled between false and true states. This is a valid use case as for example, a token may become compromised and the zkSync Era team may wish to restrict migrations of it to small amounts.

The problem lies in the accounting system of the L1ERC20Bridge. Due to the "disconnect" between the deposit limitations enforcement and the transactions of the user, the two scenarios described in the PoC can occur.

Proof of Concept 1: Loss of Failed Deposit Funds (Impact: Medium)

The L1ERC20Bridge::claimFailedDeposit function will attempt to decrement the deposit limit of the user when reverting their failed deposit. To do this, it performs a subtraction on the totalDepositedAmountPerUser if the depositLimitation is true. In the following scenario, the code will prevent the user from claiming their failed deposit:

1. Alice submits their L1-to-L2 bridge transaction via L1ERC20Bridge::deposit while the deposit limitations for a token do not exist
2. Deposit limitations are enforced for the token Alice migrated
3. Alice's transaction fails on the L2
4. Alice attempts to claim their failed deposit on L1 via the L1ERC20Bridge::claimFailedDeposit

At this point, the subtraction performed by L1ERC20Bridge::_verifyDepositLimit will underflow as it was never incremented, causing the transaction to fail and the funds to be irrecoverable until an upgrade.

Proof of Concept 2: Untracked Deposit Limits (Impact: Minor)

The issue is applicable in the other direction as well; a user may detect an on-chain transaction that is about to enforce a deposit limitation for a token. In that case, the user may wish to circumvent this restriction by migrating a significant amount of funds before the deposit limitation is set.

The user will then be able to migrate more funds after the limitation is set as their significant migration was not tracked by the system.

Tools Used

Manual Review.

Recommended Mitigation Steps

We advise the totalDepositedAmountPerUser limitation to be tracked regardless of whether the depositLimitation for the token is set and the require check's actual limitation to be solely evaluated when the depositLimitation is true.

This will ensure that the system properly tracks deposit limits per user and does not cause unexpected failures when migrating from one state to the other.

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/main/code/contracts/ethereum/contracts/zksync/facets/Executor.sol#L303-L306 https://github.com/code-423n4/2023-10-zksync/blob/main/code/contracts/ethereum/contracts/zksync/facets/Executor.sol#L409-L411

Vulnerability details

Impact

The Executor::revertBatches function will improperly revert batches with an upgrade as it permits batches before the upgrade to also be reverted.

As such, all batch commitments and validations that have been reverted will be processed as if they were submitted after the upgrade even though they may have been submitted before it.

This will cause a fatal failure in the processing of batches as the Executor::_commitBatchesWithSystemContractsUpgrade function would be executed in an earlier batch than intended, causing the Executor::_processL2Logs function to fail.

At a minimum, this will cause the executor to halt its relayed batches (which are expected to be processed in sequence) until a smart contract upgrade addresses this issue. I consider this to be of major impact as it effectively leads to a disconnection between the state of the zkSync Era blockchain and the Ethereum blockchain.

Proof of Concept

The steps outlined below describe a high-level step-by-step flow due to the complexity of creating mock batches for submission to the Executor contract:

1. Submit multiple transaction batches to the contract via Executor::commitBatches
2. (Optional) Validate these transaction batches at the contract via Executor::proveBatches
3. Perform the upgrade of the system, the BaseZkSyncUpgrade::_setL2SystemContractUpgrade specifies that the hash is meant for the next batch to be committed
4. Create a batch meant to be committed after the upgrade and submit it to the Executor::commitBatches function
5. Revert the upgrade batch as well as the previous batches via the Executor::revertBatches function

At this point, the system will have reverted the upgrade batch as well as several batches in the past that have yet to be finalized via the Executor::executeBatches function. Attempting to re-submit the same batches would fail as the batches committed before the upgrade will not have the relevant EXPECTED_SYSTEM_CONTRACT_UPGRADE_TX_HASH_KEY log value.

Tools Used

Manual Review.

Recommended Mitigation Steps

There are multiple mitigation approaches that can be adopted for this particular issue. The system already prevents batches that have been executed from being reverted; this restriction can be expanded to upgrade batches. For example, any batches before an upgrade batch cannot be reverted and the system can be reverted up to and inclusive of the l2SystemContractsUpgradeBatchNumber.

Another potential mitigation involves tracking the number of batches that were submitted before the upgrade batch and have been reverted. The system could then permit up to that number of batches to be submitted before mandating that an upgrade batch is submitted. This mitigation is slightly more complex but would allow batches before an upgrade to be reverted.

Assessed type

DoS

zkSync Era Gas Optimization Report

This submission serves as a gas optimization report of the zkSync Era codebase. All optimizations relate to contracts meant to be deployed to the Ethereum mainnet per the contest guidelines. The ways the codebase can be optimized are as follows:

UnsafeBytes

UBS-01G: Significant Optimization of Overall Transaction Parsing

The UnsafeBytes library is utilized across L1ERC20Bridge, L1WethBridge, Executor, and the Mailbox contracts to parse significantly large payloads of data sequentially in an optimized way.

All utilization points of the library make use of a memory argument when a calldata argument could be passed in (L1ERC20Bridge::_parseL2WithdrawalMessage, L1WethBridge::_parseL2EthWithdrawalMessage, Executor::_processL2Logs, Mailbox::_parseL2WithdrawalMessage). If the library was updated to work on calldata arguments, its gas cost would be significantly reduced as it would utilize calldataload, would not require copying the full calldata payload to memory, and would not incur any hidden memory expansion gas costs.

L2ContractHelper

LCH-01G: Inefficient Evaluation of Modulo

The referenced modulo operation (%) is inefficient as its purpose is to identify whether the last bit of the bytecodeLenInWords is 1.

We advise an AND operation to be utilized instead, reducing the required gas cost from 5 to 3 per operation.

LCH-02G: Inefficient Evaluation of Multiplication

The referenced multiplication statement (*) is inefficient as its purpose is to "shift" the _bytecodeHash[2] value to the left, reconstructing the uint16 variable representing the bytecode length in words.

We advise a bitwise shift operation to be utilized, reducing the gas cost required by the operation from 5 to 3 for each invocation.

AllowList

ALS-01G: Inefficient Mapping Lookups

The referenced statements will re-calculate the keccak256 hash of the hasSpecialAccessToCall mapping slots twice without being optimized.

While the compiler does optimize keccak256 calculations of the same value consecutively, the hasSpecialAccessToCall is accessed as a nested mapping causing three unique keccak256 calculations to be calculated per access thus resulting in 6 keccak256 operations that are redundant.

We advise the hasSpecialAccessToCall[_caller][_target] lookup to be stored in a local mapping(bytes4 => bool) storage variable, permitting its pre-calculated offset to be re-used when assigning the _enable flag within the ensuing if block.

Diamond

DDN-01G: Inefficient Memory Expansion

The referenced variables are declared locally within the for loop of Diamond::diamondCut whilst they are already present in memory as the function's argument.

We advise the code to instead utilize a single local variable within the for loop, a FacetCut memory variable whose members are accessed wherever the previous declarations were.

This optimization will result in a median decrease of 174 gas units per iteration with optimizations turned on for a FacetCut payload with 3 selectors. The optimizations will scale with the number of selectors present in a cut.

DDN-02G: Inefficient Mapping Lookups

The referenced statements will re-calculate the keccak256 hash of the facetToSelectors and selectorToFacet mapping slots multiple times without being optimized.

While the compiler does optimize keccak256 claculations of the same value consecutively, the referenced statements are not performed in sequence and would benefit from an optimization that caches the mapping lookup in a local variable.

The total lookups that will be optimized by a local variable are 3.

LibMap

LMP-01G: Sub-Optimal Fork of Solady

The LibMap in use by the codebase is currently sub-optimal as an implementation is present in the official Solady repository that contains a greater degree of optimization. In detail, lookups are optimized by using bitwise operators instead of divisions and multiplications (result in a 2 gas cost reduction per operation) whilst sets are entirely performed in assembly blocks further benefitting from optimized gas costs.

We advise the latest implementation by Solady to be re-imported in the codebase, optimizing its gas cost across the board.

Merkle

MEL-01G: Inefficient Evaluation of Modulo

The referenced modulo operation (%) is inefficient as its purpose is to identify whether the last bit of the _index is 1.

We advise an AND operation to be utilized instead, reducing the required gas cost from 5 to 3 per operation.

MEL-02G: Inefficient Evaluation of Division

The referenced division operation (/) is inefficient as its purpose is to divide the _index by 2 on each iteration, effectively shifting its bits.

We advise a bitwise shift operation to be utilized instead, optimizing the gas cost of each iteration by 2 gas units.

Executor

ESR-01G: Inefficient Evaluation of Ternary Operator

The Executor::_commitBatchesWithSystemContractsUpgrade function will execute special logic for the first batch of the total batches being committed inefficiently within the for loop.

We advise the system upgrade batch to be executed outside the for loop and the for loop body to be identical to the Executor::_commitBatchesWIthoutSystemContractsUpgrade albeit starting the iterator at 1 instead of 0.

Mailbox

MXO-01G: Inefficient Mapping Lookups

The referenced statements will re-calculate the keccak256 hash of the isEthWithdrawalFinalized mapping slots two times redundantly in the same codeblock.

We advise the s.isEthWithdrawalFinalized[_l2BatchNumber] lookup to be cached to a local mapping(uint256 => bool) storage variable that is consequently utilized for the true assignment statement, optimizing the gas cost of the function by one less redundant keccak256 operation.

L1ERC20Bridge

LER-01G: Inefficient Mapping Lookups

The referenced statements will re-calculate the keccak256 hash of the isWithdrawalFinalized and totalDepositedAmountPerUser mapping slots two times redundantly in each codeblock.

We advise the isWithdrawalFinalized[_l2BatchNumber] and totalDepositedAmountPerUser[_l1Token] lookups to be cached to a local mapping(uint256 => bool) storage and mapping(address => uint256) storage variables respectively, optimizing the gas cost of each function by one less redundant keccak256 operation.

L1WethBridge

LWB-01G: Inefficient Mapping Lookups

The referenced statements will re-calculate the keccak256 hash of the isWithdrawalFinalized mapping slots two times redundantly in the same codeblock.

We advise the isWithdrawalFinalized[_l2BatchNumber] lookup to be cached to a local mapping(uint256 => bool) storage variable that is consequently utilized for the true assignment statement, optimizing the gas cost of the function by one less redundant keccak256 operation.

Governance

GSE-01G: Inefficient Iterator Increment Statement

The referenced iterator increment statement, unlike the rest of the codebase does not make use of the UncheckedMath contract.

We advise it to be utilized and the increment statement to be performed unsafely, optimizing the code's gas cost.

BaseZkSyncUpgrade

BZS-01G: Inefficient Iterator Increment Statement

The referenced iterator increment statement, unlike the rest of the codebase does not make use of the UncheckedMath contract.

We advise it to be utilized and the increment statement to be performed unsafely, optimizing the code's gas cost.