Lines of code

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/BridgeFacet.sol#L636

Vulnerability details

Vulnerability Details

Assume that a malicious relayer operates a router in Connext providing fast-liquidity service. A malicious relayer could always swap the router(s) within the execute calldata with the router(s) owned by malicious relayer, and submit it to the chain for execution.

Proof-of-Concept

This example assumes a fast-liquidity path. When the relayer posts a execute calldata to destination domain's BridgeFacet.execute function, this function will trigger the BridgeFacet._executeSanityChecks function to perform a sanity check.

Assume that the execute calldata only have 1 router selected. A malicious relayer could perform the following actions:

Attacker removes original router from _args.routers[] array, and remove original router's signature from _args.routerSignatures[] array from the execute calldata.
Attacker re-calculates the routerHash (routerHash = keccak256(abi.encode(transferId, pathLength))). pathLength = 1 in this example
Attacker signs the routerHash with his own router's private key to obtain the router signature
Attacker inserts his router to the _args.routers[] array, and insert the router signature obtained fromthe previous step to _args.routerSignatures[] array
Submit the modified execute calldata to destination domain's BridgeFacet.execute function, and it will pass the BridgeFacet._executeSanityChecks function since router signature is valid.

The BridgeFacet._executeSanityChecks function is not aware of the fact that the router within the execute calldata has been changed because it will only check if the router specified within the _args.routers[] array matches with the router signature provided. Once the sanity check passes, it will store the attacker's router within s.routedTransfers[_transferId] and proceed with providing fast-liquidity service for the users.

The existing mechanism is useful in preventing malicious relayer from specifying routers belonging to someone else because the malicious relayer would not be capable of generating a valid router signature on behalf of other routers because he does not own their private key. However, this mechanism does not guard against a malicious relayer from specifying their own router because in this case they would be able to generate a valid router signature as they own the private key.

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/BridgeFacet.sol#L636

 /**
   * @notice Performs some sanity checks for `execute`
   * @dev Need this to prevent stack too deep
   */
  function _executeSanityChecks(ExecuteArgs calldata _args) private view returns (bytes32, bool) {
    // If the sender is not approved relayer, revert
    if (!s.approvedRelayers[msg.sender] && msg.sender != _args.params.agent) {
      revert BridgeFacet__execute_unapprovedSender();
    }

    // Path length refers to the number of facilitating routers. A transfer is considered 'multipath'
    // if multiple routers provide liquidity (in even 'shares') for it.
    uint256 pathLength = _args.routers.length;

    // Make sure number of routers is below the configured maximum.
    if (pathLength > s.maxRoutersPerTransfer) revert BridgeFacet__execute_maxRoutersExceeded();

    // Derive transfer ID based on given arguments.
    bytes32 transferId = _getTransferId(_args);

    // Retrieve the reconciled record. If the transfer is `forceSlow` then it must be reconciled first
    // before it's executed.
    bool reconciled = s.reconciledTransfers[transferId];
    if (_args.params.forceSlow && !reconciled) revert BridgeFacet__execute_notReconciled();

    // Hash the payload for which each router should have produced a signature.
    // Each router should have signed the `transferId` (which implicitly signs call params,
    // amount, and tokenId) as well as the `pathLength`, or the number of routers with which
    // they are splitting liquidity provision.
    bytes32 routerHash = keccak256(abi.encode(transferId, pathLength));

    // check the reconciled status is correct
    // (i.e. if there are routers provided, the transfer must *not* be reconciled)
    if (pathLength > 0) // make sure routers are all approved if needed
    {
      if (reconciled) revert BridgeFacet__execute_alreadyReconciled();

      for (uint256 i; i < pathLength; ) {
        // Make sure the router is approved, if applicable.
        // If router ownership is renounced (_RouterOwnershipRenounced() is true), then the router whitelist
        // no longer applies and we can skip this approval step.
        if (!_isRouterOwnershipRenounced() && !s.routerPermissionInfo.approvedRouters[_args.routers[i]]) {
          revert BridgeFacet__execute_notSupportedRouter();
        }

        // Validate the signature. We'll recover the signer's address using the expected payload and basic ECDSA
        // signature scheme recovery. The address for each signature must match the router's address.
        if (_args.routers[i] != _recoverSignature(routerHash, _args.routerSignatures[i])) {
          revert BridgeFacet__execute_invalidRouterSignature();
        }

        unchecked {
          i++;
        }
      }
    ..SNIP..
  }

When the nomad message eventually reaches the destination domain and triggered to the BridgeFacet._reconcile, the attacker's router will be able to claim back the asset provided in the execution step as per normal.

Impact

Malicious relayer could force Connext to use those routers owned by them to earn the liquidity fee, and at the same time causes the original router chosen by the sequencer to lost the opportunity to earn the liquidity fee. This disrupts the balance and fairness of the protocol causing normal routers to lost the opportunity to earn liquidity fee.

In bridge or cross-chain communication design, it is a good security practice to minimize the trust that Connext places on other external protocol (e.g. relayer network) wherever possible so that if the external protocol is compromised or acting against maliciously against Connext, the impact or damage would be reduced.

Recommended Mitigation Steps

It is recommended to devise a way for the Connext's destination bridge to verify that the execute calldata received from the relayer is valid and has not been altered. Ideally, the hash of the original execute calldata sent by seqencer should be compared with the hash of the execute calldata received from relayer so that a mismatch would indicate that the calldata has been modified along the way, and some action should be taken.

For instance, consider a classic 0x off-chain ordering book protocol. A user will sign his order with his private key, and attach the signature to the order, and send the order (with signature) to the relayer network. If the relayer attempts to tamper the order message or signature, the decoded address will be different from the signer's address and this will be detected by 0x's Smart contract on-chain when processing the order. This ensures that the integrity of the message and signer can be enforced.

Per good security practice, relayer network should always be considered as a hostile environment/network. Therefore, it is recommended that similar approach could be taken with regards to passing execute calldata across domains/chains.

For instance, at a high level, the sequencer should sign the execute calldata with its private key, and attach the signature to the execute calldata. Then, submit the execute calldata (with signature) to the relayer network. When the bridge receives the execute calldata (with signature), it can verify if the decoded address matches the sequencer address to ensure that the calldata has not been altered. This will ensure the intergrity of the execute calldata and prevent any issue that arise due to unauthorised modification of calldata.

Alternative Solution

Alternatively, following method could also be adopted to prevent this issue:

Assume that it is possible to embed the selected router(s) within the slow nomad message. Append the selected router(s) within the slow nomad message
Within the BridgeFacet.execute function, instead of using only the transfer ID as the array index (s.routedTransfers[_transferId] = _args.routers; ), use both transfer ID + selected router as the array index (s.routedTransfers[hash(_transferId+routers)] = _args.routers;)
When the slow nomad message arrives and triggers to the BridgeFacet._reconcile, this function will find the routers that provide the fast-liquidity based on the information within the nomad message only. It will attempt to call s.routedTransfers[hash(_transferId+routers)] and it should return nothing as there is a mismatch between attacker's router and router within the nomad message.
In this case, the attacker will not be able to claim back any of the funds he provided earlier. This will deter anyone from attempting to swap the router within the execute calldata sent to destination domain's BridgeFacet.execute function because they will not be able to claim back the funds

code-423n4 / 2022-06-connext-findings

Malicious Relayers Could Favor Their Routers #147