Arbitrum BoLD - K42's results

QA Report for Arbitrum-Foundation by K42

• I made sure these are all unique in relation to the 4naly3er report.

Summary

Table of Contents

• Summary
• Low Severity Issues
• Non-Critical Issues

Low Severity Issues

1. Error.sol: Inconsistent Error Parameter Names

   • Description: The DataTooLarge error is defined with dataLength and maxDataLength parameters, but when it is used in the SequencerInbox contract, fullDataLen and maxDataSize are passed instead. This inconsistency can lead to confusion and make the code harder to understand and maintain.
   • Recommendation: Rename the error parameters in the DataTooLarge error definition to match the variable names used in the SequencerInbox contract.
   • Code Snippet:

     error DataTooLarge(uint256 fullDataLen, uint256 maxDataSize);

2. SequencerInbox.sol: Unused Errors

   • Description: The ForceIncludeTimeTooSoon errors are imported but never used in the contract.
   • Recommendation: Remove the unused errors or use them where appropriate.
   • Code Snippet:

     // Remove unused errors
     // error ForceIncludeTimeTooSoon();

3. ISequencerInbox.sol: Similar Parameter Names

   • Description: The addSequencerL2Batch, addSequencerL2BatchFromBlobs, addSequencerL2BatchFromBlobsDelayProof, addSequencerL2BatchFromOriginDelayProof, and addSequencerL2BatchDelayProof functions have similar parameter names (prevMessageCount and newMessageCount), which could lead to confusion when implementing or using these functions.
   • Recommendation: Rename the parameters to be more descriptive and distinguishable, such as prevMessageCount and updatedMessageCount.

4. DelayBuffer.sol: Integer Division in calcBuffer Function

   • Description: The calcBuffer function uses integer division, which may result in loss of precision.
   • Recommendation: Use a safe math library or add overflow/underflow checks.
   • Code Snippet:

     uint256 buffer = Math.max(minBuffer, dataSize.div(minSize));

5. AssertionStakingPool.sol: Unchecked Return Value of safeIncreaseAllowance

   • Description: The safeIncreaseAllowance function from SafeERC20 is used without checking the return value.
   • Recommendation: Check the return value of safeIncreaseAllowance to ensure the operation was successful.
   • Code Snippet:

     require(token.safeIncreaseAllowance(to, amount), "Allowance increase failed");

6. RollupUserLogic.sol: Misleading Comment on initialize Function

   • Description: The initialize function has a comment stating it "shouldn't write to state during init," but it does write state by validating _stakeToken.
   • Recommendation: Clarify the comment or modify the function to match the comment's intention.
   • Code Snippet:

     // @dev the user logic validates configuration and ensures consistency on parameters.

7. RollupProxy.sol: Unused ContractDependencies Struct

   • Description: The ContractDependencies struct is not imported as it should be in this contract.
   • Recommendation: Import the file that defines the ContractDependencies struct in the way below, for better practice.
   • Code Snippet:

     // Import ContractDependencies struct like this for better practice
     // import { ContractDependencies } from "...";

8. RollupCreator.sol: Unused receive Function

   • Description: The receive function is defined but not used in this contract.
   • Recommendation: Consider removing the unused receive function or using it where appropriate.
   • Code Snippet:

     // Remove unused function
     // receive() external payable {...}

9. RollupCore.sol: Unbounded Array Growth

   • Description: The _stakerList array can grow unbounded, which may lead to high gas costs when looping through it. Consider using a more gas-efficient data structure if the list is expected to grow large.
   • Recommendation: Evaluate the expected size of _stakerList and consider using a more gas-efficient data structure if needed.

10. RollupCore.sol: Multiple Entries for the Same Staker in _stakerMap

    • Description: The _stakerMap mapping uses the staker's address as the key, which may lead to multiple entries for the same staker if they stake from different addresses.
    • Recommendation: Use a unique identifier for each staker in the _stakerMap mapping to avoid duplicate entries.
    • Code Snippet:

      mapping(address => uint256) private stakerMap;

