`wfCashERC4626.sol#redeem()` Lack of slippage control for market sell

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/wfCashERC4626.sol#L205-L223

Vulnerability details

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/wfCashERC4626.sol#L205-L223

function redeem(
    uint256 shares,
    address receiver,
    address owner
) public override returns (uint256) {
    // It is more accurate and gas efficient to check the balance of the
    // receiver here than rely on the previewRedeem method.
    uint256 balanceBefore = IERC20(asset()).balanceOf(receiver);

    if (msg.sender != owner) {
        _spendAllowance(owner, msg.sender, shares);
    }
    _redeemInternal(shares, receiver, owner);

    uint256 balanceAfter = IERC20(asset()).balanceOf(receiver);
    uint256 assets = balanceAfter - balanceBefore;
    emit Withdraw(msg.sender, receiver, owner, assets, shares);
    return assets;
}

The current implementation of redeem() -> _redeemInternal() provides no parameter for slippage control.

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/wfCashLogic.sol#L202-L257

function _burn(
    address from,
    uint256 amount,
    bytes memory userData,
    bytes memory operatorData
) internal override nonReentrant {
    // Save the total supply value before burning to calculate the cash claim share
    uint256 initialTotalSupply = totalSupply();
    RedeemOpts memory opts = abi.decode(userData, (RedeemOpts));
    require(opts.receiver != address(0), "Receiver is zero address");
    // This will validate that the account has sufficient tokens to burn and make
    // any relevant underlying stateful changes to balances.
    super._burn(from, amount, userData, operatorData);

    if (hasMatured()) {
        // If the fCash has matured, then we need to ensure that the account is settled
        // and then we will transfer back the account's share of asset tokens.

        // This is a noop if the account is already settled
        NotionalV2.settleAccount(address(this));
        uint16 currencyId = getCurrencyId();

        (int256 cashBalance, /* */, /* */) = NotionalV2.getAccountBalance(currencyId, address(this));
        require(0 < cashBalance, "Negative Cash Balance");

        // This always rounds down in favor of the wrapped fCash contract.
        uint256 assetInternalCashClaim = (uint256(cashBalance) * amount) / initialTotalSupply;

        // Transfer withdrawn tokens to the `from` address
        _withdrawCashToAccount(
            currencyId,
            opts.receiver,
            _safeUint88(assetInternalCashClaim),
            opts.redeemToUnderlying
        );
    } else if (opts.transferfCash) {
        // If the fCash has not matured, then we can transfer it via ERC1155.
        // NOTE: this may fail if the destination is a contract and it does not implement 
        // the `onERC1155Received` hook. If that is the case it is possible to use a regular
        // ERC20 transfer on this contract instead.
        NotionalV2.safeTransferFrom(
            address(this), // Sending from this contract
            opts.receiver, // Where to send the fCash
            getfCashId(), // fCash identifier
            amount, // Amount of fCash to send
            userData
        );
    } else {
        _sellfCash(
            opts.receiver,
            amount,
            opts.redeemToUnderlying,
            opts.maxImpliedRate
        );
    }
}

However, when redeeming before maturity, the fCash will be market sold on Notional AMM, and a slippage control can and should be done to prevent the sandwich attack.

Recommendation

Since wfCashERC4626 is an ERC4626 compatible contract, and the standard redeem() method from ERC4626 has no parameter for slippage control, in order to remain ERC4626 compatible, instead of updating the redeem() method, consider adding a new function with minReceiveAmount parameter for redeeming before maturity, and the ERC4626-compatible redeem() without slippage control can only be used after maturity.

[jeffywu]

Methods with slippage control already exits in wfCashLogic: https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/wfCashLogic.sol#L166-L170