Approving from non-zero to non-zero allowance will revert with OZ's `safeApprove()`

[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

Transaction reverting.

Proof of Concept

• Some tokens (like the very popular USDT) do not work when changing the allowance from an existing non-zero allowance value (it will revert if the current approval is not zero to protect against front-running changes of approvals). These tokens must first be approved for zero and then the actual allowance can be approved.
• Furthermore, OZ's implementation of safeApprove would throw an error if an approve is attempted from a non-zero value ("SafeERC20: approve from non-zero to non-zero allowance"): https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol#L53-L56

  require(
      (value == 0) || (token.allowance(address(this), spender) == 0),
      "SafeERC20: approve from non-zero to non-zero allowance"
  );

Affected Code

• AssetLogic.sol:

346:         // perform the swap
347:         SafeERC20.safeApprove(IERC20(_assetIn), address(pool), _amountIn);

Recommended Mitigation Steps

Set the allowance to zero immediately before the existing safeApprove() call.

[ecmendenhall]

Duplicate of #154

[jakekidd]

Closed as duplicate of https://github.com/code-423n4/2022-06-connext-findings/issues/154

[0xleastwood]

No explanation on how bridge transfers are impacted (potentially locking funds), hence I'll downgrade this to QA.

[0xleastwood]

Merging with #263.