Relayer Will Not Receive Any Fee If execute Reverts

[code423n4]

Lines of code

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

Vulnerability details

Proof-of-Concept

Connext relies on the relayer to trigger the BridgeFacet.execute function on the destination domain to initiate the token transfer and calldata execution processes. Relayers pay for the gas cost to trigger the execute function, and in return for their effort, they are reimbused with the relayer fee.

However, it is possible that the BridgeFacet.execute function will revert under certain circumstances when triggered by the relayers. For instance, when the BridgeFacet.execute function is triggered, it will call the BridgeFacet._handleExecuteLiquidity function. Within the BridgeFacet._handleExecuteLiquidity function, it will attempt to perform token swap using a StablePool. If the slippage during the swap exceeded the user-defined value, the swap will revert and subseqently the execute will revert too.

When the BridgeFacet.execute reverts, the relayers will not receive any relayer fee.

The following code shows that the relayer who can claim the relayer fee is set within the BridgeFacet.execute function at Line 415. Therefore, if this function reverts, relayer will not be able to claim the fee.

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

  function execute(ExecuteArgs calldata _args) external whenNotPaused nonReentrant returns (bytes32) {
    (bytes32 transferId, bool reconciled) = _executeSanityChecks(_args);

    // Set the relayer for this transaction to allow for future claim
    s.transferRelayer[transferId] = msg.sender;

    // execute router liquidity when this is a fast transfer
    // asset will be adopted unless specified to be local in params
    (uint256 amount, address asset) = _handleExecuteLiquidity(transferId, !reconciled, _args);

    // execute the transaction
    uint256 amountWithSponsors = _handleExecuteTransaction(_args, amount, asset, transferId, reconciled);

    // emit event
    emit Executed(transferId, _args.params.to, _args, asset, amountWithSponsors, msg.sender);

    return transferId;
  }

Impact

Lost of fund for the relayers as they pay for the gas cost to trigger the functions, but did not receive any relayer fee in return.

Recommended Mitigation Steps

Update the implementation of the BridgeFacet.execute so that it will fail gracefully and not revert when the swap fails or other functions fails. Relayers should be entitled to relayer fee regardless of the outcome of the BridgeFacet.execute call for their effort.

[jakekidd]

It's quite possible that the slippage changes while the relayer's tx is in the mempool - which I think is the valid concern here. A relayer can't know for a fact that another existing tx won't change the current slippage (assuming there are multiple approved relayers).

Relayers should be entitled to relayer fee regardless of the outcome of the BridgeFacet.execute call for their effort.

Worth noting that the mitigation step here ^ is tricky -- it can incentivize relayers to submit transactions that fail quickly to maximize profit. For example, if we pay relayers even in the event of failure when slippage is too high, they are incentivized to submit txs when the slippage is too high (because they will fail more cheaply, as opposed to fully executing). Relayers could potentially profit by pushing the slippage over a large user transfer's limit via the stableswap themselves.

I'm uncertain of whether I should acknowledge or dispute this issue because, while it is valid that relayers will lose funds in the case that the slippage changes while their tx is in the mempool, this may just be considered a core design property and a risk that relayers must factor into their executions.

(Leaving as acknowledged for now.)

[LayneHaber]

While not paying out for every relayed transaction (i.e. forcing relayers to incur/budget for some loss if the transaction fails) is a valid concern, and makes the system less appealing to relay for, I think it is the nature of relay networks to deal with these kinds of problems.

For example, even without the slippage, imagine there are two networks competing to relay for the same transaction. In this case, execution would fail for the second relayer, and there would be no fees remaining to pay them for their efforts. If you want to continue adding fees for the same transaction, you would be passing this failure cost onto the user (with a more stringent liveness condition).

This is a valid issue, but the fixes would introduce more complexity and edge cases than make sense to handle at this level.

[0xleastwood]

I'd say this is part of the risk of being a relayer but definitely worth noting so keeping it as is.