Use of SafeApprove() while swapping may revert causing DoS

[code423n4]

Lines of code

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

Vulnerability details

Impact

• _swapAssetOut() of AssetLogic.sol uses safeApprove() from openzeppelin's SafeERC20.
• safeApprove() will revert when value != 0 and allowance to the spender != 0 .
• When the _amountIn is > 0 and token.allowance(address(this), spender) > 0, the function will revert.

Proof of Concept

Snippet from SafeERC20

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

Snippet from _swapAssetOut()

function _swapAssetOut(
    // .........
        // perform the swap
        SafeERC20.safeApprove(IERC20(_assetIn), address(pool), _amountIn);
        amountIn = pool.swapExactOut(_amountOut, _assetIn, _assetOut, _maxIn);
      }
      // slippage is too high to perform swap: success = false, amountIn = 0
      ......
    }

Tools Used

Manual Analysis

Recommended Mitigation Steps

Use safeIncreaseAllowance and safeDecreaseAllowance instead of safeApprove().

[ecmendenhall]

Duplicate of #154

[jakekidd]

dup of #154

[0xleastwood]

No mention of impact on how bridge transfers would fail under these circumstances and lock funds. Downgrading to QA.

[0xleastwood]

Merging with #236.