Current implementation of arbitrary call execute failure handler may break some use case for example NFT bridge.

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/Executor.sol#L113-L192
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/Executor.sol#L194-L213

Vulnerability details

Impact

Current implementation of arbitrary call execute failure handler may break some use case for example NFT Bridge.

In the case of NFT Bridge, NFT may be lost forever.

This is likely to be happened in the case of out of gas.

Proof of Concept

Relayer receive the message to unlock BAYC on ETH chain. Relayer call execute on BridgeFacet which then call execute in Executor internally. Continue to these lines:

    // Ensure there is enough gas to handle failures
    uint256 gas = gasleft() - FAILURE_GAS;

    // Try to execute the callData
    // the low level call will return `false` if its execution reverts
    (success, returnData) = ExcessivelySafeCall.excessivelySafeCall(
      _args.to,
      gas,
      isNative ? _args.amount : 0,
      MAX_COPY,
      _args.callData
    );

    // Unset properties
    properties = LibCrossDomainProperty.EMPTY_BYTES;

    // Unset amount
    amnt = 0;

    // Handle failure cases
    if (!success) {
      _handleFailure(isNative, true, _args.assetId, payable(_args.to), payable(_args.recovery), _args.amount);
    }

Unfortunately, an enormous NFT project has just started minting their NFT when the relayer perform the execution. Causing gas price to increase by 20x.

As a result, gasLimit is 20x lesser than what we have calculated causing ExcessivelySafeCall.excessivelySafeCall to fail because of out of gas. We fall into _handleFailure function. Notice that NFT is not unlocked yet because target contract has failed to being executed.

  function _handleFailure(
    bool isNative,
    bool hasIncreased,
    address _assetId,
    address payable _to,
    address payable _recovery,
    uint256 _amount
  ) private {
    if (!isNative) {
      // Decrease allowance
      if (hasIncreased) {
        SafeERC20Upgradeable.safeDecreaseAllowance(IERC20Upgradeable(_assetId), _to, _amount);
      }
      // Transfer funds
      SafeERC20Upgradeable.safeTransfer(IERC20Upgradeable(_assetId), _recovery, _amount);
    } else {
      // Transfer funds
      AddressUpgradeable.sendValue(_recovery, _amount);
    }
  }

_handleFailure just sent dummy fund in the NFT bridge process to the useless fallback address (Useless in NFT bridge case as it doesn't involve any cross chain swapping / token transferring).

Finally, transferId will be marked as used (or reconciled). This transferId cannot be used anymore.

Recall that BAYC hasn't been unlocked yet as target contract has failed to being executed.

And we cannot reuse this transferId to retry anymore in the future as it is marked as used

As a result, BAYC is locked forever as target contract call never success anymore since transferId has been marked as used

Tools Used

Manual

Recommended Mitigation Steps

You should mix axelar style and connext style of error handling together.

Fund shouldn't be sent to fallback address immediately. Instead, leave an option for user to choose whether they want to recall the failed transaction manually or they want to transfer the fund to the fallback address.

[LayneHaber]

Acknowledge that this should be an issue, but think that the UX of having to submit multiple destination chain transactions is not great (and causes problems with fees / assuming the user has gas on the destination domain). I think this type of thing needs to be handled by the integrator themselves

[Chomtana]

I have reviewed this once again and found another case that Current implementation of arbitrary call execute failure handler may break some use case for example NFT bridge.

The idea for this to happened is simply do anything to set success to false. If NFT is failed to mint on the destination chain and revert (Likely due to bug in the contract), it cause the same consequence.

It would be better to revert and allow it to be executed again in the future by simply revert in case of arbitrary call failed. If the destination contract is upgradable, it can be fixed but if not it is out of luck. But current implementation is out of luck in all case.

Anyway, the sudden gas price increase problem can't be handled accurately as its may happened very fast. (5 gwei 2 minutes ago, 50 gwei when the NFT unlock as a big NFT project is minting). No one will spare that much gas while locking NFT in the source chain 2 minutes ago.

[0xleastwood]

Gas limit does not change deterministically. If you make a call which provides X amount of gas at Y price, if gas price changes, it will not affect any pending calls. The issue only arises if the relayer intentionally provides insufficient gas at any price.

[0xleastwood]

The recovery address is configured when the bridge transfer is initiated, if this or the executed calldata fails to be used correctly, I think the onus is on the sender. However, there are some legitimate concerns of a bridge relayer intentionally making the callback fail. Because funds aren't at direct risk, medium severity would make more sense.

[Chomtana]

The recovery address only recover ERC20 tokens managed by the Connext bridge system. Other custom made bridging solution that utilize the arbitrary message passing never utilize that recovery address passed.

Example

For example, if A lock "multiple BAYC NFT" on the Ethereum chain along with 1 WETH sent into Connext Amarok. On the destination chain, if executed calldata fails or gas limit is miscalculated (both intentionally and non-intentionally in case ether.js returns too low gas limit, it has happened many times on the complex contracts in 2021 not sure it is fixed or not), only 1 WETH is sent to the recovery address and not "multiple BAYC NFT". "Multiple BAYC NFT" will be locked on the Ethereum chain indefinitely without any chance to unlock or recovery it in the source chain. So, funds which are "multiple BAYC NFT" are at direct risk. BAYC floor price is currently more than 70 ETH. If 3 BAYC lost due to a single error, total 200+ ETH will be lost.

Here is an example case that might become common if NFT is not out of hyped but sadly the hype in NFT is currently down. It is "NFT buying frontrunning" problem.

Since Connext bridge requires 2-3 minutes between that times somebody may frontrun buying NFT without using bridge. Then the execution will be fail since NFT has been sold to buyer B. It only refund 10 ETH but not 3 NFT sent.

Mitigration

1. Simply revert in case of arbitrary call failed.
2. Provide a way for user to cancel the execution, get ERC20 refund into the recovery address and acknowledge contract in the source chain that the bridge is failed to claim back NFT.
3. Force all developers that are using your product to use Upgradeable contract on both source and destination chain to handle cases manually in case something went wrong (Not possible to force everyone).

Perfect mitigation is not found yet. There is an ongoing debate about this one at code-423n4/2022-07-axelar-findings#97

[0xleastwood]

Fully agree with your take on this. But I still stand by my decision on keeping this as a medium risk issue. Mainly because funds are not at direct risk and when they are at risk, certain assumptions must be made for this to hold true. Again, the onus seems to be on the implementer's end.

The important thing is to not integrate with Connext with the guarantee that calldata will be successfully executed on the destination contract. This should be clearly documented and I think allowing the bridge user to re-execute calldata upon failure is a potentially useful suggestion, but this doesn't come with its pitfalls too. Added complexity could expand Connext's attack surface.

[0xleastwood]

In response to your proposed mitigation(s):

1. This would impact the liveness of bridge transfers. We do not want to allow funds to get stuck due to a reliance on external requirements.
2. While this is a potential solution, I don't think Connext needs to make changes to accommodate this behaviour. The destination contract can check if the calldata was successfully executed and handle this on their end. Seaport already allows for custom behaviour.
3. Again, this would not require Connext to make any changes.