QA Report

[code423n4]

1) Description of AssetLogic._swapAsset lacks one input argument

AssetLogic._swapAsset() description has no mention of the _slippageTol argument:

  /**
   * @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

As an example:

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

2) Description of AssetLogic._swapAssetOut lacks one output argument

The success return value is omitted:

  /**
   * @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(

Recommended Mitigation Steps

As an example:

* @return Success value
* @return The amount of assetIn

3) Open TODOs in BridgeFacet

Open TODO comments that require some decisions to be made and then code completion:

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

Also:

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

Recommended Mitigation Steps

Consider removing all TODO comments with the corresponding code amendments (i.e. fix, won't fix, etc) before release as they are relevant to the development process only.

[liu-zhipeng]

1), 2) fixed

[jakekidd]

1. and 2) are actually invalid within the scope of this audit (there is no slippageTol argument. It seems that the auditor was somehow looking at the wrong branch.

Regardless, it was a good find, and seems to have helped us?? ^ lol, I'm okay with keeping this for that reason