QA Report

[code423n4]

Mismatch In The Number Of Parameters

The wfCashLogic.mintViaUnderlying and wfCashLogic.mintViaAsset functions only accept four (4) parameters.

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

function mintViaUnderlying(
    uint256 depositAmountExternal,
    uint88 fCashAmount,
    address receiver,
    uint32 minImpliedRate
) external override

function mintViaAsset(
    uint256 depositAmountExternal,
    uint88 fCashAmount,
    address receiver,
    uint32 minImpliedRate
) external override

However, within the NotionalTradeModule._mint, it attempts to call the wfCashLogic.mintViaUnderlying and wfCashLogic.mintViaAsset functions with five (5) parameters. For consistency, remove the last parameter from the ABI encoding as shown below:

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

function _mint(
    ISetToken _setToken,
    IWrappedfCashComplete _fCashPosition,
    uint256 _maxAssetAmount,
    uint256 _fCashAmount,
    bool _fromUnderlying
)
internal
{
    uint32 minImpliedRate = 0;

    bytes4 functionSelector = 
        _fromUnderlying ? _fCashPosition.mintViaUnderlying.selector : _fCashPosition.mintViaAsset.selector;
    bytes memory mintCallData = abi.encodeWithSelector(
        functionSelector,
        _maxAssetAmount,
        uint88(_fCashAmount),
        address(_setToken),
        minImpliedRate,
-        _fromUnderlying
    );
    _setToken.invoke(address(_fCashPosition), 0, mintCallData);
}

Lack of Input Validation

For defence-in-depth purpose, it is recommended to perform additional validation against the amount that the user is attempting to deposit, mint, withdraw and redeem to ensure that the submitted amount is valid.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/7c75b8aa89073376fb67d78a40f6d69331092c94/contracts/token/ERC20/extensions/ERC20TokenizedVault.sol#L94

function deposit(uint256 assets, address receiver) public override returns (uint256) {
+       require(assets <= maxDeposit(receiver), "deposit more than max");
    uint256 shares = previewDeposit(assets);
    // Will revert if matured
    _mintInternal(assets, _safeUint88(shares), receiver, 0, true);
    emit Deposit(msg.sender, receiver, assets, shares);
    return shares;
}

function mint(uint256 shares, address receiver) public override returns (uint256) {
+       require(shares <= maxMint(receiver), "mint more than max");
    uint256 assets = previewMint(shares);
    // Will revert if matured
    _mintInternal(assets, _safeUint88(shares), receiver, 0, true);
    emit Deposit(msg.sender, receiver, assets, shares);
    return assets;
}

function withdraw(
    uint256 assets,
    address receiver,
    address owner
) public override returns (uint256) {
+       require(assets <= maxWithdraw(owner), "withdraw more than max");
    uint256 shares = previewWithdraw(assets);

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

    emit Withdraw(msg.sender, receiver, owner, assets, shares);

    return shares;
}

function redeem(
        uint256 shares,
        address receiver,
        address owner
    ) public override returns (uint256) {
+           require(shares <= maxRedeem(owner), "redeem more than max");
        // 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;
    }

Approve Not Set To Zero First

Vulnerability Details

Not calling approve(0) first might cause the protocol to be vulnerable to Front-Run/Double-Spend Attack.

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

function _approve(
    ISetToken _setToken,
    IWrappedfCashComplete _fCashPosition,
    IERC20 _sendToken,
    uint256 _maxAssetAmount
)
internal
{
    if(IERC20(_sendToken).allowance(address(_setToken), address(_fCashPosition)) < _maxAssetAmount) {
        bytes memory approveCallData = abi.encodeWithSelector(_sendToken.approve.selector, address(_fCashPosition), _maxAssetAmount);
        _setToken.invoke(address(_sendToken), 0, approveCallData);
    }
}

Recommended Mitigation Steps

Approve with a zero amount first before setting the actual amount.

function _approve(
    ISetToken _setToken,
    IWrappedfCashComplete _fCashPosition,
    IERC20 _sendToken,
    uint256 _maxAssetAmount
)
internal
{
    if(IERC20(_sendToken).allowance(address(_setToken), address(_fCashPosition)) < _maxAssetAmount) {
+       bytes memory approveZeroCallData = abi.encodeWithSelector(_sendToken.approve.selector, address(_fCashPosition), 0);
+               _setToken.invoke(address(_sendToken), 0, approveZeroCallData);
        bytes memory approveCallData = abi.encodeWithSelector(_sendToken.approve.selector, address(_fCashPosition), _maxAssetAmount);
        _setToken.invoke(address(_sendToken), 0, approveCallData);
    }
}

Floating Pragma

Proof-of-Concept

The contract makes use of the floating-point pragma ^0.8.0. Contracts should be deployed using the same compiler version. Locking the pragma helps ensure that contracts are not unintentionally deployed using another pragma, such as an obsolete version, that may introduce issues in the contract system.

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

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "./wfCashLogic.sol";
import "../interfaces/IERC4626.sol";

contract wfCashERC4626 is IERC4626, wfCashLogic {
    constructor(INotionalV2 _notional, WETH9 _weth) wfCashLogic(_notional, _weth) {}
    ..SNIP..

Recommended Mitigation Steps

Consider locking the pragma version. It is advised that floating pragma should not be used in production

Unused Function

Proof-of-Concept

Following function was defined, but not used in any of the contracts.

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

function _safeNegInt88(uint256 x) private pure returns (int88) {
    int256 y = -int256(x);
    require(int256(type(int88).min) <= y);
    return int88(y);
}

Recommended Mitigation Steps

It is recommended to remove this function if it is not required.

[ckoopmann]

Flagging that this QA report contains Approve Not Set To Zero First which I confirmed as a mid-risk issue elsewhere.

[gzeoneth]

Flagging that this QA report contains Approve Not Set To Zero First which I confirmed as a mid-risk issue elsewhere.

Thanks but judged that as QA in #221.