11. RollupAdminLogic.sol: High Gas Costs in forceRefundStaker Function

    • Description: The forceRefundStaker function loops through the provided staker array, which may lead to high gas costs if the array is large. Consider adding a maximum limit to the number of stakers that can be processed in a single transaction.
    • Recommendation: Add a maximum limit to the number of stakers that can be processed in the forceRefundStaker function to avoid high gas costs.
    • Code Snippet:

    function forceRefundStaker(address[] calldata stakers) external { uint256 maxBatchSize = 100; for (uint256 i = 0; i < stakers.length && i < maxBatchSize; i++) { // function logic } }

12. IRollupAdmin.sol: Missing Description for Event Parameter

    • Description: The OwnerFunctionCalled event is missing a description of the id parameter, which may make it harder to understand its purpose.
    • Recommendation: Add a comment describing the purpose of the id parameter in the OwnerFunctionCalled event.
    • Code Snippet:

      event OwnerFunctionCalled(uint256 id); // id represents the function identifier

13. Config.sol: Mixed Immutable and Mutable Parameters

    • Description: The Config struct contains a mix of immutable and mutable parameters, which may be confusing.
    • Recommendation: Split the Config struct into separate structs for immutable and mutable parameters or add comments to clarify their mutability.
    • Code Snippet:

      struct ImmutableConfig {
          uint256 param1;
      }
      
      struct MutableConfig {
          uint256 param2;
      }

14. RollupUserLogic.sol: Lack of Error Handling for safeTransfer Function

• Description: The receiveTokens function uses safeTransferFrom without explicitly handling errors. Although safeTransferFrom reverts on failure, adding a require statement would provide explicit error handling and improve readability.
• Recommendation: Add a require statement to handle errors explicitly.
• Code Snippet:

  function receiveTokens(uint256 tokenAmount) private {
      require(IERC20(stakeToken).safeTransferFrom(msg.sender, address(this), tokenAmount), "Transfer failed");
  }

15. RollupUserLogic.sol: Inconsistent Error Messages

• Description: The contract uses different error messages for similar checks. For example, "WHITELIST_DISABLED" and "VALIDATOR_NOT_AFK" could be more descriptive.
• Recommendation: Standardize error messages to provide clearer context.
• Code Snippet:

  require(!validatorWhitelistDisabled, "Whitelist is already disabled");
  require(_validatorIsAfk(), "Validator is not inactive");

16. ChallengeEdgeLib.sol: Inconsistent Error Handling in newEdgeChecks

• Description: The newEdgeChecks function uses direct if statements for error handling, leading to inconsistencies.
• Recommendation: Standardize error handling by using require statements to improve readability and maintainability.
• Code Snippet:

  function newEdgeChecks(
      bytes32 originId,
      bytes32 startHistoryRoot,
      uint256 startHeight,
      bytes32 endHistoryRoot,
      uint256 endHeight
  ) internal pure {
      require(originId != bytes32(0), "EmptyOriginId");
      require(startHeight < endHeight, "InvalidHeights");
      require(startHistoryRoot != bytes32(0), "EmptyStartRoot");
      require(endHistoryRoot != bytes32(0), "EmptyEndRoot");
  }

17. ChallengeEdgeLib.sol: Redundant Parameter Checks in newLayerZeroEdge and newChildEdge

• Description: The newLayerZeroEdge and newChildEdge functions both call newEdgeChecks, which checks for redundant conditions already checked in their respective bodies.
• Recommendation: Remove redundant checks to avoid unnecessary code duplication and enhance efficiency.
• Code Snippet:

  function newLayerZeroEdge(
      bytes32 originId,
      bytes32 startHistoryRoot,
      uint256 startHeight,
      bytes32 endHistoryRoot,
      uint256 endHeight,
      bytes32 claimId,
      address staker,
      uint8 level
  ) internal view returns (ChallengeEdge memory) {
      require(staker != address(0), "EmptyStaker");
      require(claimId != bytes32(0), "EmptyClaimId");
      newEdgeChecks(originId, startHistoryRoot, startHeight, endHistoryRoot, endHeight);
  
      return ChallengeEdge({
          originId: originId,
          startHeight: startHeight,
          startHistoryRoot: startHistoryRoot,
          endHeight: endHeight,
          endHistoryRoot: endHistoryRoot,
          lowerChildId: bytes32(0),
          upperChildId: bytes32(0),
          createdAtBlock: uint64(block.number),
          claimId: claimId,
          staker: staker,
          status: EdgeStatus.Pending,
          level: level,
          refunded: false,
          confirmedAtBlock: 0,
          totalTimeUnrivaledCache: 0
      });
  }

18. ChallengeEdgeLib.sol: Unnecessary Explicit Zero Initialization

• Description: Explicitly initializing variables to zero is unnecessary since Solidity initializes them to zero by default.
• Recommendation: Remove explicit zero initializations to clean up the code.
• Code Snippet:

  return ChallengeEdge({
      originId: originId,
      startHeight: startHeight,
      startHistoryRoot: startHistoryRoot,
      endHeight: endHeight,
      endHistoryRoot: endHistoryRoot,
      lowerChildId: bytes32(0), // No need to explicitly initialize to zero
      upperChildId: bytes32(0), // No need to explicitly initialize to zero
      createdAtBlock: uint64(block.number),
      claimId: claimId,
      staker: staker,
      status: EdgeStatus.Pending,
      level: level,
      refunded: false,
      confirmedAtBlock: 0,
      totalTimeUnrivaledCache: 0
  });

19. ChallengeEdgeLib.sol: Potential Gas Optimization in mutualId and id Functions

• Description: The mutualId and id functions could be optimized

by using local variables for repeated access of ChallengeEdge properties.

• Recommendation: Use local variables to reduce repeated SLOAD operations, saving gas.
• Code Snippet:

  function mutualId(ChallengeEdge storage ce) internal view returns (bytes32) {
      uint8 level = ce.level;
      bytes32 originId = ce.originId;
      uint256 startHeight = ce.startHeight;
      bytes32 startHistoryRoot = ce.startHistoryRoot;
      uint256 endHeight = ce.endHeight;
      return mutualIdComponent(level, originId, startHeight, startHistoryRoot, endHeight);
  }
  
  function id(ChallengeEdge storage edge) internal view returns (bytes32) {
      uint8 level = edge.level;
      bytes32 originId = edge.originId;
      uint256 startHeight = edge.startHeight;
      bytes32 startHistoryRoot = edge.startHistoryRoot;
      uint256 endHeight = edge.endHeight;
      bytes32 endHistoryRoot = edge.endHistoryRoot;
      return idComponent(level, originId, startHeight, startHistoryRoot, endHeight, endHistoryRoot);
  }

20. EdgeChallengeManagerLib.sol: Lack of Documentation Comments

    • Description: The public and internal functions in the EdgeChallengeManagerLib library lack documentation comments explaining their purpose and usage.
    • Recommendation: Add documentation comments to provide clarity on the functions' usage and expected behavior.
    • Code Snippet:

      /// @notice Adds a new edge to the store
      /// @param store The store to add the edge to
      /// @param edge The edge to add
      function add(EdgeStore storage store, ChallengeEdge memory edge) internal returns (EdgeAddedData memory) {
          ...
      }

21. EdgeChallengeManagerLib.sol: Unchecked Return Value in bisectEdge

• Description: The bisectEdge function does not check the return value of the verifyPrefixProof function, which could lead to unexpected behavior if the verification fails.
• Recommendation: Check the return value of verifyPrefixProof and handle any potential failures appropriately.
• Code Snippet:

  (bytes32[] memory preExpansion, bytes32[] memory proof) = abi.decode(prefixProof, (bytes32[], bytes32[]));
  bool proofVerified = MerkleTreeLib.verifyPrefixProof(
      bisectionHistoryRoot, middleHeight + 1, ce.endHistoryRoot, ce.endHeight + 1, preExpansion, proof
  );
  require(proofVerified, "Prefix proof verification failed");

22. EdgeChallengeManagerLib.sol: Potential Gas Optimization in timeUnrivaledTotal

• Description: The timeUnrivaledTotal function could be optimized by caching the store.edges lookups.
• Recommendation: Use local variables to reduce repeated SLOAD operations, saving gas.
• Code Snippet:

  function timeUnrivaledTotal(EdgeStore storage store, bytes32 edgeId) internal view returns (uint256) {
      ChallengeEdge storage edge = store.edges[edgeId];
      uint256 totalTimeUnrivaled = timeUnrivaled(store, edgeId);
      if (edge.lowerChildId != bytes32(0)) {
          uint256 lowerTimer = store.edges[edge.lowerChildId].totalTimeUnrivaledCache;
          uint256 upperTimer = store.edges[edge.upperChildId].totalTimeUnrivaledCache;
          totalTimeUnrivaled += lowerTimer < upperTimer ? lowerTimer : upperTimer;
      }
      return totalTimeUnrivaled;
  }

23. EdgeChallengeManagerLib.sol: Inconsistent Error Messages

• Description: Some error messages in the EdgeChallengeManagerLib library are inconsistent or unclear, which may make debugging difficult.
• Recommendation: Standardize error messages to provide clear and consistent feedback.
• Code Snippet:

  revert EdgeNotPending(edgeId, store.edges[edgeId].status);
  // Standardize to:
  revert("Edge is not pending: invalid status");

24. EdgeChallengeManagerLib.sol: Missing Event Emissions for Significant State Changes

• Description: The setChildren, setConfirmed, and setRefunded functions perform significant state changes without emitting events.
• Recommendation: Emit events for significant state changes to enhance transparency and facilitate off-chain monitoring.
• Code Snippet:

  event ChildrenSet(bytes32 indexed edgeId, bytes32 lowerChildId, bytes32 upperChildId);
  event EdgeConfirmed(bytes32 indexed edgeId, uint64 confirmedAtBlock);
  event Refunded(bytes32 indexed edgeId);
  
  function setChildren(ChallengeEdge storage edge, bytes32 lowerChildId, bytes32 upperChildId) internal {
      if (edge.lowerChildId != 0 || edge.upperChildId != 0) {
          revert ChildrenAlreadySet(ChallengeEdgeLib.id(edge), edge.lowerChildId, edge.upperChildId);
      }
      edge.lowerChildId = lowerChildId;
      edge.upperChildId = upperChildId;
      emit ChildrenSet(ChallengeEdgeLib.id(edge), lowerChildId, upperChildId);
  }
  
  function setConfirmed(ChallengeEdge storage edge) internal {
      if (edge.status != EdgeStatus.Pending) {
          revert EdgeNotPending(ChallengeEdgeLib.id(edge), edge.status);
      }
      edge.status = EdgeStatus.Confirmed;
      edge.confirmedAtBlock = uint64(block.number);
      emit EdgeConfirmed(ChallengeEdgeLib.id(edge), edge.confirmedAtBlock);
  }
  
  function setRefunded(ChallengeEdge storage edge) internal {
      if (edge.status != EdgeStatus.Confirmed) {
          revert EdgeNotConfirmed(ChallengeEdgeLib.id(edge), edge.status);
      }
      if (!isLayerZero(edge)) {
          revert EdgeNotLayerZero(ChallengeEdgeLib.id(edge), edge.staker, edge.claimId);
      }
      if (edge.refunded == true) {
          revert EdgeAlreadyRefunded(ChallengeEdgeLib.id(edge));
      }
      edge.refunded = true;
      emit Refunded(ChallengeEdgeLib.id(edge));
  }

Non-Critical Issues

1. BridgeCreator.sol: Missing Event Documentation

   • Description: The TemplatesUpdated and ERC20TemplatesUpdated events lack documentation describing their purpose.
   • Recommendation: Add documentation comments to provide clarity on the events' usage.
   • Code Snippet:

     event TemplatesUpdated(uint256 indexed templateId); // This event is emitted when templates are updated

2. BOLDUpgradeAction.sol: Missing Event Documentation

   • Description: The RollupMigrated event lacks documentation describing its purpose and emitted parameters.
   • Recommendation: Add documentation comments to provide clarity on the event's usage and emitted data.

3. AssertionState.sol: Missing Struct Documentation

   • Description: The AssertionState struct lacks documentation explaining its purpose and the meaning of its fields.
   • Recommendation: Add documentation comments to provide clarity on the struct's usage and the significance of each field.

4. UintUtilsLib.sol: Inconsistent Naming Conventions

   • Description: The function names leastSignificantBit and mostSignificantBit use different naming conventions. It would be more consistent to use `

leastSignificantBitandmostSignificantBitorlsbandmsb`. - Recommendation: Rename the functions to follow a consistent naming convention. - Code Snippet: solidity function leastSignificantBit(uint256 x) internal pure returns (uint256) {...} function mostSignificantBit(uint256 x) internal pure returns (uint256) {...}

5. UintUtilsLib.sol: Unnecessary Variable Assignment in leastSignificantBit

   • Description: The isolated variable is assigned the result of ((x - 1) & x) ^ x, but this value is immediately passed to mostSignificantBit without being used elsewhere.
   • Recommendation: Inline the expression directly in the mostSignificantBit function call to avoid the unnecessary variable assignment.
   • Code Snippet:

     return mostSignificantBit(((x - 1) & x) ^ x);

6. MerkleTreeLib.sol: Unclear Error Message in maximumAppendBetween

   • Description: The maximumAppendBetween function reverts with the error message "Both y and z cannot be zero" when y and z are both zero. However, this error message may not provide enough context for the caller to understand the issue.
   • Recommendation: Provide a more descriptive error message that explains the reason for the revert and the expected conditions for y and z.
   • Code Snippet:

     require(y != 0 || z != 0, "Both y and z cannot be zero");

7. MerkleTreeLib.sol: Inefficient Array Creation in appendCompleteSubTree

   • Description: The appendCompleteSubTree function creates a new array next with a length based on the number of most significant bits of postSize. However, this array creation could be optimized by reusing the existing me array when possible.
   • Recommendation: Consider optimizing the array creation by reusing the existing me array when the length remains the same, and only creating a new array when necessary.

8. ChallengeErrors.sol: Inconsistent Naming Convention

   • Description: Some error names use pascalCase while others use camelCase. Inconsistent naming conventions can reduce code readability.
   • Recommendation: Adopt a consistent naming convention for all error definitions.

9. ChallengeEdgeLib.sol: Lack of Documentation Comments

   • Description: The public and internal functions in the ChallengeEdgeLib library lack documentation comments explaining their purpose and usage.
   • Recommendation: Add documentation comments to provide clarity on the functions' usage and expected behavior.
   • Code Snippet:

     /// @notice Common checks to do when adding an edge.
     /// @param originId The origin id of the edge.
     /// @param startHistoryRoot The root of all the states in the history up to the startHeight.
     /// @param startHeight The height of the start history root.
     /// @param endHistoryRoot The root of all the states in the history up to the endHeight.
     /// @param endHeight The height of the end history root.
     function newEdgeChecks(
         bytes32 originId,
         bytes32 startHistoryRoot,
         uint256 startHeight,
         bytes32 endHistoryRoot,
         uint256 endHeight
     ) internal pure {
         if (originId == 0) {
             revert EmptyOriginId();
         }
         if (endHeight <= startHeight) {
             revert InvalidHeights(startHeight, endHeight);
         }
         if (startHistoryRoot == 0) {
             revert EmptyStartRoot();
         }
         if (endHistoryRoot == 0) {
             revert EmptyEndRoot();
         }
     }

10. ChallengeEdgeLib.sol: Missing Event Emissions for Significant State Changes

• Description: The setChildren, setConfirmed, and setRefunded functions perform significant state changes without emitting events.
• Recommendation: Emit events for significant state changes to enhance transparency and facilitate off-chain monitoring.
• Code Snippet:

  event ChildrenSet(bytes32 indexed edgeId, bytes32 lowerChildId, bytes32 upperChildId);
  event EdgeConfirmed(bytes32 indexed edgeId, uint64 confirmedAtBlock);
  event Refunded(bytes32 indexed edgeId);
  
  function setChildren(ChallengeEdge storage edge, bytes32 lowerChildId, bytes32 upperChildId) internal {
      if (edge.lowerChildId != 0 || edge.upperChildId != 0) {
          revert ChildrenAlreadySet(ChallengeEdgeLib.id(edge), edge.lowerChildId, edge.upperChildId);
      }
      edge.lowerChildId = lowerChildId;
      edge.upperChildId = upperChildId;
      emit ChildrenSet(ChallengeEdgeLib.id(edge), lowerChildId, upperChildId);
  }
  
  function setConfirmed(ChallengeEdge storage edge) internal {
      if (edge.status != EdgeStatus.Pending) {
          revert EdgeNotPending(ChallengeEdgeLib.id(edge), edge.status);
      }
      edge.status = EdgeStatus.Confirmed;
      edge.confirmedAtBlock = uint64(block.number);
      emit EdgeConfirmed(ChallengeEdgeLib.id(edge), edge.confirmedAtBlock);
  }
  
  function setRefunded(ChallengeEdge storage edge) internal {
      if (edge.status != EdgeStatus.Confirmed) {
          revert EdgeNotConfirmed(ChallengeEdgeLib.id(edge), edge.status);
      }
      if (!isLayerZero(edge)) {
          revert EdgeNotLayerZero(ChallengeEdgeLib.id(edge), edge.staker, edge.claimId);
      }
      if (edge.refunded == true) {
          revert EdgeAlreadyRefunded(ChallengeEdgeLib.id(edge));
      }
      edge.refunded = true;
      emit Refunded(ChallengeEdgeLib.id(edge));
  }

11. ChallengeEdgeLib.sol: Inconsistent Usage of uint256 and uint64 Types

• Description: The ChallengeEdge struct uses both uint256 and uint64 types for similar fields, leading to inconsistency.
• Recommendation: Standardize the usage of uint256 or uint64 for height and block number fields for consistency.
• Code Snippet:

  struct ChallengeEdge {
      bytes32 originId;
      bytes32 startHistoryRoot;
      uint256 startHeight;
      bytes32 endHistoryRoot;
      uint256 endHeight;
      bytes32 lowerChildId;
      bytes32 upperChildId;
      bytes32 claimId;
      address staker;
      uint64 createdAtBlock;
      uint64 confirmedAtBlock;
      EdgeStatus status;
      uint8 level;
      bool refunded;
      uint64 totalTimeUnrivaledCache;
  }

12. RollupUserLogic.sol: Inconsistent Usage of uint256 and uint64 Types

    • Description: The deployTimeChainId is of type uint256 while block numbers are of type uint64.
    • Recommendation: Standardize the usage of uint256 or uint64 for similar fields for consistency.
    • Code Snippet:

      uint64 internal immutable deployTimeChainId = uint64(block.chainid);

13. RollupUserLogic.sol: Missing Event Emissions for Significant State Changes

    • Description: The removeWhitelistAfterFork and removeWhitelistAfterValidatorAfk functions perform significant state changes without emitting events.
    • Recommendation: Emit events for significant state changes to enhance transparency and facilitate off-chain monitoring.
    • Code Snippet:

      event WhitelistRemoved();
      
      function removeWhitelistAfterFork() external {
          require(!validatorWhitelistDisabled, "WHITELIST_DISABLED");
          require(_chainIdChanged(), "CHAIN_ID_NOT_CHANGED");
          validatorWhitelistDisabled = true;
          emit WhitelistRemoved();
      }
      
      function removeWhitelistAfterValidatorAfk() external {
          require(!validatorWhitelistDisabled, "WHITELIST_DISABLED");
          require(_validatorIsAfk(), "VALIDATOR_NOT_AFK");
          validatorWhitelistDisabled = true;
          emit WhitelistRemoved();
      }

14. RollupUserLogic.sol: Lack of Documentation Comments

    • Description: The public and internal functions in the RollupUserLogic contract lack documentation comments explaining their purpose and usage.
    • Recommendation: Add documentation comments to provide clarity on the functions' usage and expected behavior.
    • Code Snippet:

      /// @notice Initializes the RollupUserLogic contract
      /// @param _stakeToken Address of the staking token
      function initialize(address _stakeToken) external view override onlyProxy {
          require(_stakeToken != address(0), "NEED_STAKE_TOKEN");
      }
      
      /// @notice Removes the validator whitelist after a fork
      function removeWhitelistAfterFork() external {
          require(!validatorWhitelistDisabled, "WHITELIST_DISABLED");
          require(_chainIdChanged(), "CHAIN_ID_NOT_CHANGED");
          validatorWhitelistDisabled = true;
      }
      
      /// @notice Removes the validator whitelist if the validator is inactive
      function removeWhitelistAfterValidatorAfk() external {
          require(!validatorWhitelistDisabled, "WHITELIST_DISABLED");
          require(_validatorIsAfk(), "VALIDATOR_NOT_AFK");
          validatorWhitelistDisabled = true;
      }

15. RollupUserLogic.sol: Inconsistent Error Messages

• Description: The contract uses different error messages for similar checks. For example, "WHITELIST_DISABLED" and "VALIDATOR_NOT_AFK" could be more descriptive.
• Recommendation: Standardize error messages to provide clearer context.
• Code Snippet:

  require(!validatorWhitelistDisabled, "Whitelist is already disabled");
  require(_validatorIsAfk(), "Validator is not inactive");

16. RollupUserLogic.sol: Lack of Error Handling for safeTransfer Function

• Description: The receiveTokens function uses safeTransferFrom without explicitly handling errors. Although safeTransferFrom reverts on failure, adding a require statement would provide explicit error handling and improve readability.
• Recommendation: Add a require statement to handle errors explicitly.
• Code Snippet:

  function receiveTokens(uint256 tokenAmount) private {
      require(IERC20(stakeToken).safeTransferFrom(msg.sender, address(this), tokenAmount), "Transfer failed");
  }

17. BridgeCreator.sol: Missing Event Documentation

    • Description: The TemplatesUpdated and ERC20TemplatesUpdated events lack documentation describing their purpose.
    • Recommendation: Add documentation comments to provide clarity on the events' usage.
    • Code Snippet:

      event TemplatesUpdated(uint256 indexed templateId); // This event is emitted when templates are updated

18. BOLDUpgradeAction.sol: Missing Event Documentation

• Description: The RollupMigrated event lacks documentation describing its purpose and emitted parameters. This makes it harder to understand the significance and usage of the event.
• Recommendation: Add documentation comments to provide clarity on the event's purpose and the meaning of each emitted parameter.
• Code Snippet:

  /// @notice Emitted when a rollup is migrated to a new rollup address
  /// @param rollup The address of the expected rollup contract
  /// @param challengeManager The address of the new challenge manager contract
   emit RollupMigrated(expectedRollupAddress, address(challengeManager));

19. BOLDUpgradeAction.sol: Lack of Detailed Documentation for Critical Functions

• Description: Several critical functions within the contract lack detailed documentation, explaining their purpose, parameters, and behaviour. Below are notes I think are clearer and more in depth.
• Recommendation: Add detailed documentation comments for the critical functions to improve code clarity and maintainability.
• Code Snippet:

  /**
   * @notice Refunds existing stakers, pauses, and upgrades the old rollup to allow stake withdrawals while paused.
   * @dev This function handles the clean-up process for the old rollup, including pausing it and refunding stakes.
   */
  function cleanupOldRollup() private {
      IOldRollupAdmin(address(OLD_ROLLUP)).pause();
  
      uint64 stakerCount = ROLLUP_READER.stakerCount();
      if (stakerCount > 50) {
          stakerCount = 50;
      }
      for (uint64 i = 0; i < stakerCount; i++) {
          address stakerAddr = ROLLUP_READER.getStakerAddress(i);
          OldStaker memory staker = ROLLUP_READER.getStaker(stakerAddr);
          if (staker.isStaked && staker.currentChallenge == 0) {
              address[] memory stakersToRefund = new address[](1);
              stakersToRefund[0] = stakerAddr;
  
              IOldRollupAdmin(address(OLD_ROLLUP)).forceRefundStaker(stakersToRefund);
          }
      }
  
      DoubleLogicUUPSUpgradeable(address(OLD_ROLLUP)).upgradeSecondaryTo(IMPL_PATCHED_OLD_ROLLUP_USER);
  }
  
  /**
   * @notice Creates the configuration for the new rollup using the latest confirmed assertion from the old rollup.
   * @return Config The configuration object for the new rollup.
   * @dev This function fetches the latest confirmed state hash from the old rollup and constructs the genesis state for the new rollup.
   */
  function createConfig() private view returns (Config memory) {
      bytes32 latestConfirmedStateHash = ROLLUP_READER.getNode(ROLLUP_READER.latestConfirmed()).stateHash;
      (ExecutionState memory genesisExecState, uint256 inboxMaxCount) = PREIMAGE_LOOKUP.get(latestConfirmedStateHash);
  
      AssertionState memory genesisAssertionState;
      genesisAssertionState.globalState = genesisExecState.globalState;
      genesisAssertionState.machineStatus = genesisExecState.machineStatus;
  
      require(
          PREIMAGE_LOOKUP.stateHash(genesisAssertionState.toExecutionState(), inboxMaxCount)
              == latestConfirmedStateHash,
          "Invalid latest execution hash"
      );
  
      ISequencerInbox.MaxTimeVariation memory maxTimeVariation;
      BufferConfig memory bufferConfig;
  
      return Config({
          confirmPeriodBlocks: CONFIRM_PERIOD_BLOCKS,
          stakeToken: STAKE_TOKEN,
          baseStake: STAKE_AMOUNT,
          wasmModuleRoot: ROLLUP_READER.wasmModuleRoot(),
          owner: address(this),
          loserStakeEscrow: L1_TIMELOCK,
          chainId: CHAIN_ID,
          chainConfig: "",
          miniStakeValues: ConstantArrayStorage(MINI_STAKE_AMOUNTS_STORAGE).array(),
          sequencerInboxMaxTimeVariation: maxTimeVariation,
          layerZeroBlockEdgeHeight: BLOCK_LEAF_SIZE,
          layerZeroBigStepEdgeHeight: BIGSTEP_LEAF_SIZE,
          layerZeroSmallStepEdgeHeight: SMALLSTEP_LEAF_SIZE,
          genesisAssertionState: genesisAssertionState,
          genesisInboxCount: inboxMaxCount,
          anyTrustFastConfirmer: ANY_TRUST_FAST_CONFIRMER,
          numBigStepLevel: NUM_BIGSTEP_LEVEL,
          challengeGracePeriodBlocks: CHALLENGE_GRACE_PERIOD_BLOCKS,
          bufferConfig: bufferConfig
      });
  }
  
  /**
   * @notice Upgrades the surrounding contracts to set the new rollup address.
   * @param newRollupAddress The address of the new rollup contract.
   * @dev This function upgrades the bridge, sequencer inbox, outbox, and rollup event inbox to point to the new rollup address.
   */
  function upgradeSurroundingContracts(address newRollupAddress) private {
      TransparentUpgradeableProxy bridge = TransparentUpgradeableProxy(payable(BRIDGE));
      PROXY_ADMIN_BRIDGE.upgrade(bridge, IMPL_BRIDGE);
      IBridge(BRIDGE).updateRollupAddress(IOwnable(newRollupAddress));
  
      upgradeSequencerInbox();
  
      TransparentUpgradeableProxy inbox = TransparentUpgradeableProxy(payable(INBOX));
      PROXY_ADMIN_INBOX.upgrade(inbox, IMPL_INBOX);
  
      TransparentUpgradeableProxy rollupEventInbox = TransparentUpgradeableProxy(payable(REI));
      PROXY_ADMIN_REI.upgrade(rollupEventInbox, IMPL_REI);
      IRollupEventInbox(REI).updateRollupAddress();
  
      TransparentUpgradeableProxy outbox = TransparentUpgradeableProxy(payable(OUTBOX));
      PROXY_ADMIN_OUTBOX.upgrade(outbox, IMPL_OUTBOX);
      IOutbox(OUTBOX).updateRollupAddress();
  }