Relayer Handling Swap/Mint Jammed by bad usage of SafeApprove

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/facets/BridgeFacet.sol#L389
https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/facets/BridgeFacet.sol#L1025
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#L347

Vulnerability details

Context of relevant lines of code and functions:

L389: BridgeFacet.sol: handle function

L1025: BridgeFacet.sol: Comment About This

L1029: BridgeFacet.sol: SafeIncrease of Allowance

L347: AssetLogic.sol: Usage of SafeApprove

Impact

Each call apart from the first one that relies on the swapFromLocalAssetIfNeededForExactOut function will be reverted and the whole call will suffer a reversal in cascade. This will deny future nomad relayers calls to handle and as a consequence, jam the relayer logic.

Proof of Concept

The relayer logic according to the comments and description provided by the Connext team suggests that the handle function is called once a router message has been optimistically verified. The flow of how the calls are being triggered will be the following:

1. A relayer calls handle, L389.
2. This call triggers the internal call of _reconcile, L541. This function will handle how the balance of tokens is managed regarding minting and AMM swapping.
3. If the condition under L600 is true and the following one is false, _reconcileProcessPortal is called in order to repay Aave Portal.
4. Under L1011, the call triggering the swap is made by executing swapFromLocalAssetIfNeededForExactOut L226 logic managed by the AssetLogic library.
5. The last call also triggers _swapAssetOut, trigger made at L226, where the implementation is at L304.
6. Last at L342, if the conditional statement is true, the call that reverts (at Ncalls having N>0) will be triggered L347: SafeERC20.safeApprove(IERC20(_assetIn), address(pool), _amountIn).

The first time this whole loop is called, every step will pass and the transaction will be mined. But when the second call is triggered, the step 6 will be reverted by following the SafeERC20 function logic:

/**
 * @dev Deprecated. This function has issues similar to the ones found in
 * {IERC20-approve}, and its usage is discouraged.
 *
 * Whenever possible, use {safeIncreaseAllowance} and
 * {safeDecreaseAllowance} instead.
 */
function safeApprove(
    IERC20 token,
    address spender,
    uint256 value
) internal {
    // safeApprove should only be called when setting an initial allowance,
    // or when resetting it to zero. To increase and decrease it, use
    // 'safeIncreaseAllowance' and 'safeDecreaseAllowance'
    require(
        (value == 0) || (token.allowance(address(this), spender) == 0),
        "SafeERC20: approve from non-zero to non-zero allowance"
    );
    _callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, spender, value));
}

The reversal comes from the require statement under the execution of the safeApprove call, more precisely the last argument of the or logic operator.

As the handle function is meant to be called several times, this may incur in a severe jamming of the relayer logic preventing them to perform further calls.

Recommended Mitigation Steps

OpenZeppelin suggests not using this implementation as is deprecated and it should only be called if the initial allowance wants to be set (initial state of the contract allowances). In order to manage the allowances, following the SafeERC20 standard suggestions, it is advisable to use instead SafeERC20.safeIncreaseAllowance and SafeERC20.safeDecreaseAllowance.

[ecmendenhall]

Duplicate of #154

[LayneHaber]

See #154

[0xleastwood]

Good description of the same issue in #154.