Loan repayments may revert

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/libraries/AssetLogic.sol#L347
https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/libraries/AssetLogic.sol#L249
https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/libraries/AssetLogic.sol#L304
https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/facets/PortalFacet.sol#L98
https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/facets/BridgeFacet.sol#L1011

Vulnerability details

Impact

An unexpected revert could cause failures on the part of bridges and portals to repay loans.

Proof of Concept

OpenZeppelin's safeApprove() function will revert if the account has already been approved and subsequent calls pass a non-zero value.

safeApprove() is used in one instance in the codebase in the AssetLogic.sol contract:

2022-06-connext/contracts/contracts/core/connext/libraries/AssetLogic.sol:347:        SafeERC20.safeApprove(IERC20(_assetIn), address(pool),     _amountIn);

It is called within a private function _swapAssetOut() which is used by the public function swapFromLocalAssetIfNeededForExactOut().

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

This public function is called from the contracts PortalFacet.sol and BridgeFacet.sol.

File: 2022-06-connext/contracts/contracts/core/connext/facets/PortalFacet.sol
98:    (bool success, uint256 amountIn, address adopted) = AssetLogic.swapFromLocalAssetIfNeededForExactOut(

File: 2022-06-connext/contracts/contracts/core/connext/facets/BridgeFacet.sol
1011:    (bool swapSuccess, uint256 amountIn, address adopted) = AssetLogic.swapFromLocalAssetIfNeededForExactOut(

These functions both handle functionality pertaining to loan repayments: _reconcileProcessPortal() in BridgeFacet.sol and repayAavePortal() in PortalFacet.sol.

Given that safeApprove() will revert when it is called more than once with a non-zero value argument, the use of this function could seriously impact the availability of these features and result in negative financial impacts for users.

Tools Used

Reading source code

Recommended Mitigation Steps

Replace calls to safeApprove() with safeIncreaseAllowance() or safeDecreaseAllowance() where appropriate.
If this cannot be done, use safeApprove(0) if the allowance will be changed.

References

• safeApprove() source code. Note that it is deprecated.
• Similar issue in a previous code4rena audit

[LayneHaber]

Duplicate of #154

[0xleastwood]

Valid finding with a good description. Worth of being a duplicate of #154.