Linea Gas Optimizations Audit

OpenZeppelin Security | May 17, 2024

Summary

Type: ZK Rollup
Timeline: From 2024-04-29; To 2024-05-08
Languages: Solidity

Total Issues: 12 (12 resolved)
Critical Severity Issues: 0 (0 resolved)
High Severity Issues: 1 (1 resolved)
Medium Severity Issues: 0 (0 resolved)
Low Severity Issues: 3 (3 resolved)
Notes & Additional Information: 5 (5 resolved)
Client Reported Issues: 3 (3 resolved)

Scope

We audited the Consensys/zkevm-monorepo repository at commit 7f3614e. The in-scope contracts were diffed against commit 9a847b8. We also audited the changes made in PR #36 of the Consensys/linea-contracts-fix repository at commit a6a26dd. All the resolutions mentioned in this report are contained at commit bbf9091, making it the final version reviewed during this audit. This corresponds to commit b17e7c7 in the Consensys/linea-contracts repository.

In scope were the following files:

 contracts
└── contracts
    ├── LineaRollup.sol
    ├── ZkEvmV2.sol
    ├── interfaces/l1/ILineaRollup.sol
    ├── messageService
    │   ├── l1
    │   │   ├── L1MessageService.sol
    │   │   ├── TransientStorageReentrancyGuardUpgradeable.sol
    │   │   └── v1/L1MessageServiceV1.sol
    │   ├── l2/v1/L2MessageService.sol
    │   └── lib/TransientStorageHelpers.sol
    └── tokenBridge
        ├── TokenBridge.sol
        └── interfaces/ITokenBridge.sol

System Overview

This diff audit is centered around changes made to the protocol related to gas optimizations. The first major change is the use of transient storage in two different contexts. The TransientStorageReentrancyGuardUpgradeable abstract contract and the associated TransientStorageHelpers library have been introduced. This replaces OpenZeppelin's ReentrancyGuardUpgradeable contract and takes advantage of the new TSTORE and TLOAD instructions introduced in the Dencun fork in order to save gas. In addition, the L1 message service contract (which is inherited from by the L1 rollup contract) now saves the message sender in transient storage while the message is being executed.

Another major change is the deprecation of many storage variables in the L1 rollup contract. These storage variables were previously used in order to ensure consistency when submitting and finalizing new data from L2. Instead, summary variables are saved so previously submitted data can be recognized when they are passed as calldata in future submissions. This pattern requires fewer SSTORE and SLOAD operations, again saving gas. This update also comes with the added functionality that multiple blobs can be submitted together.

Trust Assumptions

This upgrade does not introduce new roles to the system. Since there is already an existing system with values stored in the proxy contract, the changes are introduced by upgrading to a new implementation contract at a predetermined time. As such, there is an initialization function that will be called to initialize the new variables. Because there is no access control on this function, it is assumed that the upgrader will atomically deploy and call this initialization function, and pass in the correct values to maintain consistency within the protocol.

High Severity

Inconsistent State Root Hash

During finalization, the operator provides the previous and new shnarfs, as well as the previous and new state root hashes. However, the shnarfs and state roots are not validated to match each other. In particular, the parent state root hash must match the correct record in the stateRootHashes mapping, which is then updated to include the final state root hash. It should be noted that this chain of state root hashes does not have to correspond to the actual L2 state root hashes that are validated through the shnarfs. Moreover, incorrect state root hashes would also cause invalid BlocksVerificationDone events.

Consider validating that the final state root hash is consistent with the finalized shnarf.

Update: Resolved in pull request #3183.

Low Severity

Incomplete Deprecation

The _messageSender contract variable has been deprecated but it is still being used in the old claimMessage function, and after this call, it is set to the DEFAULT_SENDER_ADDRESS.

Consider completing the deprecation as well as removing DEFAULT_SENDER_ADDRESS in favor of DEFAULT_MESSAGE_SENDER_TRANSIENT_VALUE.

Update: Resolved in pull request #3174.

Magic Numbers

The _computePublicInput function extracts several fields from a FinalizationDataV2 struct using hard-coded numeric offsets.

For code clarity, consider defining named offset constants next to the struct so they can be validated directly.

Update: Resolved. This is not an issue. The Linea team stated:

As the interface does not support constants, additional comments with expected struct parameter offsets were added to the public input computation function.

Mutable Submission Record

In the submitDataAsCalldata function, duplicate submissions are prevented. However, a caller can reuse previous data corresponding to a known shnarf, but provide a different final block number in order to overwrite a previous submission.

Instead of strictly preventing duplicates, consider updating the check to prevent overwriting any non-zero record, such as in submitBlobs, to ensure the immutability of submitted data.

Update: Resolved in pull request #3175.

Notes & Additional Information

Non-Standard Storage Locations

The codebase includes two pseudorandom storage locations (1, 2) specified as direct keccak256 outputs. Although these are used for transient storage and cannot overwrite existing storage records, address collisions could still cause unnecessary confusion.

Consider using the ERC-1967 mechanism to choose these locations.

Update: Resolved in pull request #3174.

Unused Parameters

Consider removing the following unused parameters to improve code clarity and save gas:

The dataParentHash parameter in the SubmissionData struct
The dataParentHash parameter in the SupportingSubmissionData struct
The firstBlockInData parameter in the StoredSubmissionData struct
The finalDataHash parameter in the FinalizationDataV2 struct

Update: Resolved in pull request #3177 and pull request #3183.

Unnecessary Computation

In the LineaRollup contract, an operator can submit data by either calling the submitBlobs function or the submitDataAsCalldata function. In both functions, the caller is required to pass in a ParentShnarfData struct. The data in this struct is used here and here to compute the claimed parent shnarf. However, as the parent information is user-provided, and the user already provides the parentShnarf through the SubmissionData and SupportingSubmissionData structs, this computation is unnecessary and provides no additional security guarantees.

Consider removing the ParentShnarfData as a parameter to the submitBlobs and submitDataAsCalldata functions and using the parentShnarf field instead.

Update: Resolved in pull request #3177.

Grammatical Error

The submitBlobs function has a grammatically incorrect clause, stating "the intermediate are not stored".

Consider correcting the above incorrect comment to improve the readability of the codebase.

Update: Resolved in pull request #3176.

Redundant Check

In the submitDataAsCalldata function, there are multiple checks to ensure the consistency of the passed-in data, which is essential for the protocol. However, this validation is redundant as it has already been checked inside the _validateSubmissionData function.

Consider removing this redundant check to avoid code duplication and to reduce gas costs.

Update: Resolved in pull request #3177.

Client Reported

Token Bridge Updates

After the audit was completed, the Linea team shared the following issues and optimizations reported by Cyfrin relating to the TokenBridge contract.

The _safeDecimal function defaults to 18 if the decimals cannot be retrieved. However, this could lead to an inconsistency between the L1 and L2 tokens. Moreover, ERC-721 tokens can only be bridged in one direction. The function should revert instead.
The setCustomContract function can override an existing nativeToBridgedToken record.
The removeReserved function should emit an event to facilitate off-chain processing.
The bridgeTokenWithPermit function modifiers are redundant because they are already included in the bridgeToken invocation.
The nativeToken variable is assigned but not used.
The sourceChainId record in the bridgeToken and removeReserved functions could be assigned to a local variable to avoid multiple storage reads of the same value.
This validation could reverse the two conditions so the more common case fails first.
Instead of assigning to the bridgedMappingValue variable and then optionally copying the result to the nativeToken variable, the nativeToken could be used in both case (and possibly overwritten).

Update: Resolved in pull request #3249.

Reinitializer Synchronization

The initializeParentShnarfsAndFinalizedState function uses reinitializer version 4, which is consistent with the current mainnet version 3. However, the Sepolia deployment already uses version 4. For simplicity, the codebase should set it to version 5 so that the same deployment is valid on both chains.

Update: Resolved in pull request #3249.

Event processing incompatibility

Pull Request #36 updated the BridgingInitiated and BridgingFinalized events to index the recipient parameter instead of the amount parameter. However, this is not a backwards-compatible change with existing off-chain event processing functionality. Instead, the original interface should be restored and the new interface can be implemented with a new event.

Update: Resolved in pull request #3288.

Conclusion

The audited codebase adds gas optimizations and support for submitting multiple blobs together. One high-severity issue was found along with a few lower-severity issues. The codebase was found to be well-written and well-documented. We appreciated the Linea team's cooperation throughout the engagement.