Linea Canonical Token Bridge

Description

If the bridge token B of a native token A is already deployed and confirmDeployment is called on the other layer and setDeployed sets A’s nativeToBridgedToken value to DEPLOYED_STATUS. The bridge token B cannot bridge to native token A in completeBridging function, because A’s nativeToBridgedToken value is not NATIVE_STATUS, as a result the native token won’t be transferred to the receiver. User’s bridge token will be locked in the original layer

Examples

contracts/TokenBridge.sol:L217-L229

if (nativeMappingValue == NATIVE_STATUS) {
  // Token is native on the local chain
  IERC20(_nativeToken).safeTransfer(_recipient, _amount);
} else {
  bridgedToken = nativeMappingValue;
  if (nativeMappingValue == EMPTY) {
    // New token
    bridgedToken = deployBridgedToken(_nativeToken, _tokenMetadata);
    bridgedToNativeToken[bridgedToken] = _nativeToken;
    nativeToBridgedToken[_nativeToken] = bridgedToken;
  }
  BridgedToken(bridgedToken).mint(_recipient, _amount);
}

contracts/TokenBridge.sol:L272-L279

function setDeployed(address[] memory _nativeTokens) external onlyMessagingService fromRemoteTokenBridge {
  address nativeToken;
  for (uint256 i; i < _nativeTokens.length; i++) {
    nativeToken = _nativeTokens[i];
    nativeToBridgedToken[_nativeTokens[i]] = DEPLOYED_STATUS;
    emit TokenDeployed(_nativeTokens[i]);
  }
}

Recommendation

Add an condition nativeMappingValue = DEPLOYED_STATUS for native token transfer in confirmDeployment

