Did Not Enforce Underlying To Be A Component Of SetToken Before Calling `setRedeemToUnderlying`

[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 https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol#L185 https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol#L294

Vulnerability details

Proof-of-Concept

Within the NotionalTradeModule.mintFCashPosition function, it will check if the _sendToken submitted is a component of the SetToken before the minting process. Within the NotionalTradeModule.redeemFCashPosition function, it will check if the wrappedfCash is is a component of the SetToken before the redemption.

By default, when a fCash (e.g. Wrapped fDAI @ 10 October 2022) is redeemed, it will redeem to an asset token (e.g. cDAI). In most cases, a manager will mint fCash by calling NotionalTradeModule.mintFCashPosition with an asset token (e.g. cDAI) as the _sendToken. Thus, the asset token (e.g. cDAI) will be added to the components of the SetToken because if the manager did not do so, the minting process will revert due to the require(_setToken.isComponent(address(_sendToken)), "Send token must be an index component");

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);
}

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

/**
 * @dev MANAGER ONLY: Trades out of an existing fCash position.
 * Will revert if no wrapper for the selected fCash token was deployed
 * @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 _redeemAmount               Amount of fCash token to redeem 
 * @param _receiveToken               Token to redeem into, must be either asset or underlying token of the fCash token
 * @param _minReceiveAmount           Minimum amount of receive token to receive
 */
function redeemFCashPosition(
    ISetToken _setToken,
    uint16 _currencyId,
    uint40 _maturity,
    uint256 _redeemAmount,
    address _receiveToken,
    uint256 _minReceiveAmount
)
    external
    nonReentrant
    onlyManagerAndValidSet(_setToken)
    returns(uint256)
{
    IWrappedfCashComplete wrappedfCash = _getWrappedfCash(_currencyId, _maturity);
    require(_setToken.isComponent(address(wrappedfCash)), "FCash to redeem must be an index component");

    return _redeemFCashPosition(_setToken, wrappedfCash, IERC20(_receiveToken), _redeemAmount, _minReceiveAmount);
}

A manager could configure a SetToken to always redeem to underlying tokens upon reaching maturity by calling the NotionalTradeModule.setRedeemToUnderlying function.

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

// Mapping for a set token, wether or not to redeem to underlying upon reaching maturity
mapping(ISetToken => bool) public redeemToUnderlying;

function setRedeemToUnderlying(
    ISetToken _setToken,
    bool _toUnderlying
)
external
onlyManagerAndValidSet(_setToken)
{
    redeemToUnderlying[_setToken] = _toUnderlying;
}

Assume that the manager has just minted 100 "Wrapped fDAI @ 10 October 2022" tokens by calling NotionalTradeModule.mintFCashPosition with an asset token (e.g. cDAI) as the _sendToken. After minting, the manager decided to configure the SetToken to always redeem to underlying token instead of asset token upon reaching maturity. Thus, the fCash will always be redeemed to its underlying token, which is DAI in this example.

It was observed that throughout the process, the system did not enforce that the underlying token (DAI) must be an index component (in other word DAI must be a component of the SetToken). Thus, it is possible to reach a state where an underlying token (e.g.DAI) exists in the SetToken contract, but it is not a component of the SetToken.

Impact

If this happens, following issue will occur:

• After theNotionalTradeModule.redeemMaturedPositions function is triggered to redeem matured fCash positions, the redeemed underlying token will be sent to the SetToken Contract.
• 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 DAI is not in the components of the SetToken, user will never get his DAI during redemption. As users already burned their SetToken during redemption, so there is no way for them to get back their share of DAI even if the manager realises and fixes this issue at a later date by adding DAI 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 an underlying token (e.g.DAI) exists in the SetToken contract, but it 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 underlying token to components of the SetToken before calling NotionalTradeModule.setRedeemToUnderlying 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.

Considering implement additional checks as shown below:

function setRedeemToUnderlying(
    ISetToken _setToken,
    bool _toUnderlying
)
external
onlyManagerAndValidSet(_setToken)
{
    // Ensure that all the fCash's underlying is an index component before allowing manager to configure to this setting to True
    if (_toUnderlying) {
        checkAllUnderlyingInComponents(_setToken);
    }
    redeemToUnderlying[_setToken] = _toUnderlying;
}

function checkAllUnderlyingInComponents(ISetToken _setToken) internal
{
    ISetToken.Position[] memory positions = _setToken.getPositions();
    uint positionsLength = positions.length;

    for(uint256 i = 0; i < positionsLength; i++) {
        // Check that the given position is an equity position
        if(positions[i].unit > 0) {
            address component = positions[i].component;
            if(_isWrappedFCash(component)) {
                IWrappedfCashComplete fCashPosition = IWrappedfCashComplete(component);
                IERC20 underlyingToken = fCashPosition.getUnderlyingToken();
                require(_setToken.isComponent(underlyingToken), "Error: One or more fCash's underlying is not an index component");
            }
        }
     }
}

[ckoopmann]

When matured fCash components are redeemed to the underlying token, the underlying token should be added as a component via the _updateSetTokenPositions method in _redeemFCashPosition. So the underlying token does not have to be a set component before that redemption.

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