Unbounded Number Of Positions Can Cause Redemption Failure

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

Vulnerability details

Proof-of-Concept

The SetToken does not impose any upper limit on the number of positions that can be added. Thus, it is possible for the manager for add large number of positions to the SetToken until a point where an "Out of Gas" error or a "Block Gas Limit" error happens when the _redeemMaturedPositions function is executed.

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

The setToken issuance and redemption depend on the proper operation of _redeemMaturedPositions function. Thus, if the _redeemMaturedPositions function is broken, the SetToken is unusable and is "bricked" since no issuance and redemption can be done.

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

/**
 * @dev Hook called once before setToken issuance
 * @dev Ensures that no matured fCash positions are in the set when it is issued
 */
function moduleIssueHook(ISetToken _setToken, uint256 /* _setTokenAmount */) external override onlyModule(_setToken) {
    _redeemMaturedPositions(_setToken);
}

/**
 * @dev Hook called once before setToken redemption
 * @dev Ensures that no matured fCash positions are in the set when it is redeemed
 */
function moduleRedeemHook(ISetToken _setToken, uint256 /* _setTokenAmount */) external override onlyModule(_setToken) {
    _redeemMaturedPositions(_setToken);
}

Recommended Mitigation Steps

Define a max number of positions that can be added to the SetToken, and have the positions's length checked.

Alternatively, if the protocol decides not to limit the number of positions that a SetToken can hold, consider implement additional _redeemMaturedPositions function that allows users to redeem the positions in batches instead of redeeming all positions at one go.

[ckoopmann]

While it is technically true that the number of components on a setToken is not limited, the above outlined issue should not be a big issue in practice since, since the manager can add components and would have to wilfully add a large amount of tokens. Also I don't think the set token would be permanently unusable / "bricked" in this case since the manager could reduce the number of components again for example by redeeming fCash positions one by one.

Assuming a malicious set token manager, this manager might be able to create a state where users are unable to mint / redeem their tokens. However since such a set token manager would also be able to deploy arbitary IERC20 contracts and could probably add those as a set token component using one of the other trade modules, this risk might be unavoidable in the given set architecture.

I will have to check with the guys from SetProtocol if this is a known and accepted risk or see if there is a good mitigation strategy.

[gzeoneth]

Consider with #164