manifoldfinance / mevETH2

mevETH LST Protocol - Repo has migrated see link
https://github.com/MEV-Protocol/meveth
27 stars 2 forks source link

Audit: MANETH-3 #112

Closed 0xKitsune closed 1 year ago

0xKitsune commented 1 year ago

Status

Reported

Type

Vulnerability

Severity

Highest

Code Snippet:

    function withdraw(uint256 assets, address receiver, address owner) external returns (uint256 shares) {
        shares = convertToShares(assets);

        if (owner != msg.sender) {
            if (allowance[owner][msg.sender] < shares) revert MevEthErrors.TransferExceedsAllowance();
            unchecked {
                allowance[owner][msg.sender] -= shares;
            }
        }

        unchecked {
            fraction.elastic -= uint128(assets);
            fraction.base -= uint128(shares);
        }

        if (fraction.base > 10_000) {
            revert MevEthErrors.BelowMinimum();
        }

        _burn(owner, shares);

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

        WETH.deposit{ value: assets }();
        ERC20(address(WETH)).safeTransfer(receiver, assets);
    }
    function redeem(uint256 shares, address receiver, address owner) external returns (uint256 assets) {
        assets = convertToAssets(shares);

        if (owner != msg.sender) {
            if (allowance[owner][msg.sender] < shares) revert MevEthErrors.TransferExceedsAllowance();
            unchecked {
                allowance[owner][msg.sender] -= shares;
            }
        }

        unchecked {
            fraction.elastic -= uint128(assets);
            fraction.base -= uint128(shares);
        }

        if (fraction.base > 10_000) {
            revert MevEthErrors.BelowMinimum();
        }

        _burn(owner, shares);

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

        WETH.deposit{ value: assets }();
        ERC20(address(WETH)).safeTransfer(receiver, assets);
    }

Remediation

The issue arises from the incorrect use of the comparison operator in the check.

Change

        if (fraction.base > 10_000) {
            revert MevEthErrors.BelowMinimum();
        }

to

        if (fraction.base < 10_000) {
            revert MevEthErrors.BelowMinimum();
        }

Description


PATH: MevEth.sol

The withdraw() and redeem() functions in the MevEth contract include a check to ensure that the contract has a minimum balance to handle potential withdrawal and redemption requests. However, the current implementation of the check is problematic: if (fraction.base > 10_000) lines 496 and 539, as it could lead to a scenario where the contract has 10_000 shares or more (most likely), causing further withdrawals and redemptions to be restricted.
sandybradley commented 1 year ago

Fixed in previous commit: https://github.com/manifoldfinance/mevETH2/commit/5760acec9a4de213058acf55e71770978d2ec57e