December 4, 2024
Table of Contents
Summary
- Type
- DeFi
- Timeline
- From 2024-11-04
- To 2024-11-15
- Languages
- Solidity
- Total Issues
- 11 (11 resolved)
- Critical Severity Issues
- 0 (0 resolved)
- High Severity Issues
- 0 (0 resolved)
- Medium Severity Issues
- 1 (1 resolved)
- Low Severity Issues
- 3 (3 resolved)
- Notes & Additional Information
- 7 (7 resolved)
Scope
We performed a diff audit of the Consensys/linea-monorepo repository, comparing the commit tagged contract-freeze-2024-11-01 with the commit tagged contract-audit-2024-05-24.
Update: We have completed the review of all submissions and finalized the audit report. The final commit reviewed is df30415. In addition, we have verified the deployed bytecode matches the code at the aforementioned audited final commit and note that there have been no additional changes to in-scope files since adb097a.
In scope were the following files:
contracts
├── LineaRollup.sol
├── ZkEvmV2.sol
├── interfaces
│ ├── IGenericErrors.sol
│ ├── IMessageService.sol
│ ├── IPauseManager.sol
│ ├── IPermissionsManager.sol
│ ├── IRateLimiter.sol
│ ├── l1
│ │ ├── IL1MessageManager.sol
│ │ ├── IL1MessageManagerV1.sol
│ │ ├── IL1MessageService.sol
│ │ ├── ILineaRollup.sol
│ │ ├── IPlonkVerifier.sol
│ │ └── IZkEvmV2.sol
│ └── l2
│ ├── IL2MessageManager.sol
│ └── IL2MessageServiceV1.sol
├── lib
│ ├── L2MessageServicePauseManager.sol
│ ├── LineaRollupPauseManager.sol
│ ├── PauseManager.sol
│ ├── PermissionsManager.sol
│ ├── TokenBridgePauseManager.sol
│ └── Utils.sol
└── messageService
│ ├── MessageServiceBase.sol
│ ├── l1
│ │ ├── L1MessageManager.sol
│ │ ├── L1MessageService.sol
│ │ ├── TransientStorageReentrancyGuardUpgradeable.sol
│ │ └── v1
│ │ ├── L1MessageManagerV1.sol
│ │ └── L1MessageServiceV1.sol
│ ├── l2
│ │ ├── L2MessageManager.sol
│ │ ├── L2MessageService.sol
│ │ └── v1
│ │ ├── L2MessageManagerV1.sol
│ │ └── L2MessageServiceV1.sol
│ └── lib
│ ├── MessageHashing.sol
│ ├── RateLimiter.sol
│ ├── SparseMerkleTreeVerifier.sol
│ ├── TimeLock.sol
│ └── TransientStorageHelpers.sol
└── tokenBridge
├── TokenBridge.sol
├── interfaces
│ └── ITokenBridge.sol
└── lib
└── StorageFiller39.sol
System Overview
The system under review has already been described in our previous audit report. In this diff audit, we reviewed several significant changes made to the codebase, with the most notable ones being the following:
- Removal of Block Numbers from Blob Submissions: Block numbers were removed from blob submissions, events, and checks. Block numbers, which only become valid after verification, were eliminated to reduce gas and calldata costs and prevent confusion. The
shnarf
process now handles checks like emptyparentShnarf
andfinalStateRootHash
. - State Reconstruction-Compatible Events: Events related to blob submission and finalization were modified to support L2 state reconstruction, removing the need for block numbers. This change simplifies data retrieval for state reconstruction.
- Granular Role Reconfiguration and Updated
TokenBridge
Roles and Permissions: Permissions across contracts were reworked to a more granular level to allow for finer control of access roles. These permissions will be set/reset as part of the upgrade transaction forLineaRollup
,L2MessageService
, andTokenBridge
. In theTokenBridge V2
, roles and permissions were further refined. The storage layout was adjusted to avoid data misalignment after the upgrade and slot data cleanup was performed. - Removal of Finalization without Proof: The
finalizeBlocksWithoutProof
function was removed. Now, all finalizations must include a proof. - Permissionless Operator Role Grant: A new permissionless function was added. This function grants the
OPERATOR_ROLE
to a pre-designated fallback operator address if no finalization has occurred in the last six months. If the current block time is greater than or equal to the last finalized timestamp plus six months, any address can call this function to assign theOPERATOR_ROLE
to the fallback operator. Once assigned, anyone can finalize blocks through the fallback operator address. If regular finalizations resume, the security council can revoke theOPERATOR_ROLE
from the fallback operator to reset the counter.
Other changes made to the codebase include changing the Solidity pragma to 0.8.26, code optimizations, improvements to errors, NatSpec additions, and the implementation of recommendations made in previous audits.
Security Model and Trust Assumptions
The updates made to the contracts introduce role-based permissions for controlling access to business-critical functions. These changes have been implemented across the codebase, with particular focus on their application to the TokenBridge
contract, as the update involves modifications to the storage layout of the TokenBridge
proxy contract. Crucially, it is assumed that a pending owner has not been set in the TokenBridge
contract and that the bridge will not be paused at the time of the upgrade.
Given that the audited codebase is an existing system, newly added initialization and reinitialization functions have been included in the LineaRollup
and TokenBridge
contracts. It is assumed that the deployments and initializations are done atomically to prevent the front-running of the initialization process. In addition, it is assumed that the correct values are passed during the upgrade in order to maintain consistency throughout the protocol and ensure correct initialization of the role-based access controls.
Privileged Roles
As described above, extensive role-based permissions for access control have been introduced in the update. These affect multiple functions throughout the codebase. A comprehensive list of these privileged roles can be found in Appendix A of this report.
Medium Severity
Denial of Service in Finalization Blocks Due to Fallback Operator Role Renunciation
In the rollup contract, the setFallbackOperator
function allows the fallbackOperator
to assume the OPERATOR_ROLE
if six months have passed since the last finalization. The fallback operator is designed to be assigned to a publicly accessible address, such as a multicall address or well-known public key, allowing any user to initiate a finalization. Crucially, the setFallbackOperator
function is only meant to be called in the worst-case scenario when the Linea team is unable to maintain the finalization of blocks. It, is in essence, a fallback of last resort.
However, this setup introduces a vulnerability: any user can call finalizeBlocks
, which updates the currentFinalizedState
and advances the last finalized L2 block timestamp. The user can then renounce the OPERATOR_ROLE
, rendering the fallback address unable to regain the OPERATOR_ROLE
for another six months. This renunciation-reset cycle can be repeated indefinitely, effectively preventing the fallback operator from maintaining a stable role and causing significant delays in the finalizations, up to six months each time. In the context of this worst-case scenario, it is unlikely that the Linea team would be able to intervene and set a new operator, making this a permanent issue.
Consider preventing the fallback operator address from being able to call renounceRole
.
Update: Resolved in pull request #298. The Linea team stated:
Acknowledged. We have overridden the
renounceRole
function and the transaction now reverts if the account being reverted is thefallbackOperator
.
Low Severity
Missing Zero-Address Checks
When assigning an address from a user-provided parameter, it is crucial to ensure that the provided address is not zero. Setting an address to zero is problematic because it has special burn/renounce connotations. This action should be handled by a separate function to prevent accidental loss of access during value or ownership transfers.
Throughout the codebase, multiple instances of assignment operations missing zero-address checks were identified:
- The
_defaultAdmin
assignment operation within the contractL2MessageService
inL2MessageService.sol
- The
_fallbackOperator
assignment operation within the contractLineaRollup
inLineaRollup.sol
- The
_tokens
assignment operation within the contractTokenBridge
inTokenBridge.sol
- The
_nativeTokens
assignment operation within the contractTokenBridge
inTokenBridge.sol
Consider performing a zero-address check before assigning a state variable.
Update: Resolved in pull request #305. The Linea team stated:
Partially acknowledged:
_defaultAdmin
and_fallbackOperator
do not have checks._tokens
and_nativeTokens
will never encounter the scenario of an empty address because the transaction would revert upon token deployment checks.setCustomContract
andsetReserved
already have default checks.- We noticed that
_tokens
did not have an empty list check which we have now fixed as well.
Incorrect Docstrings
Throughout the codebase, multiple instances of incorrect or incomplete docstrings were identified:
-
In the
IPlonkVerifier.sol
interface, theVerify
function has an incomplete docstring where the named return value is not documented. -
In the
LineaRollup
contract, theshnarfFinalBlockNumbers
variable has been replaced withblobShnarfExists
. The newly added comment for this variable repeats theNB: NB:
characters.
Consider completing the return variable docstring for the Verify
function of the IPlonkVerifier
interface and correcting the additional comment on blobShnarfExists
.
Update: Resolved in pull request #304.
Missing Docstrings
Throughout the codebase, multiple instances of missing docstrings were identified:
- In
L1MessageManager.sol
, therollingHashes
state variable - In
L1MessageManager.sol
, thel2MerkleRootsDepths
state variable - In
L2MessageManager.sol
, thelastAnchoredL1MessageNumber
state variable - In
LineaRollup.sol
, thecurrentFinalizedShnarf
state variable - In
MessageServiceBase.sol
, themessageService
state variable - In
MessageServiceBase.sol
, theremoteSender
state variable - In
TokenBridge.sol
, thetokenBeacon
state variable - In
TokenBridge.sol
, thenativeToBridgedToken
state variable - In
ZkEvmV2.sol
, thecurrentL2BlockNumber
state variable - In
ZkEvmV2.sol
, thestateRootHashes
state variable - In
ZkEvmV2.sol
, theverifiers
state variable
Consider adding docstrings for the above-listed state variables, and also for the newly added role-related constants. In addition, consider adding a comment to the SIX_MONTHS_IN_SECONDS
constant in the LineaRollup
contract to note that the approximation of 6 months is intentional.
Overall, consider thoroughly documenting all functions (and their parameters) that are part of any contract's public API. Functions implementing sensitive functionality, even if not public, should be clearly documented as well. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).
Update: Resolved in pull request #307. The Linea team stated:
Acknowledged. We adjusted the suggested contracts and additional areas as recommended.
Notes & Additional Information
Lack of Security Contact
Providing a specific security contact (such as an email or ENS name) within a smart contract significantly simplifies the process for individuals to communicate if they identify a vulnerability in the code. This practice is quite beneficial as it permits the code owners to dictate the communication channel for vulnerability disclosure, eliminating the risk of miscommunication or failure to report due to a lack of knowledge on how to do so. In addition, if the contract incorporates third-party libraries and a bug surfaces in those, it becomes easier for their maintainers to contact the appropriate person about the problem and provide mitigation instructions.
Throughout the codebase, all contracts except the Utils
library have a security contact.
Consider adding a NatSpec comment containing a security contact above the library definition which conforms to the style used throughout the codebase: @custom:security-contact security-report@linea.build
Update: Resolved in pull request #301. The Linea team stated:
Acknowledged. We have added the NatSpec for the library title and the security contact.
Lack of Indexed Event Parameters
In the IPauseManager.sol
interface, some of the events do not have indexed parameters:
To improve the ability of off-chain services to search and filter for specific events, consider indexing event parameters.
Update: Resolved in pull request #302. The Linea team stated:
Acknowledged. The events are now indexed.
Inconsistent Integer Base
In assembly, memory offsets can be declared using decimals or hexadecimals. However, it is important to use a consistent notation to avoid confusion and to make it easy to understand the purpose and usage of these sizes.
In the LineaRollup
contract, memory offsets have been declared inconsistently. One memory offset has been declared using decimals, while the rest have been declared using hexadecimals.
Consider using a consistent notation for expressing memory offsets throughout the codebase.
Update: Resolved in pull request #300. The Linea team stated:
Acknowledged. We are now using
0x20
instead of the 32 value.
Magic Numbers in the Code
Throughout the TokenBridge
contract, multiple instances of literal values with unexplained meanings were identified:
- The
32
literal number inTokenBridge.sol
- The
64
literal number inTokenBridge.sol
- The
32
literal number inTokenBridge.sol
- The
32
literal number inTokenBridge.sol
Consider defining and using constant
variables instead of using literals, or adding code comments (where appropriate) to improve the readability of the codebase.
Update: Resolved in pull request #303. The Linea team stated:
Acknowledged. This has been fixed with 3 constant values.
Calls Can Be Front-Run
Calls to EIP-2612's permit()
or OpenZeppelin Governance's delegateBySig()
functions can be front-run by a malicious actor. As a result, the user whose call has been front-run will not be able to perform any action once the call ends since their transaction will revert. The IERC20PermitUpgradeable(_token).permit(msg.sender, address(this), amount, deadline, v, r, s)
call in TokenBridge.sol
is vulnerable to such a DoS attack.
Consider properly wrapping vulnerable calls within try-catch
blocks to avoid DoS attacks.
Update: Resolved in pull request #306. The Linea team stated:
Acknowledged. We have fixed this with an alternate approach of only using the permit if there is not enough allowance. In the case of front-running, the token bridge would have an allowance and the permit call would not be used.
Function Visibility Overly Permissive
Throughout the codebase, multiple instances of functions with unnecessarily permissive visibility were identified:
- The
setMessageService
function inTokenBridge.sol
withpublic
visibility could be limited toexternal
. - The
setReserved
function inTokenBridge.sol
withpublic
visibility could be limited toexternal
.
To better convey the intended use of functions and to potentially realize some additional gas savings, consider changing a function's visibility to be only as permissive as required.
Update: Resolved in pull request #299.
Inconsistent Use of whenTypeAndGeneralNotPaused
Modifier
In the TokenBridge
contract, the check for whenTypeAndGeneralNotPaused
is performed manually on the bridgeToken
function instead of using the whenTypeAndGeneralNotPaused
modifier. This introduces inconsistency in the codebase, as the modifier is used in other parts of the contract to enforce the same check.
Consider using the whenTypeAndGeneralNotPaused
modifier consistently for improved code clarity and readability.
Update: Resolved. The Linea team stated:
Due to
nonZeroAddress(_token) nonZeroAddress(_recipient) nonZeroAmount(_amount) nonReentrant
and the 3 function parameters, the stack gets too deep and this does not work.
Conclusion
This audit covered the diff between the commits tagged as contract-audit-05-24
and contract-freeze-2024-11-01
. The changes related to multiple additions and optimizations across the codebase, most notably regarding blob submissions, role-based access controls, and the setting of a fallback operator.
The audit uncovered several issues and made recommendations aimed at improving code consistency, readability, and gas efficiency. We would like to thank the Linea team for providing comprehensive documentation regarding the changes made, as well as for their prompt responses to the questions posed by the audit team.
Appendix A
The updates to the codebase have introduced multiple privileged roles to the codebase. The list includes, but is not limited to:
L1_L2_MESSAGE_SETTER_ROLE
: access toanchorL1L2MessageHashes
.PAUSE_L1_L2_ROLE
: pausing of L1 to L2 messaging.UNPAUSE_L1_L2_ROLE
: unpausing of L1 to L2 messaging.PAUSE_L2_L1_ROLE
: pausing of L2 to L1 messaging.UNPAUSE_L2_L1_ROLE
: unpausing of L2 to L1 messaging.MINIMUM_FEE_SETTER_ROLE
: setting of theminimumFeeInWei
.VERIFIER_UNSETTER_ROLE
: unset trusted verifier.PAUSE_BLOB_SUBMISSION_ROLE
: pause blob submissions.UNPAUSE_BLOB_SUBMISSION_ROLE
: unpause blob submissions.PAUSE_FINALIZATION_ROLE
: pause finalization of blocks.UNPAUSE_FINALIZATION_ROLE
: unpause finalization of blocks.PAUSE_ALL_ROLE
: general system pause.UNPAUSE_ALL_ROLE
: general system unpause.RATE_LIMIT_SETTER_ROLE
: reset rate limit.USED_RATE_LIMIT_RESETTER_ROLE
: reset amount used in period.SET_MESSAGE_SERVICE_ROLE
: set the message service.SET_REMOTE_TOKENBRIDGE_ROLE
: set the remote token bridge.SET_RESERVED_TOKEN_ROLE
: set a token as reserved.REMOVE_RESERVED_TOKEN_ROLE
: remove reserved status for a token.SET_CUSTOM_CONTRACT_ROLE
: set a token to use a custom contract.PAUSE_INITIATE_TOKEN_BRIDGING_ROLE
: pauses token bridging initiation.UNPAUSE_INITIATE_TOKEN_BRIDGING_ROLE
: unpause token bridging initiation.PAUSE_COMPLETE_TOKEN_BRIDGING_ROLE
: pause bridging completion.UNPAUSE_COMPLETE_TOKEN_BRIDGING_ROLE
: unpause bridging completion.OPERATOR_ROLE
: allows data submission as blobs and/or calldata, as well as finalization of blocks.