Connext Amarok contest - cloudjunky's results

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/facets/PortalFacet.sol#L80

Vulnerability details

Impact

When repaying the AAVE Portal in repayAavePortal() the _local asset is used to repay the loan in _backLoan() rather than the adopted asset. This is likely to cause issues in production when actually repaying loans if the asset/token being repayed to AAVE is not the same as the asset/token that was borrowed.

Proof of Concept

The comment on L93 of PortalFacet.sol states;

// Need to swap into adopted asset or asset that was backing the loan
// The router will always be holding collateral in the local asset while the loaned asset
// is the adopted asset

The swap is executed on L98 in the call to AssetLogic.swapFromLocalAssetIfNeededForExactOut() however the return value adopted is never used (it's an unused local variable). The full function is shown below;

// Swap for exact `totalRepayAmount` of adopted asset to repay aave
(bool success, uint256 amountIn, address adopted) = AssetLogic.swapFromLocalAssetIfNeededForExactOut(
  _local,
  totalAmount,
  _maxIn
);

if (!success) revert PortalFacet__repayAavePortal_swapFailed();

// decrement router balances
unchecked {
  s.routerBalances[msg.sender][_local] -= amountIn;
}

// back loan
_backLoan(_local, _backingAmount, _feeAmount, _transferId);

The balance of the _local token is reduced but instead of the adopted token being passed to _backLoan() in L112 the _local token is used.

Tools Used

Vim

Recommended Mitigation Steps

To be consistent with the comments in the repayAavePortal() function adopted should be passed to _backLoan so that the loan is repayed in the appropriate token.

Remove the reference to _local in the _backLoan() function and replace it with adopted so it reads;

_backLoan(adopted, _backingAmount, _feeAmount, _transferId);

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.