Livepeer contest - WatchPug's results

Handle

WatchPug

Vulnerability details

In the current implementation of DelegatorPool.sol#claim(), it first requires claimedInitialStake < initialStake, or it throws an error of DelegatorPool#claim: FULLY_CLAIMED.

However, since it's an onlyMigrator function, the felicity of _delegator and _stake should be assured by the Migrator contract, otherwise, this require statement itself also can not prevent bad results caused by the wrong inputs.

Furthermore, even if the purpose of this require statement is to make sure that claimedInitialStake can never surpass the initialStake, the expression should be claimedInitialStake + _stake <= initialStake instead of claimedInitialStake < initialStake.

https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L2/pool/DelegatorPool.sol#L58-L93

function claim(address _delegator, uint256 _stake) external onlyMigrator {
    require(
        claimedInitialStake < initialStake,
        "DelegatorPool#claim: FULLY_CLAIMED"
    );

    // Calculate stake owed to delegator
    uint256 currTotalStake = pendingStake();
    uint256 owedStake = (currTotalStake * _stake) /
        (initialStake - claimedInitialStake);

    // Calculate fees owed to delegator
    uint256 currTotalFees = pendingFees();
    uint256 owedFees = (currTotalFees * _stake) /
        (initialStake - claimedInitialStake);

    // update claimed balance
    claimedInitialStake += _stake;

    // Transfer owed stake to the delegator
    transferBond(_delegator, owedStake);

    // Transfer owed fees to the delegator
    IBondingManager(bondingManager).withdrawFees(
        payable(_delegator),
        owedFees
    );

    emit Claimed(_delegator, owedStake, owedFees);
}

Recommandation

Consider removing it or changing to:

require(
    claimedInitialStake + _stake <= initialStake,
    "DelegatorPool#claim: FULLY_CLAIMED"
);

Handle

WatchPug

Vulnerability details

Per the document: https://github.com/livepeer/LIPs/blob/master/LIPs/LIP-73.md#upgrade-process

Phase 1

• The L1 RoundsManager will be upgraded to disable round initialization at LIP_73_ROUND
• During this phase, protocol transactions will be executed normally
• During this phase, the following contracts will be deployed:
  • Protocol contracts on L2
  • Migrator contracts on L1 and L2
  • LPT bridge contracts on L1 and L2
  • All of these contracts will start off paused

However, the current implementation of L1LPTGateway, L2LPTGateway are not automatically paused on deployment.

We recommend adding _pause() to the end of the constructor() in L1LPTGateway, L2LPTGateway, like the constructor of L1Migrator.sol#L143, and unpause() when Phase 2 starts.

This will help avoid tx to happen in an intermediate state between Phase1 and Phase 2, which may cause certain txs to fail, for instance:

When in Phase 1, L1LPTGateway cant calls bridgeMint() on the BridgeMinter to mint LPT to the user, as L1 Minter have not migrateToNewMinter() to BridgeMinter yet. If a user in L2 tries to move LPT from L2 to L1, their tx may fail.

Handle

WatchPug

Vulnerability details

For the storage variables that will be accessed multiple times, cache and read from the stack can save ~100 gas from each extra read (SLOAD after Berlin).

For example:

https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L2/pool/DelegatorPool.sol#L58-L93

    function claim(address _delegator, uint256 _stake) external onlyMigrator {
        require(
            claimedInitialStake < initialStake,
            "DelegatorPool#claim: FULLY_CLAIMED"
        );

        uint256 currTotalStake = pendingStake();
        uint256 owedStake = (currTotalStake * _stake) /
            (initialStake - claimedInitialStake);

        // Calculate fees owed to delegator
        uint256 currTotalFees = pendingFees();
        uint256 owedFees = (currTotalFees * _stake) /
            (initialStake - claimedInitialStake);

        // update claimed balance
        claimedInitialStake += _stake;

        // Transfer owed stake to the delegator
        transferBond(_delegator, owedStake);

        // Transfer owed fees to the delegator
        IBondingManager(bondingManager).withdrawFees(
            payable(_delegator),
            owedFees
        );

        emit Claimed(_delegator, owedStake, owedFees);
    }

initialStake and claimedInitialStake can be cached.

Recommendation

