NotionalTradeModule's mintFCashPosition can become stuck on positive amount approval when allowance is already positive

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol#L490-L505

Vulnerability details

mintFCashPosition -> _mintFCashPosition -> _approve call sequence tend to leave positive unspent allowance, which can make new fCash positions opening unavailable for an underlying that forbid approval of a positive value if allowance is positive (most known example is USDT).

Setting severity to be medium as this is system unavailability case and USDT is supported by both Aave and Compound and, being widely used, can be reasonably expected to added to Notional.

Proof of Concept

mintFCashPosition uses arbitrary underlying _sendToken:

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol#L147-L167

    /**
     * @dev MANAGER ONLY: Trades into a new fCash position.
     * @param _setToken                   Instance of the SetToken
     * @param _currencyId                 CurrencyId of the fCash token as defined by the notional protocol. 
     * @param _maturity                   Maturity of the fCash token as defined by the notional protocol.
     * @param _mintAmount                 Amount of fCash token to mint 
     * @param _sendToken                  Token to mint from, must be either the underlying or the asset token.
     * @param _maxSendAmount              Maximum amount to spend
     */
    function mintFCashPosition(
        ISetToken _setToken,
        uint16 _currencyId,
        uint40 _maturity,
        uint256 _mintAmount,
        address _sendToken,
        uint256 _maxSendAmount
    )
        external
        nonReentrant
        onlyManagerAndValidSet(_setToken)
        returns(uint256)

Underlying _sendToken there can be an ERC20 that do not allow an approval of a positive value if allowance is positive already (for example, USDT):

https://github.com/d-xo/weird-erc20#approval-race-protections

mintFCashPosition -> _mintFCashPosition calls _approve() for _sendToken ERC20 with _maxSendAmount, which is the maximum allowed _sendToken to be spent:

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol#L433-L449

        _approve(_setToken, _fCashPosition, _sendToken, _maxSendAmount);

        uint256 preTradeSendTokenBalance = _sendToken.balanceOf(address(_setToken));
        uint256 preTradeReceiveTokenBalance = _fCashPosition.balanceOf(address(_setToken));

        _mint(_setToken, _fCashPosition, _maxSendAmount, _fCashAmount, fromUnderlying);

        (sentAmount,) = _updateSetTokenPositions(
            _setToken,
            address(_sendToken),
            preTradeSendTokenBalance,
            address(_fCashPosition),
            preTradeReceiveTokenBalance
        );

        require(sentAmount <= _maxSendAmount, "Overspent");

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol#L490-L505

    /**
     * @dev Approve the given wrappedFCash instance to spend the setToken's sendToken 
     */
    function _approve(     
        ISetToken _setToken,
        IWrappedfCashComplete _fCashPosition,
        IERC20 _sendToken,
        uint256 _maxAssetAmount
    )
    internal
    {
        if(IERC20(_sendToken).allowance(address(_setToken), address(_fCashPosition)) < _maxAssetAmount) {
            bytes memory approveCallData = abi.encodeWithSelector(_sendToken.approve.selector, address(_fCashPosition), _maxAssetAmount);
            _setToken.invoke(address(_sendToken), 0, approveCallData);
        }
    }

As _maxSendAmount is a limit, in most cases the actual _sendToken amount spent is less and the allowance will remain partially unspent. Then, if _sendToken is as described each next mintFCashPosition will be reverted, the function become unavailable.

Recommended Mitigation Steps

Consider adding zero amount approval in _approve() before actual amount approval to force zero allowance:

+           bytes memory approveCallData = abi.encodeWithSelector(_sendToken.approve.selector, address(_fCashPosition), 0);
+          _setToken.invoke(address(_sendToken), 0, approveCallData);
-           bytes memory approveCallData = abi.encodeWithSelector(_sendToken.approve.selector, address(_fCashPosition), _maxAssetAmount);
+           approveCallData = abi.encodeWithSelector(_sendToken.approve.selector, address(_fCashPosition), _maxAssetAmount);
            _setToken.invoke(address(_sendToken), 0, approveCallData);
        }
    }

[ckoopmann]

Will have to review this and see if I can reproduce this issue.

[gzeoneth]

While the variable is named _maxSendAmount, in wfCashLogic.mintViaAsset or wfCashLogic.mintViaUnderlying the entire amount is transferred. It doesn't seems to be an instance where there is dust in allowance. https://github.com/code-423n4/2022-06-notional-coop-findings/issues/89#issuecomment-1155977567 Downgrading to Low / QA.

[gzeoneth]

Consider with #228