Malicious Relayer Could Cause A Router To Provide More Liquidity Than It Should

[code423n4]

Lines of code

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

Vulnerability details

Proof-of-Concept

Assume this is a fast-transfer path and the sequencer has a good reason (e.g. some sophisticated liquidity load balancing algorithm) to assign 3 routers to provide liquidity for a transfer of 90 DAI

Therefore, each of them will provide 30 DAI equally.

_args.routers[] array = [Router A, Router B, Router C]

However, a malicious relayer could rearrange the _args.routers[] array to any of the following and still pass the sanity checks within BridgeFacet._executeSanityChecks

_args.routers[] array = [Router A, Router A, Router A]

_args.routers[] array = [Router A, Router A, Router B]

_args.routers[] array = [Router A, Router A, Router C]

_args.routers[] array = [Router C, Router A, Router C]

The point is that as long as the attacker ensures that the pathLength is correct, he will be able to pass the router check within BridgeFacet._executeSanityChecks.

Assume that malicious relayer decided to rearrange the _args.routers[] array to as follows, this will cause Router A to provide more liquidity than it should be and overwrite the sequencer decision. In this case, Router A will be forced to provide 90 DAI.

_args.routers[] array = [Router A, Router A, Router A]

This is possible because the routerHash used to generate the router signature only consists of two items (transferId and pathLength)

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

  function _executeSanityChecks(ExecuteArgs calldata _args) private view returns (bytes32, bool) {
    ..SNIP..
    // 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..
  }

Impact

Malicious relayer could overwrite sequencer decision, or cause a certain router to drain more liquidity than it should be.

Recommended Mitigation Steps

Generate the routerHash with the following items should help to prevent this attack:

• transferId

• pathLength

• _args.routers[] array

In this case, if the attacker attempts to re-arrange the _args.routers[] array, the routerHash generate on the bridge will be different. Thus, it will fail the router signature verification within _executeSanityChecks function and it will revert.

[jakekidd]

The relayer doing this would actually not be an attack on the router providing liquidity; it would actually benefit that router, giving it the full bulk of the transfer - assuming the router can afford it - as that router will claims fees for the whole thing. However, it would grief other routers from getting access to this transfer in the process. This isn't great, but keep in mind relayers are currently a permissioned role (whitelisted) because the trust vectors there have not been abstracted entirely (among other reasons). As such, I am suggesting we de-escalate this issue to QA/Low Risk.

Additionally, the suggested mitigation step is invalid within current design - routerHash for the router's signature is generated prior to knowing which routers will be selected by sequencer. The only way to prevent this would be to check to make sure there are no duplicates in the routers array manually (which would incur not-so-great gas costs). Acknowledging the issue as we may want to implement that fix in the future.

EDIT: Removed disagree with severity tag. Seems appropriate in hindsight as it would be subverting the functionality of the protocol.

[0xleastwood]

I agree with the validity of this finding. Keeping it as is.