Change to:

    function claim(address _delegator, uint256 _stake) external onlyMigrator {
        uint256 _intialStake = initialStake;
        uint256 _claimedInitialStake = claimedInitialStake;

        require(
            _claimedInitialStake < _initialStake,
            "DelegatorPool#claim: FULLY_CLAIMED"
        );

        uint256 currTotalStake = pendingStake();
        uint256 owedStake = (currTotalStake * _stake) /
            (_initialStake - _claimedInitialStake);

        // Calculate fees owed to delegator
        uint256 currTotalFees = pendingFees();
        uint256 owedFees = (currTotalFees * _stake) /
            (_initialStake - _claimedInitialStake);

        // update claimed balance
        claimedInitialStake += _stake;

        // Transfer owed stake to the delegator
        transferBond(_delegator, owedStake);

        // Transfer owed fees to the delegator
        IBondingManager(bondingManager).withdrawFees(
            payable(_delegator),
            owedFees
        );

        emit Claimed(_delegator, owedStake, owedFees);
    }

Handle

WatchPug

Vulnerability details

https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L1/gateway/L1Migrator.sol#L471-L471

uint256 total = 0;

https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L1/gateway/L1Migrator.sol#L472-L472

for (uint256 i = 0; i < _unbondingLockIds.length; i++)

Setting uint256 variables to 0 is redundant as they default to 0.

Handle

WatchPug

Vulnerability details

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

Instances include:

• L1Migrator.sol#getMigrateUnbondingLocksParams()

  https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L1/gateway/L1Migrator.sol#L472-L472

• L2Migrator.sol#finalizeMigrateUnbondingLocks()

  https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L2/gateway/L2Migrator.sol#L197-L197

Handle

WatchPug

Vulnerability details

Using ++i is more gas efficient than i++, especially in a loop.

We suggest to use unchecked {++i} to even better gas performance. (for >= 0.8)

For example:

https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L1/gateway/L1Migrator.sol#L472-L472

 for (uint256 i = 0; i < _unbondingLockIds.length; i++) 

https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L2/gateway/L2Migrator.sol#L197-L197

for (uint256 i = 0; i < _params.unbondingLockIds.length; i++) 

Handle

WatchPug

Vulnerability details

https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L2/pool/DelegatorPool.sol#L70-L78

        // Calculate stake owed to delegator
        uint256 currTotalStake = pendingStake();
        uint256 owedStake = (currTotalStake * _stake) /
            (initialStake - claimedInitialStake);

        // Calculate fees owed to delegator
        uint256 currTotalFees = pendingFees();
        uint256 owedFees = (currTotalFees * _stake) /
            (initialStake - claimedInitialStake);

The local variable currTotalStake, currTotalFees is used only once. Making the expression inline can save gas.

Similar issue exists in L2Migrator.sol#claimStake(), L1Migrator.sol#migrateETH(), L1Migrator.sol#migrateLPT(), L1ArbitrumMessenger.sol#onlyL2Counterpart().

Recommendation

Change to:

        // Calculate stake owed to delegator
        uint256 owedStake = (pendingStake() * _stake) /
            (initialStake - claimedInitialStake);

        // Calculate fees owed to delegator
        uint256 owedFees = (pendingFees() * _stake) /
            (initialStake - claimedInitialStake);

Handle

WatchPug

Vulnerability details

https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L2/gateway/L2LPTDataCache.sol#L91-L94

return
    l1TotalSupply >= l2SupplyFromL1
        ? l1TotalSupply - l2SupplyFromL1
        : 0;

When l1TotalSupply == l2SupplyFromL1, l1TotalSupply - l2SupplyFromL1 == 0. Therefore, replace >= with > can avoid the unnecessary arithmetic operations and storage reads (l1TotalSupply - l2SupplyFromL1).

Recommendation

Change to:

return
    l1TotalSupply > l2SupplyFromL1
        ? l1TotalSupply - l2SupplyFromL1
        : 0;

Handle

WatchPug

Vulnerability details

Every reason string takes at least 32 bytes.

Use short reason strings that fits in 32 bytes or it will become more expensive.

Instances include:

https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L1/gateway/L1Migrator.sol#L506-L509

        require(
            _l2Addr != address(0),
            "L1Migrator#requireValidMigration: INVALID_L2_ADDR"
        );

https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L1/gateway/L1Migrator.sol#L510-L515

        require(
            msg.sender == _l1Addr ||
                recoverSigner(_structHash, _sig) == _l1Addr,
            "L1Migrator#requireValidMigration: FAIL_AUTH"
        );
    }

https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L2/gateway/L2Migrator.sol#L134-L137

        require(
            !migratedDelegators[_params.l1Addr],
            "L2Migrator#finalizeMigrateDelegator: ALREADY_MIGRATED"
        );

https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L2/gateway/L2Migrator.sol#L184-L184

        require(ok, "L2Migrator#finalizeMigrateDelegator: FAIL_FEE");

