Did Not Enforce fCash To Be A Component Of SetToken Before Minting

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

Vulnerability details

Proof-of-Concept

Assume that the manager decided to add a fCash position called "Wrapped fDAI @ 10 October 2022", which will mature at 10 October 2022, to the SetToken.

To do so, the manager will call the NotionalTradeModule.mintFCashPosition function. The manager set the _sendToken to the asset token, which is cDAI in this example. Within the function, it will check if the _sendToken is a component in the SetToken. If not, the function will revert with the "Send token must be an index component" error. At this stage, it is clear-cut to the manager that the _sendToken, which is cDAI in this example, has to be added to the components of the SetToken. Otherwise, the manager cannot proceed with the minting process.

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

/**
 * @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)
{
    require(_setToken.isComponent(address(_sendToken)), "Send token must be an index component");

    IWrappedfCashComplete wrappedfCash = _deployWrappedfCash(_currencyId, _maturity);
    return _mintFCashPosition(_setToken, wrappedfCash, IERC20(_sendToken), _mintAmount, _maxSendAmount);
}

Assume the manager called the NotionalTradeModule.mintFCashPosition and the transaction has completed. At this point, the SetToken holds 100 "Wrapped fDAI @ 10 October 2022" tokens.

In this scenario, let assume that manager did not call the setToken.addComponent("Wrapped fDAI @ 10 October 2022") function to add the newly added fCash position to the components of the SetToken before calling the NotionalTradeModule.mintFCashPosition function (minting process).

Note that when the `NotionalTradeModule.mintFCashPosition function is called, it only check whether the _sendToken is a component of the SetToken. However, it did not check whether the fCash ("Wrapped fDAI @ 10 October 2022") is a component of the SetToken before minting.

Thus, it is possible to reach a state where the fCash ("Wrapped fDAI @ 10 October 2022") is minted to the SetToken, but the fCash ("Wrapped fDAI @ 10 October 2022") is not a component of this SetToken.

Impact

If this happens, it will cause a number of issues as described below:

• How _redeemMaturedPositions function works is that it will loop through all the components of the SetToken and attempt to redeem them if a fCash has matured. If a fCash address is not added to the components, this means that this particular matured fCash will never be redeemed. _redeemMaturedPositions is called in many occasions (e.g. Within SetToken issuance and redemption hooks ) and failure of this function can lead to internal accounting issue.
• When a user redeems their SetToken, the contract will loop through all the components of the SetToken and calculate the share of the components that the user is entitled to based on the amount the user is trying to redeem. Since the fCash is not in the components of the SetToken, user will never get his fCash during redemption. As users already burned their SetToken during redemption, so there is no way for them to get back their share of fCash even if the manager realises and fixes this issue at a later date by adding the fCash to the components.

Recommended Mitigation Steps

To eliminate such an event from happening, it is recommended to implement additional validation check to ensure that the system will never reach a state where a fCash has been minted to a SetToken, but this fCash is not a component of the SetToken.

Without this control in place, it is likely that this issue will happen as no one can ensure that all managers that utilise the NotionalTradeModule module will know that they are required to add the fCash to components of the SetToken before calling NotionalTradeModule.mintFCashPosition function.

Additionally, human-error might happen and they might forget to perform this step. Thus, it is essential for the protocol to implement the necessary controls to prevent their manager from falling into this pitfall.

Additional require statement can be added as shown below:

/**
 * @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( // @audit-ok
    ISetToken _setToken,
    uint16 _currencyId,
    uint40 _maturity,
    uint256 _mintAmount,
    address _sendToken,
    uint256 _maxSendAmount
)
    external
    nonReentrant
    onlyManagerAndValidSet(_setToken)
    returns(uint256)
{
    require(_setToken.isComponent(address(_sendToken)), "Send token must be an index component");

    IWrappedfCashComplete wrappedfCash = _deployWrappedfCash(_currencyId, _maturity);
+   require(_setToken.isComponent(wrappedfCash), "FCash to mint must be an index component");
    return _mintFCashPosition(_setToken, wrappedfCash, IERC20(_sendToken), _mintAmount, _maxSendAmount);
}

[ckoopmann]

The fCash token should be added as a component via the _updateSetTokenPositions method in _mintFCashPosition automatically. So the manager does not have to do this manually. (which is not possible anyway since the addComponent method is callable via authorized set protocol modules anyway.

I don't see any reasoning / evidence here that points to this mechanism not working.