if (nativeMappingValue == NATIVE_STATUS || nativeMappingValue == DEPLOYED_STATUS) {
   IERC20(_nativeToken).safeTransfer(_recipient, _amount);

Description

If the bridging failed due to the single coordinator is down, censoring the message, or bridge token contract is set to a bad or wrong contract address by setCustomContract, user’s funds will stuck in the TokenBridge contract until coordinator is online or stop censoring, there is no way to withdraw the deposited funds

Examples

contracts/TokenBridge.sol:L341-L348

function setCustomContract(
  address _nativeToken,
  address _targetContract
) external onlyOwner isNewToken(_nativeToken) {
  nativeToBridgedToken[_nativeToken] = _targetContract;
  bridgedToNativeToken[_targetContract] = _nativeToken;
  emit CustomContractSet(_nativeToken, _targetContract);
}

Recommendation

Add withdraw functionality to let user withdraw the funds under above circumstances or at least add withdraw functionality for Admin (admin can send the funds to the user manually), ultimately decentralize coordinator and sequencer to reduce bridging failure risk.

Description

Currently, the system design does not support the scenarios where native tokens with the same addresses (which is possible with the same deployer and nonce) on different layers can be bridged.

For instance, Let’s consider, there is a native token A on L1 which has already been bridged on L2. If anyone tries to bridge native token B on L2 with the same address as token A , instead of creating a new bridge on L1 and minting new tokens, the token bridge will transfer native token A on L1 to the _recipient which is incorrect.

The reason is the mappings don’t differentiate between the native tokens on two different Layers.

  mapping(address => address) public nativeToBridgedToken;
  mapping(address => address) public bridgedToNativeToken;

Examples

contracts/TokenBridge.sol:L208-L220

function completeBridging(
  address _nativeToken,
  uint256 _amount,
  address _recipient,
  bytes calldata _tokenMetadata
) external onlyMessagingService fromRemoteTokenBridge {
  address nativeMappingValue = nativeToBridgedToken[_nativeToken];
  address bridgedToken;

  if (nativeMappingValue == NATIVE_STATUS) {
    // Token is native on the local chain
    IERC20(_nativeToken).safeTransfer(_recipient, _amount);
  } else {

Recommendation

Redesign the approach to handle the same native tokens on different layers. One possible approach could be to define the set of mappings for each layer.

Description

In TokenBridge contract’s initialize function, there is no check for initializing parameters including _securityCouncil, _messageService, _tokenBeacon and _reservedTokens. If any of these address is set to 0 or other invalid value, TokenBridge would not work, user may lose funds.

Examples

contracts/TokenBridge.sol:L97-L111

function initialize(
  address _securityCouncil,
  address _messageService,
  address _tokenBeacon,
  address[] calldata _reservedTokens
) external initializer {
  __Pausable_init();
  __Ownable_init();
  setMessageService(_messageService);
  tokenBeacon = _tokenBeacon;
  for (uint256 i = 0; i < _reservedTokens.length; i++) {
    setReserved(_reservedTokens[i]);
  }
  _transferOwnership(_securityCouncil);
}

Recommendation

Add non-zero address check for _securityCouncil, _messageService, _tokenBeacon and _reservedTokens

Description

The function setCustomContract allows the owner to update arbitrary status for new native tokens without confirmation, bypassing the bridge protocol.

• It can set DEPLOYED_STATUS for a new native token, even if there exists no bridged token for it.
• It can set NATIVE_STATUS for a new native token even if it’s not.
• It can set RESERVED_STATUS disallowing any new native token to be bridged.

Examples

contracts/TokenBridge.sol:L341-L348

function setCustomContract(
  address _nativeToken,
  address _targetContract
) external onlyOwner isNewToken(_nativeToken) {
  nativeToBridgedToken[_nativeToken] = _targetContract;
  bridgedToNativeToken[_targetContract] = _nativeToken;
  emit CustomContractSet(_nativeToken, _targetContract);
}

Recommendation

The function should not allow _targetContract to be any state code

Description

The function setCustomContract allows the owner, to define a custom ERC20 contract for the native token. However, it doesn’t check whether the target contract has already been defined as a bridge to a native token or not. As a result, the owner may take advantage of the design flaw and bridge another new native token that has not been bridged yet, to an already existing target(already a bridge for another native token). Now, if a user tries to bridge this native token, the token bridge on the source chain will take the user’s tokens, and instead of deploying a new bridge on the destination chain, tokens will be minted to the _recipient on an existing bridge defined by the owner, or it can be any random EOA address to create a DoS.

The owner can also try to front-run calls to completeBridging for new Native Tokens on the destination chain, by setting a different bridge via setCustomContract. Although, the team states that the role will be controlled by a multi-sig which makes frontrunning less likely to happen.

Examples

contracts/TokenBridge.sol:L341-L348

function setCustomContract(
  address _nativeToken,
  address _targetContract
) external onlyOwner isNewToken(_nativeToken) {
  nativeToBridgedToken[_nativeToken] = _targetContract;
  bridgedToNativeToken[_targetContract] = _nativeToken;
  emit CustomContractSet(_nativeToken, _targetContract);
}

contracts/TokenBridge.sol:L220-L229

} else {
  bridgedToken = nativeMappingValue;
  if (nativeMappingValue == EMPTY) {
    // New token
    bridgedToken = deployBridgedToken(_nativeToken, _tokenMetadata);
    bridgedToNativeToken[bridgedToken] = _nativeToken;
    nativeToBridgedToken[_nativeToken] = bridgedToken;
  }
  BridgedToken(bridgedToken).mint(_recipient, _amount);
}

Recommendation

Make sure, a native token should bridge to a single target contract. A possible approach could be to check whether the bridgedToNativeToken for a target is EMPTY or not. If it’s not EMPTY, it means it’s already a bridge for a native token and the function should revert. The same can be achieved by adding the modifier isNewToken(_targetContract).

Note:- However, it doesn’t resolve the issue of frontrunning, even if the likelihood is less.

Description

In the second round of the audit, we discovered an edge case that may exist because of an address collision of native tokens. In the first round, we found issue 4.3 explaining how the bridges only support a single native token on both layers and may cause incorrect bridging. In response to that, the Linea team implemented a change that reverts whenever there is an attempt to bridge a native token with the same address on the other layer.

However, the issue still exists because of the inconsistent state of native tokens while bridging. The reason is, there could be an attempt to bridge a token with the same address on both layers at the same time, which could be done deliberately by an attacker by monitoring the bridging call at source layer and frontrunning them on the destination layer. As a consequence, both the tokens will get the NATIVE_STATUS on both layers, as the bridges can’t check the state of a token on the other layer while bridging. Now, the bridging that was initiated for a native token on the source layer will be completed with the native token on the destination layer, as the bridging was initiated at the same time.

    if (nativeMappingValue == NATIVE_STATUS || nativeMappingValue == DEPLOYED_STATUS) {
      // Token is native on the local chain
      IERC20Upgradeable(_nativeToken).safeTransfer(_recipient, _amount);

For the issue, the Linea team came back with the solution in the PR 1041 with the final commit a875e67e0681ce387825127a08f1f924991a274c. The solution implemented adds a flag _isNativeLayer while sending the message to Message Service on source layer

    messageService.sendMessage{ value: msg.value }(
      remoteSender,
      msg.value, // fees
      abi.encodeCall(ITokenBridge.completeBridging, (nativeToken, _amount, _recipient, _isNativeLayer, tokenMetadata))
    );

and is used to verify the state of native token on destination layer while calling completeBridging.

...
    if (_isNativeLayer == false && (nativeMappingValue == NATIVE_STATUS || nativeMappingValue == DEPLOYED_STATUS)) {
      // Token is native on the local chain
      IERC20Upgradeable(_nativeToken).safeTransfer(_recipient, _amount);
    }
    else{
    ...
          if (
        nativeMappingValue == EMPTY ||
        (_isNativeLayer && (nativeMappingValue == NATIVE_STATUS || nativeMappingValue == DEPLOYED_STATUS))
      ) {
              // New token
        bridgedToken = deployBridgedToken(_nativeToken, _tokenMetadata);
        bridgedToNativeToken[bridgedToken] = _nativeToken;
        nativeToBridgedToken[_nativeToken] = bridgedToken;
      }
      BridgedToken(bridgedToken).mint(_recipient, _amount);
      ...
    }
   

The logic adds two conditional checks: 1- If _isNativeLayer is false, it means the token is not native on the source layer, and if for the same address the status is either Native or Deployed, then the native token of the destination layer should be bridged. 2- If the flag is true, it means the token is native on the source layer, and if there exists a collision of the Native or Deployed status, then a new bridge token should be created.

Minting Bad Tokens We reviewed the PR and new integrations and found that it is still problematic. The reason is the bridging of a token with the same address, will create a bridgedToken on each layer. Now the nativeToBridgedToken status is no more Native or Deployed, but a bridge address. Let’s call the native token A and bridgedToken be B and C on each layer respectively. Now if the bridgeToken B is tried to be bridged back to native token A, it’ll be not possible, as while doing completeBridging both of the conditional checks will be unsatisfied. However, in any case, the bridge on the destination layer will still mint the bridgedTokens

      BridgedToken(bridgedToken).mint(_recipient, _amount);

As an example, the B tokens can be bridged to mint C bridgedTokens and vice-versa as and when needed. So, now it is mandatory to call confirmDeployment in order to allow condition 1 to be satisfied for this bridging and avoid this kind of bad minting.

Description

The function setMessageService allows the owner to update the message service address. However, it does not emit any event reflecting the change. As a result, in case the owner gets compromised, it can silently add a malicious message service, exploiting users’ funds. Since, there was no event emitted, off-chain monitoring tools wouldn’t be able to trigger alarms and users would continue using rogue message service until and unless tracked manually.

Examples

contracts/TokenBridge.sol:L237-L240

function setMessageService(address _messageService) public onlyOwner {
  messageService = IMessageService(_messageService);
}

Recommendation

Consider emitting an event reflecting the update from the old message service to the new one.

Description

Contracts should be deployed with the same compiler version they have been tested with. Locking the pragma helps ensure that contracts do not accidentally get deployed using, for example, the latest compiler which may have higher risks of undiscovered bugs. Contracts may also be deployed by others and the pragma indicates the compiler version intended by the original authors.

See Locking Pragmas in Ethereum Smart Contract Best Practices.

Examples

contracts/interfaces/IMessageService.sol:L2

pragma solidity ^0.8.19;

contracts/BridgedToken.sol:L2

pragma solidity ^0.8.19;

contracts/TokenBridge.sol:L2

pragma solidity ^0.8.19;

contracts/interfaces/ITokenBridge.sol:L2

pragma solidity ^0.8.19;

Recommendation

Lock the Solidity version to the latest version before deploying the contracts to production.

pragma solidity 0.8.19;

Description

1- Using Standard Interfaces and Libraries instead of Upgradeable ones.

contracts/TokenBridge.sol:L5-L10

import { IERC20Permit } from "@openzeppelin/contracts/token/ERC20/extensions/draft-IERC20Permit.sol";
import { IERC20Metadata } from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol";
import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import { OwnableUpgradeable } from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
import { PausableUpgradeable } from "@openzeppelin/contracts-upgradeable/security/PausableUpgradeable.sol";
import { SafeERC20 } from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";

We recommend using upgradeable interfaces and libraries to avoid any unexpected issues while contract upgrades, also increasing code readability.

2- Using Deprecated Files The contract imports file draft-IERC20Permit from the npm module openzeppelin/contracts.

contracts/TokenBridge.sol:L5

import { IERC20Permit } from "@openzeppelin/contracts/token/ERC20/extensions/draft-IERC20Permit.sol";

However, the file has been deprecated now, as the EIP2612 has been finalized. We recommend using the correct file which is extensions/IERC20Permit.sol or the corresponding upgradeable one.

Description

TokenBridge defines a privileged role Owner, however, it uses a single-step approach, which immediately transfers the ownership to the new address. If accidentally passed an incorrect address, the current owner will immediately lose control over the system as there is no fail-safe mechanism.

A safer approach would be to first propose the ownership to the new owner, and let the new owner accept the proposal to be the new owner. It will add a fail-safe mechanism for the current owner as in case it proposes ownership to an incorrect address, it will not immediately lose control, and may still propose again to a correct address.

Examples

contracts/TokenBridge.sol:L22

contract TokenBridge is ITokenBridge, PausableUpgradeable, OwnableUpgradeable {

Recommendation

Consider moving to a 2-step approach for the ownership transfers as recommended above. Note:- Openzeppelin provides another helper utility as Ownable2StepUpgradeable which follows the recommended approach

Description

1- Importing ITokenBridge twice The contract defines the import of interface ITokenBridge twice, out of which one can be removed.

contracts/TokenBridge.sol:L4

import { ITokenBridge } from "./interfaces/ITokenBridge.sol";

contracts/TokenBridge.sol:L14

import { ITokenBridge } from "./interfaces/ITokenBridge.sol";

2- Using bytes32 for a bytes4 selector The contract stores a 4-byte selector of _PERMIT_SELECTOR in a bytes32 type.

contracts/TokenBridge.sol:L24-L25

bytes32 private constant _PERMIT_SELECTOR =
  bytes4(keccak256(bytes("permit(address,address,uint256,uint256,uint8,bytes32,bytes32)")));

The selector is then compared with the first 4 bytes of _permitData to make sure, the calldata is indeed a calldata of permit function.

contracts/TokenBridge.sol:L435-L436

if (bytes4(_permitData[:4]) != _PERMIT_SELECTOR)
  revert InvalidPermitData(bytes4(_permitData[:4]), _PERMIT_SELECTOR);

The check will work as intended, however, if it fails, the error will revert with info containing 32 bytes of _PERMIT_SELECTOR as 0xd505accf00000000000000000000000000000000000000000000000000000000, which may create unnecessary confusion for end users. As a recommendation, consider using a bytes4 type for _PERMIT_SELECTOR and also for the custom error InvalidPermitData.

contracts/interfaces/ITokenBridge.sol:L20

error InvalidPermitData(bytes4 permitData, bytes32 permitSelector);