https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L2/gateway/L2Migrator.sol#L199-L202

        require(
            !migratedUnbondingLocks[_params.l1Addr][id],
            "L2Migrator#finalizeMigrateUnbondingLocks: ALREADY_MIGRATED"
        );

https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L2/gateway/L2Migrator.sol#L219-L222

        require(
            !migratedSenders[_params.l1Addr],
            "L2Migrator#finalizeMigrateSender: ALREADY_MIGRATED"
        );

https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L2/gateway/L2Migrator.sol#L255-L258

        require(
            claimStakeEnabled,
            "L2Migrator#claimStake: CLAIM_STAKE_DISABLED"
        );

https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L2/gateway/L2Migrator.sol#L267-L270

        require(
            merkleSnapshot.verify(keccak256("LIP-73"), _proof, leaf),
            "L2Migrator#claimStake: INVALID_PROOF"
        );

https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L2/gateway/L2Migrator.sol#L272-L275

        require(
            !migratedDelegators[delegator],
            "L2Migrator#claimStake: ALREADY_MIGRATED"
        );

https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L2/pool/DelegatorPool.sol#L39-L39

        require(msg.sender == migrator, "DelegatorPool#claim: NOT_MIGRATOR");

https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L2/pool/DelegatorPool.sol#L59-L62

        require(
            claimedInitialStake < initialStake,
            "DelegatorPool#claim: FULLY_CLAIMED"
        );

Handle

WatchPug

Vulnerability details

Unused named returns increase contract size and gas usage at deployment.

https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L2/gateway/L2LPTGateway.sol#L65-L89

    function outboundTransfer(
        address _l1Token,
        address _to,
        uint256 _amount,
        bytes calldata _data
    ) public override whenNotPaused returns (bytes memory res) {
        require(_l1Token == l1Lpt, "TOKEN_NOT_LPT");

        (address from, bytes memory extraData) = parseOutboundData(_data);
        require(extraData.length == 0, "CALL_HOOK_DATA_NOT_ALLOWED");

        Mintable(l2Lpt).burn(from, _amount);
        IL2LPTDataCache(l2LPTDataCache).decreaseL2SupplyFromL1(_amount);

        uint256 id = sendTxToL1(
            from,
            l1Counterpart,
            getOutboundCalldata(_l1Token, from, _to, _amount, extraData)
        );

        // we don't need to track exitNums (b/c we have no fast exits) so we always use 0
        emit WithdrawalInitiated(_l1Token, from, _to, id, 0, _amount);

        return abi.encode(id);
    }

res is unused.

Recommendation

Change to:

    function outboundTransfer(
        address _l1Token,
        address _to,
        uint256 _amount,
        bytes calldata _data
    ) public override whenNotPaused returns (bytes memory) {
        require(_l1Token == l1Lpt, "TOKEN_NOT_LPT");

        (address from, bytes memory extraData) = parseOutboundData(_data);
        require(extraData.length == 0, "CALL_HOOK_DATA_NOT_ALLOWED");

        Mintable(l2Lpt).burn(from, _amount);
        IL2LPTDataCache(l2LPTDataCache).decreaseL2SupplyFromL1(_amount);

        uint256 id = sendTxToL1(
            from,
            l1Counterpart,
            getOutboundCalldata(_l1Token, from, _to, _amount, extraData)
        );

        // we don't need to track exitNums (b/c we have no fast exits) so we always use 0
        emit WithdrawalInitiated(_l1Token, from, _to, id, 0, _amount);

        return abi.encode(id);
    }

Handle

WatchPug

Vulnerability details

For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.

For example:

https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L2/gateway/L2LPTDataCache.sol#L62-L66

        if (_amount > l2SupplyFromL1) {
            l2SupplyFromL1 = 0;
        } else {
            l2SupplyFromL1 -= _amount;
        }

l2SupplyFromL1 -= _amount will never underflow.

https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L1/gateway/L1LPTGateway.sol#L149-L156

        if (amount <= escrowBalance) {
            TokenLike(l1Token).transferFrom(l1LPTEscrow, to, amount);
        } else {
            if (escrowBalance > 0) {
                TokenLike(l1Token).transferFrom(l1LPTEscrow, to, escrowBalance);
            }
            IMinter(minter).bridgeMint(to, amount - escrowBalance);
        }

amount - escrowBalance will never underflow.

