Connext Amarok contest - Chom's results

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/BridgeFacet.sol#L856-L877 https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/Executor.sol#L142-L144 https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/Executor.sol#L160-L166 https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/Executor.sol#L194-L213

Vulnerability details

Impact

_handleExecuteTransaction may not working correctly on fee-on-transfer tokens. As duplicated fee is applied to fee on transfer token when executing a arbitrary call message passing request. Moreover, the Executor contract increase allowance on that token for that target contract in full amount without any fee, this may open a vulnerability to steal dust fund in the contract

Moreover, failure is trying to send full amount without any fee which is not possible because fee is already applied one time for example 100 Safemoon -> 90 Safemoon but trying to transfer 100 safemoon to _recovery address. Obviously not possible since we only have 90 Safemoon. This will revert and always revert causing loss of fund in all case of fee-on-transfer tokens.

Proof of Concept

A message to transfer 100 Safemoon with some contract call payload has been submitted by a relayer to _handleExecuteTransaction function on BridgeFacet these lines hit.

      // 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
        )
      );

Noticed that 100 Safemoon is transferred to executor before contract execution. Now executor has 90 Safemoon due to 10% fee on transfer.

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

Next, we increase allowance of target contract to transfer 100 more Safemoon.

    // 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
    );

After that, it call target contract

    // 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
    );

Target contract tried to pull 100 Safemoon from Executor but now Executor only has 90 Safemoon, so contract call failed. Moving on to the failure handle.

  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);
    }
  }

isNative = false because of Safemoon

Trying to transfer 100 Safemoon to _recovery while only having 90 Safemoon in the contract. Thus failure handle is reverted.

Tools Used

Manual

Recommended Mitigation Steps

You should approve one step only. Avoid an extra token transfer.

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.

safeApprove, safeIncreaseAllowance and safeDecreaseAllowance will be failed if allowance is not zero

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/Executor.sol#L143 https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/PortalFacet.sol#L185 https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/libraries/AssetLogic.sol#L288 https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/BridgeFacet.sol#L1029 https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/Executor.sol#L205 https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/BridgeFacet.sol#L1043 https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/libraries/AssetLogic.sol#L347

Vulnerability details

Impact

safeApprove, safeIncreaseAllowance and safeDecreaseAllowance will be failed if allowance is not zero.

This lead to broken logic in swapping (and other actions) these special tokens such as USDT.

Proof of Concept

According to SafeERC20 implementation https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol

    function safeApprove(
        IERC20 token,
        address spender,
        uint256 value
    ) internal {
        // safeApprove should only be called when setting an initial allowance,
        // or when resetting it to zero. To increase and decrease it, use
        // 'safeIncreaseAllowance' and 'safeDecreaseAllowance'
        require(
            (value == 0) || (token.allowance(address(this), spender) == 0),
            "SafeERC20: approve from non-zero to non-zero allowance"
        );
        _callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, spender, value));
    }

    function safeIncreaseAllowance(
        IERC20 token,
        address spender,
        uint256 value
    ) internal {
        uint256 newAllowance = token.allowance(address(this), spender) + value;
        _callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, spender, newAllowance));
    }

    function safeDecreaseAllowance(
        IERC20 token,
        address spender,
        uint256 value
    ) internal {
        unchecked {
            uint256 oldAllowance = token.allowance(address(this), spender);
            require(oldAllowance >= value, "SafeERC20: decreased allowance below zero");
            uint256 newAllowance = oldAllowance - value;
            _callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, spender, newAllowance));
        }
    }

You will see that both safeIncreaseAllowance and safeDecreaseAllowance still use token.approve.selector as same as safeApprove. Except that safeApprove require approval to be zero.

But sadly, USDT also require approval to be zero in all case when calling token.approve.selector as safeIncreaseAllowance and safeDecreaseAllowance are also calling token.approve.selector. They are required to have zero allowance before call.

    function approve(address _spender, uint _value) public onlyPayloadSize(2 * 32) {

        // To change the approve amount you first have to reduce the addresses`
        //  allowance to zero by calling `approve(_spender, 0)` if it is not
        //  already 0 to mitigate the race condition described here:
        //  https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729
        require(!((_value != 0) && (allowed[msg.sender][_spender] != 0)));

        allowed[msg.sender][_spender] = _value;
        Approval(msg.sender, _spender, _value);
    }

From your note

    // Edge case with some tokens: Example USDT in ETH Mainnet, after the backUnbacked call there could be a remaining allowance if not the whole amount is pulled by aave.
    // Later, if we try to increase the allowance it will fail. USDT demands if allowance is not 0, it has to be set to 0 first.
    // TODO: Should we call approve(0) and approve(totalRepayAmount) instead? or with a try catch to not affect gas on all cases?
    // Example: https://github.com/aave/aave-v3-periphery/blob/ca184e5278bcbc10d28c3dbbc604041d7cfac50b/contracts/adapters/paraswap/ParaSwapRepayAdapter.sol#L138-L140
    SafeERC20.safeIncreaseAllowance(IERC20(adopted), s.aavePool, totalRepayAmount);

This mean after backUnbacked call there may be dust token (non-zero allowance). If this case happened, subsequence swap will be failed as safeIncreaseAllowance is always revert due to non-zero allowance.

Tools Used

Manual

Recommended Mitigation Steps

You should call approve(0) and approve(totalRepayAmount) instead? or with a try catch to not affect gas on all cases

    // Better version of approve to prevent revert
    function safeApproveNew(
        IERC20 token,
        address spender,
        uint256 value
    ) internal {
        try token.approve(spender, value) returns (bool) {} catch {
            _callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, spender, 0));
            _callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, spender, value));
        }
    }

You can safely use this function in your contract to approve token seamlessly without extra gas cost.