Linea Rollup Update

Description

In the initialize function of the LineaRollup contract, the genesis shnarf(GENESIS_SHNARF) is hard-coded, it cannot work with new networks that has a different genesis shnarf without modifying the contract, thus restricts the contract’s adaptability and interoperability across different networks.

Examples

contracts/contracts/LineaRollup.sol:L143-L143

currentFinalizedShnarf = GENESIS_SHNARF;

contracts/contracts/LineaRollup.sol:L398-L414

function _computeShnarf(
  bytes32 _parentShnarf,
  bytes32 _snarkHash,
  bytes32 _finalStateRootHash,
  bytes32 _dataEvaluationPoint,
  bytes32 _dataEvaluationClaim
) internal pure returns (bytes32 shnarf) {
  assembly {
    let mPtr := mload(0x40)
    mstore(mPtr, _parentShnarf)
    mstore(add(mPtr, 0x20), _snarkHash)
    mstore(add(mPtr, 0x40), _finalStateRootHash)
    mstore(add(mPtr, 0x60), _dataEvaluationPoint)
    mstore(add(mPtr, 0x80), _dataEvaluationClaim)
    shnarf := keccak256(mPtr, 0xA0)
  }
}

Recommendation

Remove the hard-coded genesis shnarf, compute it from a parameter passed in the initialize function ( _initializationData.initialStateRootHash).

Description

In the initialize function of the LineaRollup contract, there is no validation for fallback operator address(_initializationData.fallbackOperator) to be non zero. As a result, the fall back operator would fail to work in case of Linea stops submitting blobs and finalizing for 6 months.

Examples

contracts/contracts/LineaRollup.sol:L135

fallbackOperator = _initializationData.fallbackOperator;

Recommendation

Add the missing non-zero validation for fallback operator address.

Description

In the initialize function of the TokenBridge contract, the source chainID(_initializationData.sourceChainId) and target chain ID (_initializationData.targetChainId) of the bridge is not validated to be distinct and neither is set to zero. As a result, incorrect chain ID will be set or identical chain IDs for the source and target chains, which fundamentally compromises the functionality of the bridge by allowing for the possibility of erroneous or unintended bridge operations.

Examples

contracts/contracts/tokenBridge/TokenBridge.sol:L160-L161

sourceChainId = _initializationData.sourceChainId;
targetChainId = _initializationData.targetChainId;

Recommendation

Add the validation for source and target chain ID to ensure they are distinct and non-zero.

Description

In the initialize function of both the LineaRollup and TokenBridge contract, the DEFAULT_ADMIN_ROLE role of the contract is granted to _initializationData.defaultAdmin, which is presumed to be security council account. However there is no validation checks to ensure that _initializationData.defaultAdmin is not a zero address. The absence of such validation could potentially result in the contracts being initialized without a designated Admin, compromising the permission management system within these contracts and leaving the contracts vulnerable to unauthorized access and manipulation.

Examples

contracts/contracts/LineaRollup.sol:L129

_grantRole(DEFAULT_ADMIN_ROLE, _initializationData.defaultAdmin);

contracts/contracts/tokenBridge/TokenBridge.sol:L155

_grantRole(DEFAULT_ADMIN_ROLE, _initializationData.defaultAdmin);

Recommendation

Add validation of non zero address for _initializationData.defaultAdmin in the initialize function for both the LineaRollup and TokenBridge contracts.

Description

The pauseByType and unPauseByType functions allow pausing and unpausing functionality by specifying a PauseType value. However, the UNUSED pause type, which is intended as a default value, can still be used in these functions. This creates a potential issue where someone could unintentionally pause or unpause using the UNUSED type and the transaction will successfully pass, which can make the authorised role think that the execution has been paused with the GENERAL type, but it will have no effect to the system.

Examples

contracts/contracts/lib/PauseManager.sol:L110-L136

/**
 * @notice Pauses functionality by specific type.
 * @dev Requires the role mapped in `_pauseTypeRoles` for the pauseType.
 * @param _pauseType The pause type value.
 */
