Matured Position Can Be Redeemed For Underlying Token

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

Vulnerability details

Proof-of-Concept

Per the comment of redeemMaturedPosition function, it states that matured fCash positions will be redeemed for their asset token (cToken). Refer to the second line of the developer comment.

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

/**
 * @dev CALLABLE BY ANYBODY: Redeem all matured fCash positions of given setToken
 * Redeem all fCash positions that have reached maturity for their asset token (cToken)
 * This will update the set tokens components and positions (removes matured fCash positions and creates / increases positions of the asset token).
 * @param _setToken                     Instance of the SetToken
 */
function redeemMaturedPositions(ISetToken _setToken) public nonReentrant onlyValidAndInitializedSet(_setToken) {
    _redeemMaturedPositions(_setToken);
}

However, this statement is not entirely true as it is possible for the manager to configure the redeemToUnderlying array to force the matured fCash position to be redeemed for their underlying token (e.g. DAI, USDC). The bool toUnderlying = redeemToUnderlying[_setToken]; code will force the redemption to be done in asset tokens or underlying tokens depending on Manager's prior configuration.

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

/**
 * @dev Redeem all matured fCash positions for the given SetToken
 */
function _redeemMaturedPositions(ISetToken _setToken)
internal
{
    ISetToken.Position[] memory positions = _setToken.getPositions();
    uint positionsLength = positions.length;

    bool toUnderlying = redeemToUnderlying[_setToken];

    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);
                if(fCashPosition.hasMatured()) {
                    (IERC20 receiveToken,) = fCashPosition.getToken(toUnderlying);
                    if(address(receiveToken) == ETH_ADDRESS) {
                        receiveToken = weth;
                    }
                    uint256 fCashBalance = fCashPosition.balanceOf(address(_setToken));
                    _redeemFCashPosition(_setToken, fCashPosition, receiveToken, fCashBalance, 0);
                }
            }
        }
    }
}

There is a major difference between redeeming to asset token (cToken)(e.g. cDAI) and underlying token (e.g. DAI). The asset token (cToken) will continue to passively accrue cToken lending rate, while the underlying token (e.g. DAI) does not. Thus, having cToken in the SetToken would indirectly increase the valuation of a SetToken compared to having underlying token.

Thus, the redeemMaturedPositions need to be clear (in implemention or comment) whether the matured positions will only be redeemed for asset token OR the matured position might be redeemed for asset or underlying token depending on the Manager's decision.

Otherwise, investors might make certain investment decision based on the wrong information given to them and causing them to loss fund in the worst case scenario. This creates unnecessary complication for the users and protocol.

Recommended Mitigation Steps

If the redeemMaturedPositions function intention was to redeem all fCash positions that have reached maturity for their asset token (cToken) only, update the relevant functions to ensure that it is not possible for the redeemMaturedPositions function to redeem to underlying tokens.

Otherwise, update the comment of the redeemMaturedPositions function to as follows so that it is aligned with the actual implementation:

/**
 * @dev CALLABLE BY ANYBODY: Redeem all matured fCash positions of given setToken
- * Redeem all fCash positions that have reached maturity for their asset token (cToken)
+ * Redeem all fCash positions that have reached maturity for their asset token (cToken) or underlying token
 * This will update the set tokens components and positions (removes matured fCash positions and creates / increases positions of the asset token).
 * @param _setToken                     Instance of the SetToken
 */
function redeemMaturedPositions(ISetToken _setToken) public nonReentrant onlyValidAndInitializedSet(_setToken) {
    _redeemMaturedPositions(_setToken);
}

[ckoopmann]

The described behaviour is the inteded one. I agree that the function comment is inaccurate and needs to be extended to mention the possibility of redeeming to underlying.

Seems more like a QA issue to me as it only affects documentation

[gzeoneth]

Consider with #164