Approve not compatible with Tether (USDT) implementation

[code423n4]

Lines of code

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

Vulnerability details

Impact

Approve not compatible with Tether (USDT) implementation.

Proof of Concept

Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens. For example Tether (USDT or CVX)'s approve() function will revert if the current approval is not zero, to protect against front-running changes of approvals.

function approve(address _spender, uint _value) public onlyPayloadSize(2 * 32) {

    // To change the approve amount you first have to reduce the addresses`
    //  allowance to zero by calling `approve(_spender, 0)` if it is not
    //  already 0 to mitigate the race condition described here:
    //  https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729
    require(!((_value != 0) && (allowed[msg.sender][_spender] != 0)));

    allowed[msg.sender][_spender] = _value;
    Approval(msg.sender, _spender, _value);
}

The code as currently implemented does not handle these sorts of tokens properly, which would prevent USDT or CVX, from being used by this project.

Affected source code:

• AssetLogic.sol#L347

Recommended Mitigation Steps

Change from:

SafeERC20.safeApprove(IERC20(_assetIn), address(pool), _amountIn);

to:

SafeERC20.safeIncreaseAllowance(IERC20(_assetIn), address(pool), _amountIn);

[jakekidd]

Duplicate of #154

[0xleastwood]

While the warden's finding is valid, they have failed to clearly outline the impact on functions related to the bridge transfer logic. As such, I think QA makes more sense.

[0xleastwood]

Merging with #71.