code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

Divide by zero error on convertToShares due to totalAssets() is zero.

This will prevent user from withdrawing their asset.

Proof of Concept

    function convertToShares(uint256 assets) public view override returns (uint256 shares) {
        uint256 supply = totalSupply();
        if (supply == 0) {
            // Scales assets by the value of a single unit of fCash
            uint256 unitfCashValue = _getPresentValue(uint256(Constants.INTERNAL_TOKEN_PRECISION));
            return (assets * uint256(Constants.INTERNAL_TOKEN_PRECISION)) / unitfCashValue;
        }

        return (assets * totalSupply()) / totalAssets();
    }

Zero check is not performed on totalAssets().

totalAssets() can be zero on some case that totalSupply is too low, so that getPresentfCashValue return 0. After that, pvExternal will be 0 which mean totalAssets() == 0 while supply != 0.

In this case it will be reverted.

This is used on previewWithdraw

    /** @dev See {IERC4626-previewWithdraw} */
    function previewWithdraw(uint256 assets) public view override returns (uint256 shares) {
        if (hasMatured()) {
            shares = convertToShares(assets);
        } else {
            // If withdrawing non-matured assets, we sell them on the market (i.e. borrow)
            (uint16 currencyId, uint40 maturity) = getDecodedID();
            (shares, /* */, /* */) = NotionalV2.getfCashBorrowFromPrincipal(
                currencyId,
                assets,
                maturity,
                0,
                block.timestamp,
                true
            );
        }
    }

if fCash is matured and totalAssets is zero while having some shares. convertToShares will be reverted causing previewWithdraw to be reverted thus user cannot withdrawal.

Tools Used

Manual

Recommended Mitigation Steps

Check both supply == 0 and totalAssets() == 0 for edge case

   /** @dev See {IERC4626-convertToShares} */
    function convertToShares(uint256 assets) public view override returns (uint256 shares) {
        uint256 supply = totalSupply();
        uint256 totalAssetsVar = totalAssets();
        if (supply == 0 || totalAssetsVar == 0) {
            // Scales assets by the value of a single unit of fCash
            uint256 unitfCashValue = _getPresentValue(uint256(Constants.INTERNAL_TOKEN_PRECISION));
            return (assets * uint256(Constants.INTERNAL_TOKEN_PRECISION)) / unitfCashValue;
        }

        return (assets * supply) / totalAssetsVar;
    }

jeffywu commented 2 years ago

If the value of totalAssets == 0 then there is no value to withdraw. On an edge case that this has somehow rounded down to 0 there would be very little value to withdraw in any case, certainly not enough worth paying the gas cost for the transaction.

I think this should be a QA report.

gzeoneth commented 2 years ago

Consider with #138

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