https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L2/pool/DelegatorPool.sol#L59-L78

        require(
            claimedInitialStake < initialStake,
            "DelegatorPool#claim: FULLY_CLAIMED"
        );

        // _stake is the delegator's original stake
        // This contract started off with initalStake
        // We can calculate how much of the contract's current stake and fees
        // are owed to the delegator proportional to _stake / (initialStake - claimedInitialStake)
        // where claimedInitialStake is the stake of the contract that has already been claimed

        // Calculate stake owed to delegator
        uint256 currTotalStake = pendingStake();
        uint256 owedStake = (currTotalStake * _stake) /
            (initialStake - claimedInitialStake);

        // Calculate fees owed to delegator
        uint256 currTotalFees = pendingFees();
        uint256 owedFees = (currTotalFees * _stake) /
            (initialStake - claimedInitialStake);

initialStake - claimedInitialStake will never underflow.

https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L2/gateway/L2LPTDataCache.sol#L91-L94

        return
            l1TotalSupply >= l2SupplyFromL1
                ? l1TotalSupply - l2SupplyFromL1
                : 0;

l1TotalSupply - l2SupplyFromL1 will never underflow.

Handle

WatchPug

Vulnerability details

https://github.com/livepeer/protocol/blob/20e7ebb86cdb4fe9285bf5fea02eb603e5d48805/contracts/token/BridgeMinter.sol#L90-L98

function withdrawETHToL1Migrator() external onlyL1Migrator returns (uint256) {
    uint256 balance = address(this).balance;

    // call() should be safe from re-entrancy here because the L1Migrator and l1MigratorAddr are trusted
    (bool ok, ) = l1MigratorAddr.call.value(address(this).balance)("");
    require(ok, "BridgeMinter#withdrawETHToL1Migrator: FAIL_CALL");

    return balance;
}

At L94, address(this).balance can be replaced with balance to avoid unnecessarily repeated read of account balance state to save some gas.

Handle

WatchPug

Vulnerability details

When there are multiple checks, adjusting the sequence to allow the tx to fail earlier can save some gas.

Checks using less gas should be executed earlier than those with higher gas costs, to avoid unnecessary storage read, arithmetic operations, etc when it reverts.

For example:

https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L2/gateway/L2Migrator.sol#L255-L275

        require(
            claimStakeEnabled,
            "L2Migrator#claimStake: CLAIM_STAKE_DISABLED"
        );

        IMerkleSnapshot merkleSnapshot = IMerkleSnapshot(merkleSnapshotAddr);

        address delegator = msg.sender;
        bytes32 leaf = keccak256(
            abi.encodePacked(delegator, _delegate, _stake, _fees)
        );

        require(
            merkleSnapshot.verify(keccak256("LIP-73"), _proof, leaf),
            "L2Migrator#claimStake: INVALID_PROOF"
        );

        require(
            !migratedDelegators[delegator],
            "L2Migrator#claimStake: ALREADY_MIGRATED"
        );

The check of !migratedDelegators[delegator] can be done earlier to avoid reading from storage when migratedDelegators[delegator] == true.

Recommendation

Change to:

        require(
            claimStakeEnabled,
            "L2Migrator#claimStake: CLAIM_STAKE_DISABLED"
        );

        address delegator = msg.sender;
        require(
            !migratedDelegators[delegator],
            "L2Migrator#claimStake: ALREADY_MIGRATED"
        );

        IMerkleSnapshot merkleSnapshot = IMerkleSnapshot(merkleSnapshotAddr);
        require(
            merkleSnapshot.verify(keccak256("LIP-73"), _proof, leaf),
            "L2Migrator#claimStake: INVALID_PROOF"
        );

        bytes32 leaf = keccak256(
            abi.encodePacked(delegator, _delegate, _stake, _fees)
        );

Handle

WatchPug

Vulnerability details

Booleans are more expensive than uint256 or any type that takes up a full word because each write operation emits an extra SLOAD to first read the slot's contents, replace the bits taken up by the boolean, and then write back. This is the compiler's defense against contract upgrades and pointer aliasing, and it cannot be disabled.

See: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/ReentrancyGuard.sol#L23-L27

https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L2/gateway/L2Migrator.sol#L51-L51

    bool public claimStakeEnabled;

https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L2/gateway/L2Migrator.sol#L53-L57

    mapping(address => bool) public migratedDelegators;
    // mapping(address => address) public delegatorPools;
    // mapping(address => uint256) public claimedDelegatedStake;
    mapping(address => mapping(uint256 => bool)) public migratedUnbondingLocks;
    mapping(address => bool) public migratedSenders;

Handle

WatchPug

Vulnerability details

https://github.com/livepeer/protocol/blob/20e7ebb86cdb4fe9285bf5fea02eb603e5d48805/contracts/Manager.sol#L48-L50

function _onlyController() internal view {
    require(msg.sender == address(controller), "caller must be Controller");
}

