Immature fCash positions should not be left behind when removing the `NotionalTradeModule`

[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#L246-L259

Vulnerability details

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

function removeModule() external override onlyValidAndInitializedSet(ISetToken(msg.sender)) {
    ISetToken setToken = ISetToken(msg.sender);

    // Redeem matured positions prior to any removal action
    _redeemMaturedPositions(setToken);

    // Try if unregister exists on any of the modules
    address[] memory modules = setToken.getModules();
    for(uint256 i = 0; i < modules.length; i++) {
        if(modules[i].isContract()){
            try IDebtIssuanceModule(modules[i]).unregisterFromIssuanceModule(setToken) {} catch {}
        }
    }
}

Once the module is removed, it will unregister from the DebtIssuanceModule, therefore the hooks (eg, moduleIssueHook and moduleRedeemHook) won't continue to be called.

As a result, it can no longer ensure that no matured fCash positions are in the set when it is issued/redeemed.

Recommendation

We believe in removeModule(), it should either require that there are no immature fCash positions or all the immature fCash positions will be liquidated prior to the removal action.

[ckoopmann]

The described behaviour is intended.

The main use case envisioned for calling this function is to upgrade to a newer version of the NotionalTradeModule. (So we would remove the old version first and then add the new version). In this case we would not want or need to redeem all positions.

If the manager decides to permanently remove the NotionalTradeModule he can manually redeem all fCash positions first avoiding any of the issues raised here.