QA Report

[code423n4]

1. Unpatched token approval issue in BridgeFacet, open TODOs (low)

Proof of Concept

Race condition issue isn't addressed, open TODO:

https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/facets/BridgeFacet.sol#L1027-L1029

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

Another TODO comment:

https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/facets/BridgeFacet.sol#L579-L581

      // TODO: do we need to keep this
      bytes32 details = action.detailsHash();
      IBridgeToken(token).setDetailsHash(details);

Recommended Mitigation Steps

Consider choosing and implementing the method to address the approval issue, i.e. either do two step 0 -> amount approval or try-catch, before release.

It's advised to remove TODO comments in either way from production version of the code.

2. Incomplete description of AssetLogic's _swapAsset function (low)

_swapAsset() function description omits _slippageTol argument:

https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/libraries/AssetLogic.sol#L252-L268

  /**
   * @notice Swaps assetIn t assetOut using the stored stable swap or internal swap pool
   * @dev Will not swap if the asset passed in is the adopted asset
   * @param _canonicalId - The canonical token id
   * @param _assetIn - The address of the from asset
   * @param _assetOut - The address of the to asset
   * @param _amount - The amount of the local asset to swap
   * @return The amount of assetOut
   * @return The address of assetOut
   */
  function _swapAsset(
    bytes32 _canonicalId,
    address _assetIn,
    address _assetOut,
    uint256 _amount,
    uint256 _slippageTol
  ) internal returns (uint256, address) {

Recommended Mitigation Steps

Add the description, for example:

  /**
   * @param _amount      - The amount of the local asset to swap
   * @param _slippageTol - Slippage tolerance with LIQUIDITY_FEE_DENOMINATOR decimals
   * @return The amount of assetOut
   * @return The address of assetOut
   */

3. Incomplete description of AssetLogic's _swapAssetOut function (low)

There is a typo and success return value is omitted:

https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/libraries/AssetLogic.sol#L294-L310

  /**
   * @notice Swaps assetIn t assetOut using the stored stable swap or internal swap pool
   * @dev Will not swap if the asset passed in is the adopted asset
   * @param _canonicalId - The canonical token id
   * @param _assetIn - The address of the from asset
   * @param _assetOut - The address of the to asset
   * @param _amountOut - The amount of the _assetOut to swap
   * @return The amount of assetIn
   * @return The address of assetOut
   */
  function _swapAssetOut(
    bytes32 _canonicalId,
    address _assetIn,
    address _assetOut,
    uint256 _amountOut,
    uint256 _maxIn
  )

Recommended Mitigation Steps

Consider updating to:

* @notice Swaps assetIn to assetOut using the stored stable swap or internal swap pool
...
* @return Swap success flag
* @return The amount of assetIn
...

4. AssetFacet's events aren't indexed (non-critical)

Filtering on unindexed events is disabled, which makes it harder to programmatically use and analyse the system.

Proof of Concept

AssetFacet's events don't have any indices:

https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/facets/AssetFacet.sol#L21-L62

  /**
   * @notice Emitted when the wrapper variable is updated
   * @param oldWrapper - The wrapper old value
   * @param newWrapper - The wrapper new value
   * @param caller - The account that called the function
   */
  event WrapperUpdated(address oldWrapper, address newWrapper, address caller);

  /**
   * @notice Emitted when the tokenRegistry variable is updated
   * @param oldTokenRegistry - The tokenRegistry old value
   * @param newTokenRegistry - The tokenRegistry new value
   * @param caller - The account that called the function
   */
  event TokenRegistryUpdated(address oldTokenRegistry, address newTokenRegistry, address caller);

  /**
   * @notice Emitted when a new stable-swap AMM is added for the local <> adopted token
   * @param canonicalId - The canonical identifier of the token the local <> adopted AMM is for
   * @param domain - The domain of the canonical token for the local <> adopted amm
   * @param swapPool - The address of the AMM
   * @param caller - The account that called the function
   */
  event StableSwapAdded(bytes32 canonicalId, uint32 domain, address swapPool, address caller);

  /**
   * @notice Emitted when a new asset is added
   * @param canonicalId - The canonical identifier of the token the local <> adopted AMM is for
   * @param domain - The domain of the canonical token for the local <> adopted amm
   * @param adoptedAsset - The address of the adopted (user-expected) asset
   * @param supportedAsset - The address of the whitelisted asset. If the native asset is to be whitelisted,
   * the address of the wrapped version will be stored
   * @param caller - The account that called the function
   */
  event AssetAdded(bytes32 canonicalId, uint32 domain, address adoptedAsset, address supportedAsset, address caller);

  /**
   * @notice Emitted when an asset is removed from whitelists
   * @param canonicalId - The canonical identifier of the token removed
   * @param caller - The account that called the function
   */
  event AssetRemoved(bytes32 canonicalId, address caller);

Recommended Mitigation Steps

Consider adding indexes to ids and addresses in the all important events to improve their usability

[jakekidd]

incomplete function descriptors are not low risk, should be considered QA only here

TODOs have been resolved/patched