function pauseByType(PauseType _pauseType) external onlyRole(_pauseTypeRoles[_pauseType]) {
  if (isPaused(_pauseType)) {
    revert IsPaused(_pauseType);
  }

  _pauseTypeStatusesBitMap |= 1 << uint256(_pauseType);
  emit Paused(_msgSender(), _pauseType);
}

/**
 * @notice Unpauses functionality by specific type.
 * @dev Requires the role mapped in `_unPauseTypeRoles` for the pauseType.
 * @param _pauseType The pause type value.
 */
function unPauseByType(PauseType _pauseType) external onlyRole(_unPauseTypeRoles[_pauseType]) {
  if (!isPaused(_pauseType)) {
    revert IsNotPaused(_pauseType);
  }

  _pauseTypeStatusesBitMap &= ~(1 << uint256(_pauseType));
  emit UnPaused(_msgSender(), _pauseType);
}

Recommendation

Add validation to the pauseByType and unPauseByType functions to prevent the use of the UNUSED pause type.

Description

In the _finalizeBlocks function of the LineaRollup contract, the latest block number (_finalizationData.endBlockNumber) in the finalization is validated that it’s greater than the last finalized block number (_lastFinalizedBlock) to ensure the block number sequence is correct during the finalization process. However the block number sequence is already verified in the proof along with the state in the finalization. Therefore this validation to compare the latest and last finalized block numbers is unnecessary.

Examples

contracts/contracts/LineaRollup.sol:L507-L509

if (_finalizationData.endBlockNumber <= _lastFinalizedBlock) {
  revert FinalBlockNumberLessThanOrEqualToLastFinalizedBlock(_finalizationData.endBlockNumber, _lastFinalizedBlock);
}

contracts/contracts/LineaRollup.sol:L486-L494

uint256 publicInput = _computePublicInput(
  _finalizationData,
  lastFinalizedShnarf,
  finalShnarf,
  lastFinalizedBlockNumber,
  _finalizationData.endBlockNumber
);

_verifyProof(publicInput, _proofType, _aggregatedProof);

Recommendation

Remove the validation to compare the latest and last finalized block numbers in _finalizeBlocks.

Description

In the function _computePublicInput of the LineaRollup contract, the parameter _endBlockNumber is redundant as it’s included in the parameter _finalizationData, the function can load the parameter from _finalizationData directly.

Examples

contracts/contracts/LineaRollup.sol:L683-L689

