Approve Returned Value Not Validated

[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#L493

Vulnerability details

Proof-of-Concept

The _approve function attempts to performs an ERC20.approve() call, but does not check if the returned value is true (Succeed) or false (Failed). Some tokens do not revert if the approval failed but return false instead.

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

/**
 * @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);
    }
}

Impact

Tokens that don't actually perform the approve and return false might be counted as a correct approve. Thus, the function might proceed with the execution and assume that there is sufficient allowance to work with. This might result in a revert at the later stage of the execution when the code notice that there is insufficient allowance during transfer.

_mintFCashPosition function that relies on _approve might break if this issue occur, causing the manager to be unable to mint new fCash position.

Recommended Mitigation Steps

Check the return value of ERC20 approve operation to validate that they were successfully completed.

[ckoopmann]

The SetToken uses functionCallWithValue method from OpenZeppelin's Address library to execute these calls, which raises an error upon a failed call, so the above described issue should not apply.

See: https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/SetToken.sol#L207