code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

function repayAavePortalFor() in PortalFacet is to allows anyone to repay the portal in the adopted asset for a given router and transfer but the code don't use the value of _router anywhere and in the end execution flow emit event AavePortalRouterRepayment() with wrong data for router address.

Proof of Concept

This is repayAavePortalFor() and _backLoan() code in PortalFacet:

  /**
   * @notice This allows anyone to repay the portal in the adopted asset for a given router
   * and transfer
   * @dev Should always be paying in the backing asset for the aave loan
   * @param _router Router who took out the credit
   * @param _adopted Address of the adopted asset (asset backing the loan)
   * @param _backingAmount Amount of principle to repay
   * @param _feeAmount Amount of fees to repay
   * @param _transferId Corresponding transfer id for the fees
   */
  function repayAavePortalFor(
    address _router,
    address _adopted,
    uint256 _backingAmount,
    uint256 _feeAmount,
    bytes32 _transferId
  ) external payable {
    address adopted = _adopted == address(0) ? address(s.wrapper) : _adopted;
    // Ensure the asset is whitelisted
    ConnextMessage.TokenId memory canonical = s.adoptedToCanonical[adopted];
    if (canonical.id == bytes32(0)) {
      revert PortalFacet__repayAavePortalFor_notSupportedAsset();
    }

    // Transfer funds to the contract
    uint256 total = _backingAmount + _feeAmount;
    if (total == 0) revert PortalFacet__repayAavePortalFor_zeroAmount();

    (, uint256 amount) = AssetLogic.handleIncomingAsset(_adopted, total, 0);

    // If this was a fee on transfer token, reduce the total
    if (amount < total) {
      uint256 missing = total - amount;
      if (missing < _feeAmount) {
        // Debit fee amount
        _feeAmount -= missing;
      } else {
        // Debit backing amount
        missing -= _feeAmount;
        _feeAmount = 0;
        _backingAmount -= missing;
      }
    }

    // No need to swap because this is the adopted asset. Simply
    // repay the loan
    _backLoan(adopted, _backingAmount, _feeAmount, _transferId);
  }

  // ============ Internal functions ============

  /**
   * @notice Calls backUnbacked on the aave contracts
   * @dev Assumes funds in adopted asset are already on contract
   * @param _asset Address of the adopted asset (asset backing the loan)
   * @param _backing Amount of principle to repay
   * @param _fee Amount of fees to repay
   * @param _transferId Corresponding transfer id for the fees
   */
  function _backLoan(
    address _asset,
    uint256 _backing,
    uint256 _fee,
    bytes32 _transferId
  ) internal {
    // reduce debt
    s.portalDebt[_transferId] -= _backing;
    s.portalFeeDebt[_transferId] -= _fee;

    // increase allowance
    SafeERC20Upgradeable.safeIncreaseAllowance(IERC20Upgradeable(_asset), s.aavePool, _backing + _fee);

    // back loan
    IAavePool(s.aavePool).backUnbacked(_asset, _backing, _fee);

    // emit event
    emit AavePortalRouterRepayment(msg.sender, _asset, _backing, _fee);
  }

As you can see _router is never used in any logic and _backLoan() is get called and in the end code run emit AavePortalRouterRepayment(msg.sender, _asset, _backing, _fee) which emits event with wrong data as uses msg.sender instead of _router.

Tools Used

VIM

Recommended Mitigation Steps

add _router value to _backLoan() parameters and update where _backLoan() is called and update emit AavePortalRouterRepayment()

jakekidd commented 2 years ago

Resolved - we now just use transferId instead of router to index debt and repayment

0xleastwood commented 2 years ago

I don't think this is a serious concern but it is definitely a worthwhile improvment. Emitting events with incorrect details does not pose any direct security risk. The only issue present is the lack of integrity checks on _transferId. There are no checks to ensure_router is the same router whom provided liquidity to the respective _transferId. Downgrading to QA.