function _computePublicInput(
  FinalizationDataV3 calldata _finalizationData,
  bytes32 _lastFinalizedShnarf,
  bytes32 _finalShnarf,
  uint256 _lastFinalizedBlockNumber,
  uint256 _endBlockNumber
) private pure returns (uint256 publicInput) {

contracts/contracts/interfaces/l1/ILineaRollup.sol:L102-L115

struct FinalizationDataV3 {
  bytes32 parentStateRootHash;
  uint256 endBlockNumber;
  ShnarfData shnarfData;
  uint256 lastFinalizedTimestamp;
  uint256 finalTimestamp;
  bytes32 lastFinalizedL1RollingHash;
  bytes32 l1RollingHash;
  uint256 lastFinalizedL1RollingHashMessageNumber;
  uint256 l1RollingHashMessageNumber;
  uint256 l2MerkleTreesDepth;
  bytes32[] l2MerkleRoots;
  bytes l2MessagingBlocksOffsets;
}

Recommendation

Remove the parameter _endBlockNumber and load it from _finalizationData inside the function using calldatacopy

Description

The LineaRollup contract redundantly imports Initializable from OpenZeppelin:

contracts/contracts/LineaRollup.sol:L4

import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";

However, Initializable is already inherited through AccessControlUpgradeable, which includes the Initializable contract in its own inheritance hierarchy:

abstract contract AccessControlUpgradeable is Initializable, ContextUpgradeable, IAccessControlUpgradeable, ERC165Upgradeable {

The PermissionsManager contract, ZkEvmV2 contract, PauseManager contract, L1MessageServiceV1 contract, L2MessageManagerV1 contract, RateLimiter contract, TokenBridge contract are also redundantly imports Initializable contract:

contracts/contracts/lib/PermissionsManager.sol:L4-L14

import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
import { AccessControlUpgradeable } from "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol";
import { IGenericErrors } from "../interfaces/IGenericErrors.sol";
import { IPermissionsManager } from "../interfaces/IPermissionsManager.sol";

/**
 * @title Contract to manage permissions initialization.
 * @author ConsenSys Software Inc.
 * @custom:security-contact [email protected]
 */
abstract contract PermissionsManager is Initializable, AccessControlUpgradeable, IPermissionsManager, IGenericErrors {

contracts/contracts/tokenBridge/TokenBridge.sol:L14

import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";

contracts/contracts/ZkEvmV2.sol:L4

import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";

contracts/contracts/messageService/lib/RateLimiter.sol:L4

import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";

contracts/contracts/messageService/l1/v1/L1MessageServiceV1.sol:L4

import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";

contracts/contracts/messageService/l2/v1/L2MessageServiceV1.sol:L4

import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";

This duplicate import is unnecessary and increases code complexity without providing additional functionality.

Recommendation

We recommend removing the redundant Initializable import, this cleanup will reduce unnecessary imports, improving code clarity and maintainability. The contract will still compile correctly as the Initializable features are inherited through AccessControlUpgradeable or other contracts.

Description

The _verifyProof function uses abi.encodeWithSelector to encode the function call data for the call operation with the verifier contract. While this approach is functional, it lacks the compile-time type safety provided by abi.encodeCall, introduced in Solidity version 0.8.11. The absence of type checking increases the risk of encoding errors due to mismatches between the expected and actual types, which could lead to runtime failures.

Examples

contracts/contracts/ZkEvmV2.sol:L51-L53

(bool callSuccess, bytes memory result) = verifierToUse.call(
  abi.encodeWithSelector(IPlonkVerifier.Verify.selector, _proof, publicInput)
);

Recommendation

Replace abi.encodeWithSelector with abi.encodeCall to leverage compile-time type checking and adhere to Solidity best practices:

(bool callSuccess, bytes memory result) = verifierToUse.call(
    abi.encodeCall(IPlonkVerifier.Verify, (_proof, publicInput))
);

Description

The __PauseManager_init function is used to initialize the pause and unpause roles during PauseManager contract deployment. However, the design does not include a mechanism to reconfigure or update these roles after initialization, the initialization function is the only one which can modify _pauseTypeRoles and _unPauseTypeRoles mappings. If a role is compromised, there is no way to revoke or reassign it, potentially allowing malicious actors to pause or unpause the system indefinitely, as well as if roles are configured incorrectly during initialization or not configured at all, for example if the pause role has been configured and unpause role hasn’t, the only recourse is to redeploy the contract, which is costly and operationally challenging.

Examples

contracts/contracts/lib/PauseManager.sol:L13-L18

abstract contract PauseManager is Initializable, IPauseManager, AccessControlUpgradeable {
  /// @notice This is used to pause all pausable functions.
  bytes32 public constant PAUSE_ALL_ROLE = keccak256("PAUSE_ALL_ROLE");

  /// @notice This is used to unpause all unpausable functions.
  bytes32 public constant UNPAUSE_ALL_ROLE = keccak256("UNPAUSE_ALL_ROLE");

contracts/contracts/lib/PauseManager.sol:L61-L80

/**
 * @notice Initializes the pause manager with the given pause and unpause roles.
 * @dev This function is called during contract initialization to set up the pause and unpause roles.
 * @param _pauseTypeRoleAssignments An array of PauseTypeRole structs defining the pause types and their associated roles.
 * @param _unpauseTypeRoleAssignments An array of PauseTypeRole structs defining the unpause types and their associated roles.
 */
function __PauseManager_init(
  PauseTypeRole[] calldata _pauseTypeRoleAssignments,
  PauseTypeRole[] calldata _unpauseTypeRoleAssignments
) internal onlyInitializing {
  for (uint256 i; i < _pauseTypeRoleAssignments.length; i++) {
    _pauseTypeRoles[_pauseTypeRoleAssignments[i].pauseType] = _pauseTypeRoleAssignments[i].role;
    emit PauseTypeRoleSet(_pauseTypeRoleAssignments[i].pauseType, _pauseTypeRoleAssignments[i].role);
  }

  for (uint256 i; i < _unpauseTypeRoleAssignments.length; i++) {
    _unPauseTypeRoles[_unpauseTypeRoleAssignments[i].pauseType] = _unpauseTypeRoleAssignments[i].role;
    emit UnPauseTypeRoleSet(_unpauseTypeRoleAssignments[i].pauseType, _unpauseTypeRoleAssignments[i].role);
  }
}

Recommendation

We recommend reviewing the architecture and adding configuration functions if neccesary, for example:

function updatePauseRole(uint256 pauseType, bytes32 newRole) external onlyRole(PAUSE_ALL_ROLE) {
    require(newRole != _pauseTypeRoles[pauseType], "Role already set");
    _pauseTypeRoles[pauseType] = newRole;
    emit PauseTypeRoleSet(pauseType, newRole);
}

function updateUnpauseRole(uint256 pauseType, bytes32 newRole) external onlyRole(UNPAUSE_ALL_ROLE)  {
    require(newRole != _unPauseTypeRoles[pauseType], "Role already set");
    _unPauseTypeRoles[pauseType] = newRole;
    emit UnPauseTypeRoleSet(pauseType, newRole);
}

This would allow for dynamic role reconfiguration while maintaining security through appropriate access controls.

Description

The setVerifierAddress function allows the role with VERIFIER_SETTER_ROLE to update the verifier address for a given proof type to set it, while with the unsetVerifierAddress function the VERIFIER_UNSETTER_ROLE has an access to unset this value. While the access for management of setter and unsetter function is designed to be separated, there are no restrictions for VERIFIER_SETTER_ROLE preventing the overwriting of existing verifier addresses with arbitrary values, such as dead addresses (0xdead) and by doing that successfully unsetting the verifier contract. Separating roles enhance security of the protocol, as if the VERIFIER_SETTER_ROLE is compromised, it shouldn’t be able to unset the verifier blocking all of the execution, but with the current design the setter role can unset values to other dead addresses, only except for zero address, making the VERIFIER_UNSETTER_ROLE role and unsetVerifierAddress function almost redundant, and VERIFIER_SETTER_ROLE role overpowered.

Examples

contracts/contracts/LineaRollup.sol:L30-L34

/// @notice The role required to set/add  proof verifiers by type.
bytes32 public constant VERIFIER_SETTER_ROLE = keccak256("VERIFIER_SETTER_ROLE");

/// @notice The role required to set/remove  proof verifiers by type.
bytes32 public constant VERIFIER_UNSETTER_ROLE = keccak256("VERIFIER_UNSETTER_ROLE");

contracts/contracts/LineaRollup.sol:L189-L204

/**
 * @notice Adds or updates the verifier contract address for a proof type.
 * @dev VERIFIER_SETTER_ROLE is required to execute.
 * @param _newVerifierAddress The address for the verifier contract.
 * @param _proofType The proof type being set/updated.
 */
function setVerifierAddress(address _newVerifierAddress, uint256 _proofType) external onlyRole(VERIFIER_SETTER_ROLE) {
  if (_newVerifierAddress == address(0)) {
    revert ZeroAddressNotAllowed();
  }

  emit VerifierAddressChanged(_newVerifierAddress, _proofType, msg.sender, verifiers[_proofType]);

  verifiers[_proofType] = _newVerifierAddress;
}

contracts/contracts/LineaRollup.sol:L229-L238

/**
 * @notice Unset the verifier contract address for a proof type.
 * @dev VERIFIER_UNSETTER_ROLE is required to execute.
 * @param _proofType The proof type being set/updated.
 */
function unsetVerifierAddress(uint256 _proofType) external onlyRole(VERIFIER_UNSETTER_ROLE) {
  emit VerifierAddressChanged(address(0), _proofType, msg.sender, verifiers[_proofType]);

  delete verifiers[_proofType];
}

Recommendation

We recommend reviewing current roles, and if needed restricting the ability to overwrite existing verifier addresses to cases where the current address is unset (address(0)). For example:

if (verifiers[_proofType] != address(0)) {
    revert("Cannot overwrite existing verifier address");
}