QA Report

[code423n4]

Require with empty error messages

Issue

Multicall.solL18 has a require statement with an empty message - providing no reason as to why it might revert;

function aggregate(Call[] memory calls) public returns (uint256 blockNumber, bytes[] memory returnData) {
    blockNumber = block.number;
    returnData = new bytes[](calls.length);
    for (uint256 i = 0; i < calls.length; i++) {
      (bool success, bytes memory ret) = calls[i].target.call(calls[i].callData);
      require(success);
      returnData[i] = ret;
    }
  }

Remediation

Ensure that each require statement has a clear message for why it failed.

Incomplete natspec documentation or missing params

Issue

The following natspec comments have missing params or incomplete documentation;

• The XCalled() event in BridgeFacet.sol has no parameter documentation despite the very next event Reconciled() having complete param documentation.
• The RouterLiquidityAdded() event in RoutersFacet.sol is missing param documentation for canonicalId in the natspec.
• The swapExact() function in StableSwapFacet.sol is missing param documentation for minAmountOut and deadline in natspec.
• The swapExactOut() function in StableSwapFacet.sol is missing param documentation for maxAmountIn and deadline in natspec.
• The setupAsset() function in AssetFacet.sol is missing param documentation for _stableSwapPool in natspec.
• The repayAavePortal() function in PortalFacet.sol is missing param documentation for _transferId in natspec.
• The natspec in the IExecutor{} interface of IExecutor.sol is confusing as to whether it is intended for the ExecutorArgs struct or the Executed event. Furthermore the params names don't match. The natspec uses an underscore where the struct and the event do not. In addition transferId and recovery are missing from the natspec documentation.
• The swapFromLocalAssetIfNeededForExactOut() function in AssetLogic.sol is missing param documentation for _maxIn in natspec.
• The _swapAsset() function in AssetLogic.sol is missing param documentation for _slippageTol in natspec.
• The _swapAssetOut() function in AssetLogic.sol is missing param documentation for _maxIn in natspec.
• The ExecuteArgs struct in LibConnextStorage.sol is missing param documentation for routerSignatures in natspec.
• The RouterPermissionsManagerInfo struct in LibConnextStorage.sol is missing param documentation for approvedForPortalRouters in natspec.

Remediation

Complete natspec documentation for functions and events identified above. I tried not to be pedantic here and focused only on functions that had some form of natspec param documentation assuming they were important and more accurate documentation would help.

Unused local variables

Issue

• The nonce value passed to the handle() function in BridgeFacet.sol is never used.
• The _router value passed to the _reconcileProcessPortal() function in BridgeFacet.sol is never used.
• The adopted value returned from AssetLogic.swapFromLocalAssetIfNeededForExactOut() in PortalFacet.sol is never used.
• The _router value passed to repayAavePortalFor() in PortalFacet.sol is never used.

Remediation

1. The nonce or _nonce value is is passed to numerous handle() functions throughout the code base sufficing the IMessageRecipient{} interface. However there seems to be no requirement for it in BridgeFacet.sol and it should be documented as unused similar to the Nomad GovernanceRouter.sol handle() function i.e. uint32, // nonce (unused).
2. _reconcileProcessPortal() is called within the _reconcile() function in BridgeFacet.sol. The first router in the path is passed to the function _reconcileProcessPortal(amount, token, routers[0], transferId);. If reconciling portal payments is not related to the router that took on the credit risk then remove routers[0] from the _reconcile() function and from the _reconcileProcessPortal() function definition.
3. The comment suggests this is more of an issue than just an unused local variable (I'll raise this as a medium bug). However for this Q&A report suffice to say that either the variable should be ommitted with (bool success, uint256 amountIn, _) = AssetLogic.swapFromLocalAssetIfNeededForExactOut(.. or the medium bug is true and adopted should be used instead of _local for the rest of the function.
4. _router should either be used in the function body or removed.

[jakekidd]

1 is invalid, multicall not in scope