search

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

1 stars 0 forks source link

Destination Contract Will Not Receive Their Tokens #217

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Background

BridgeFacet.xcall function can be used to perform a cross-chain transfer. During the transfer, user can also specify a callData that is execute on the receiving chain

Proof-of-Concept

To execute a transfer on the destination domain, relayers will call the BridgeFacet.execute, which will in turn trigger the BridgeFacet._handleExecuteTransaction function.

Within the BridgeFacet._handleExecuteTransaction function, if the transfer is initiated with a callData, it will proceed to call the s.executor.execute function.

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

  function _handleExecuteTransaction(
    ExecuteArgs calldata _args,
    uint256 _amount,
    address _asset, // adopted (or local if specified)
    bytes32 _transferId,
    bool _reconciled
  ) private returns (uint256) {
    ..SNIP..
    // execute the the transaction
    if (keccak256(_args.params.callData) == EMPTY) {
      // no call data, send funds to the user
      AssetLogic.transferAssetFromContract(_asset, _args.params.to, _amount);
    } else {
      // execute calldata w/funds
      AssetLogic.transferAssetFromContract(_asset, address(s.executor), _amount);
      (bool success, bytes memory returnData) = s.executor.execute(
        IExecutor.ExecutorArgs(
          _transferId,
          _amount,
          _args.params.to,
          _args.params.recovery,
          _asset,
          _reconciled
            ? LibCrossDomainProperty.formatDomainAndSenderBytes(_args.params.originDomain, _args.originSender)
            : LibCrossDomainProperty.EMPTY_BYTES,
          _args.params.callData
        )
      );
      ..SNIP..

The s.executor.execute code references to the following Executor.execute. This function is responsible for the arbitrary call data on a given address and transfer the tokens to the recipient.

However, it was observed that in Line 143, instead of transfering the tokens directly to the destination address (_args.to), it choses to increase the allowance of destination address and expects the destination contract to perform a ERC20.transferFrom operation to retrieve their tokens from bridge.

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

   /**
   * @notice Executes some arbitrary call data on a given address. The
   * call data executes can be payable, and will have `amount` sent
   * along with the function (or approved to the contract). If the
   * call fails, rather than reverting, funds are sent directly to
   * some provided fallback address
   * @param _args ExecutorArgs to function.
   */
 function execute(ExecutorArgs memory _args) external payable override onlyConnext returns (bool, bytes memory) {
    // Check if the callTo is a contract
    bool success;
    bytes memory returnData;

    bool isNative = _args.assetId == address(0);

    if (!AddressUpgradeable.isContract(_args.to)) {
      _handleFailure(isNative, false, _args.assetId, payable(_args.to), payable(_args.recovery), _args.amount);
      ..SNIP..
      return (success, returnData);
    }

    // If it is not ether, approve the callTo
    // We approve here rather than transfer since many external contracts
    // simply require an approval, and it is unclear if they can handle
    // funds transferred directly to them (i.e. Uniswap)

    if (!isNative) {
      SafeERC20Upgradeable.safeIncreaseAllowance(IERC20Upgradeable(_args.assetId), _args.to, _args.amount);
    }
    ..SNIP..

Impact

User tokens will be stuck in Connext bridge.

Recommended Mitigation Steps

Based on the comments, it was understood that the rational for such an approach is that they are unclear if the destination contract could handle the funds transferred directly to them.

Although it is a good intention to prevent the tokens from being stuck in the destination/user contract, this is an unlikely scenario. This approach will cause more harm than benefits. It is the responsibility of the users to know that their destination contract can process the received tokens. If the tokens get stuck in the destination contract, the responsibility lies with the users, not the protocol.

The current approach is also not aligned with the default ERC20 or Ether transfer pattern. By default, all tokens or values transfer will be sent directly to the destination address, and this pattern should be followed as users will expect Connext's transfer to behave the same.

Protocol should not make assumption on the destination contract, and it should simply deliver the tokens to the destination. The assumption might be right on some contracts, but wrong in the rest of the contracts, therefore no assumption should be made at all.

Within the blockchain, smart contracts are always considered as immutable, therefore, majority of the contracts are built not to be upgradable. When a user transfers tokens to a destination contract, they will expect the tokens to be sent directly to the destination contract instead of having to manually retrieve them from Connext bridge. As a result, majority of the existing contracts will not have features to retrieve their tokens from Connext bridge and are immutable, thus their tokens will end up stuck in Connext bridge. If this incident happens, the responsibilty would gravitate towards Connext since it has made an assumption on behalf of the users beforehand.

Additionally, this also means that with the current approach, any existing contract that is not immutable will not be able to use Connext bridge for token transfer (with callData) because they have no way to include new logic to retrieve token from Connext bridge, and they also do not know what will be the Connext bridge address at this point of time since it has not been deployed to the production yet (e.g. Ethereum mainnet)

In summary, update the execute function to simply transfer the tokens to the destination contract/address

 function execute(ExecutorArgs memory _args) external payable override onlyConnext returns (bool, bytes memory) {
    ..SNIP..
    if (!isNative) {
-       SafeERC20Upgradeable.safeIncreaseAllowance(IERC20Upgradeable(_args.assetId), _args.to, _args.amount);
+       SafeERC20Upgradeable.safeTransfer(IERC20Upgradeable(_args.assetId), _args.to, _args.amount);
    }
    ..SNIP..
 }
LayneHaber commented 2 years ago

Hm, I understand the logic here and appreciate it being laid out with such thought and detail.

I am not sure that I agree, however. One of the canonical examples (currently being used on our initial version of nxtp by lifi) is to use the calldata to execute a swap to allow any to any crosschain transfers by calling an AMM on the destination chain. For this use case, you would not be able to simply transfer to the destination contract as the AMM contracts generally need a transferFrom call (same thing applies to many DeFi protocols which require approvals).

Either way, you are making an assumption that the contract can handle funds sent directly to it, or it needs an approval. From our experience, an approval is easier to manage. In the case of an approval, you can always write a thin wrapper contract to accept the funds, then transfer them directly to another contract before executing some calldata. In contrast, if you send funds directly to the contract and it has to approve a different one, you must put all of that logic into a fallback function.

0xleastwood commented 2 years ago

I actually agree with the sponsor here, either way the contract needs to be able to handle the funds and approving the contract seems to provide more flexibility about how those funds might be utilised. Marking as invalid because the sponsor's design decision makes more sense.