routerBalances[msg.sender][_local] can be inflated in repayAavePortal due to underflow in unchecked math

[code423n4]

Lines of code

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

Vulnerability details

  function repayAavePortal(
    address _local,
    uint256 _backingAmount,
    uint256 _feeAmount,
    uint256 _maxIn,
    bytes32 _transferId
  ) external {
    uint256 totalAmount = _backingAmount + _feeAmount; // in adopted
    uint256 routerBalance = s.routerBalances[msg.sender][_local]; // in local

    // Sanity check: has that much to spend
    if (routerBalance < _maxIn) revert PortalFacet__repayAavePortal_insufficientFunds();

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

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

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

  function swapFromLocalAssetIfNeededForExactOut(
    address _asset,
    uint256 _amount,
    uint256 _maxIn
  )
    internal
    returns (
      bool,
      uint256,
      address
    )
  {
    AppStorage storage s = LibConnextStorage.connextStorage();

    // Get the token id
    (, bytes32 id) = s.tokenRegistry.getTokenId(_asset);

    // If the adopted asset is the local asset, no need to swap
    address adopted = s.canonicalToAdopted[id];
    if (adopted == _asset) {
      return (true, _amount, _asset);
    }

    return _swapAssetOut(id, _asset, adopted, _amount, _maxIn);
  }

When the _local is canonicalToAdopted, AssetLogic.swapFromLocalAssetIfNeededForExactOut() will return the totalAmount as amountIn, and totalAmount is the total of two input values: _backingAmount + _feeAmount.

At L91, it checks for routerBalance < _maxIn, but there is no guarantee that _maxIn >= amountIn.

As a result, when repayAavePortal() is called with _backingAmount + _feeAmount > routerBalance and _local is an adopted asset, L108 will underflow, and the routerBalance can be inflated to a very high number.

https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/facets/RoutersFacet.sol#L591-L593

  // Sanity check: amount can be deducted for the router
  if (routerBalance < _amount) revert RoutersFacet__removeRouterLiquidity_insufficientFunds();

The routerBalance is used for sanity check and once it's been inflated, the router can withdraw possibly all the funds in the contract.

Recommendation

Generally, we suggest not to use unchecked in critical storage updates

    if (!success) revert PortalFacet__repayAavePortal_swapFailed();

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

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

[ecmendenhall]

Duplicate of #68

[LayneHaber]

Closing in favor of #68