https://github.com/livepeer/protocol/blob/20e7ebb86cdb4fe9285bf5fea02eb603e5d48805/contracts/Manager.sol#L11-L14

modifier onlyController() {
    _onlyController();
    _;
}

_onlyController() is unnecessary as it's being used only once. Therefore it can be inlined in modifier onlyController() to make the code simpler and save gas.

Recommendation

Change to:

modifier onlyController() {
    require(msg.sender == address(controller), "caller must be Controller");
    _;
}

Handle

WatchPug

Vulnerability details

https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L1/gateway/L1LPTGateway.sol#L136-L159

function finalizeInboundTransfer(
    address l1Token,
    address from,
    address to,
    uint256 amount,
    bytes calldata data
) external override onlyL2Counterpart(l2Counterpart) {
    require(l1Token == l1Lpt, "TOKEN_NOT_LPT");
    (uint256 exitNum, ) = abi.decode(data, (uint256, bytes));

    uint256 escrowBalance = TokenLike(l1Token).balanceOf(l1LPTEscrow);

    // mint additional tokens if requested amount exceeds escrowed amount
    if (amount <= escrowBalance) {
        TokenLike(l1Token).transferFrom(l1LPTEscrow, to, amount);
    } else {
        if (escrowBalance > 0) {
            TokenLike(l1Token).transferFrom(l1LPTEscrow, to, escrowBalance);
        }
        IMinter(minter).bridgeMint(to, amount - escrowBalance);
    }

    emit WithdrawalFinalized(l1Token, from, to, exitNum, amount);
}

As L143 already assured that l1Token equals immutable variable l1Lpt, therefore l1Token can be replaced with immutable variable l1Lpt, to avoid local variable reads (MLOAD) to save some gas.

Handle

WatchPug

Vulnerability details

https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L2/token/LivepeerToken.sol#L12-L16

constructor() ERC20("Livepeer Token", "LPT") ERC20Permit("Livepeer Token") {
    _setupRole(DEFAULT_ADMIN_ROLE, _msgSender());
    _setRoleAdmin(MINTER_ROLE, DEFAULT_ADMIN_ROLE);
    _setRoleAdmin(BURNER_ROLE, DEFAULT_ADMIN_ROLE);
}

https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/ControlledGateway.sol#L18-L24

constructor(address _l1Lpt, address _l2Lpt) {
    _setupRole(DEFAULT_ADMIN_ROLE, _msgSender());
    _setRoleAdmin(GOVERNOR_ROLE, DEFAULT_ADMIN_ROLE);

    l1Lpt = _l1Lpt;
    l2Lpt = _l2Lpt;
}

constant DEFAULT_ADMIN_ROLE = 0x00

By default, the admin role for all roles is DEFAULT_ADMIN_ROLE.

Therefore, _setRoleAdmin(***_ROLE, DEFAULT_ADMIN_ROLE); is redundant.

Removing it will make the code simpler and save some gas.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/783ac759a902a7b4a218c2d026a77e6a26b6c42d/contracts/access/AccessControl.sol#L40-L43

 * By default, the admin role for all roles is `DEFAULT_ADMIN_ROLE`, which means
 * that only accounts with this role will be able to grant or revoke other
 * roles. More complex role relationships can be created by using
 * {_setRoleAdmin}.

https://docs.openzeppelin.com/contracts/3.x/access-control#granting-and-revoking

AccessControl includes a special role, called DEFAULT_ADMIN_ROLE, which acts as the default admin role for all roles. An account with this role will be able to manage any other role, unless _setRoleAdmin is used to select a new admin role.

Recommendation

Remove the redundant code.

Handle

WatchPug

Vulnerability details

There is no risk of overflow caused by increamenting the iteration index in for loops (the i++ in for for (uint256 i = 0; i < _unbondingLockIds.length; i++)).

Increments perform overflow checks that are not necessary in this case.

Recommendation

Surround the increment expressions with an unchecked { ... } block to avoid the default overflow checks. For example, change the for loop:

https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L1/gateway/L1Migrator.sol#L472-L479

for (uint256 i = 0; i < _unbondingLockIds.length; i++) {
    (uint256 amount, ) = bondingManager.getDelegatorUnbondingLock(
        _l1Addr,
        _unbondingLockIds[i]
    );

    total += amount;
}

to:

for (uint256 i = 0; i < _unbondingLockIds.length;) {
    (uint256 amount, ) = bondingManager.getDelegatorUnbondingLock(
        _l1Addr,
        _unbondingLockIds[i]
    );

    total += amount;

    unchecked { ++i; }
}