search

code-423n4 / 2022-06-notional-coop-findings

1 stars 1 forks source link

Users Might Not Be Able To Purchase Or Redeem SetToken #154

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

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#L309 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

Whenever a setToken is issued or redeemed, the moduleIssueHook and moduleRedeemHook will be triggered. These two hooks will in turn call the _redeemMaturedPositions function to ensure that no matured fCash positions remain in the Set by redeeming any matured fCash position.

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

The _redeemMaturedPositions will loop through all its fCash positions and attempts to redeem any fCash position that has already matured. However, if one of the fCash redemptions fails, it will cause the entire function to revert. If this happens, no one could purchase or redeem the setToken because moduleIssueHook and modileRedeemHook hooks will revert every single time. Thus, the setToken issuance and redemption will stop working entirely and this setToken can be considered "bricked".

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

Impact

User will not be able to purchase or redeem the setToken. User's fund will stuck in the SetToken Contract. Unable to remove matured fCash positions from SetToken and update positions of its asset token.

Recommended Mitigation Steps

This is a problem commonly encountered whenever a method of a smart contract calls another contract – you cannot rely on the other contract to work 100% of the time, and it is dangerous to assume that the external call will always be successful.

It is recommended to:

ckoopmann commented 2 years ago

The _isWrappedFCash(component) should make sure that these calls are only executed on valid wrappedfCash instances deployed from the configured factory.

The issue does not outline a practical scenario / test case where this issue would actually arise. If it did the manager could probably still remove / redeem the component either via this module, or one of the other Trade modules.

However I'll have to look into this if I can find a scenario where this would actually arise. I will also consider making this redeem step optional, but I will have to think more about this as this might introduce other more serious issues.

ckoopmann commented 2 years ago

@jeffywu Do you see any scenario where the redemption of a matured fCash position might fail in this context`?

EDIT: Just noticed that https://github.com/code-423n4/2022-06-notional-coop-findings/issues/226 mentions (among others) the scenario of USDC tokens being blocked which I guess is unlikely but outside of our control. This makes me think we might want to make the redemption of matured tokens during issuance / redemption optional.

ckoopmann commented 2 years ago

Changed label from acknowledged to confirmed, since on second thought I think we likely want to adpot some kind of mitigation strategy for this. However still tentative / unclear which strategy we want to adopt.