Audit: MANETH-30 - Githubissues

Status

Addressed

Type

Vulnerability

Severity

High

Code Snippet:

 function grantRewards() external payable {
        // Process the withdrawal queue, paying out any pending withdrawal tickets before updating the fraction.
        processWithdrawalQueue();
        if (!(msg.sender == address(stakingModule) || msg.sender == mevEthShareVault)) revert MevEthErrors.InvalidSender();

        /// @dev Note that while a small possiblity, it is possible for the MevEthShareVault rewards + fraction.elastic to overflow a uint128.
        /// @dev in this case, the grantRewards call will fail and the funds will be secured to the MevEthShareVault.beneficiary address.
        fraction.elastic += uint128(msg.value);
        emit Rewards(msg.sender, msg.value);
    }

    /// @notice  Emitted when validator withdraw funds are received.
    event ValidatorWithdraw(address sender, uint256 amount);

    /// @notice Allows the MevEthShareVault or the staking module to withdraw validator funds from the contract.
    /// @dev Before updating the fraction, the withdrawal queue is processed, which pays out any pending withdrawals.
    /// @dev This function is only callable by the MevEthShareVault or the staking module.
    function grantValidatorWithdraw() external payable {
        // Check that the sender is the staking module or the MevEthShareVault.
        if (!(msg.sender == address(stakingModule) || msg.sender == mevEthShareVault)) revert MevEthErrors.InvalidSender();
    function _withdraw(address receiver, address owner, uint256 assets, uint256 shares) internal {
        if (address(this).balance >= assets) {
            emit Withdraw(msg.sender, owner, receiver, assets, shares);
            WETH.deposit{ value: assets }();
            ERC20(address(WETH)).safeTransfer(receiver, assets);
        } else {
            uint256 availableBalance = address(this).balance;
            uint256 amountOwed = assets - availableBalance;
            emit WithdrawalQueueOpened(receiver, amountOwed);
            withdrawalQueue[queueLength] = WithdrawalTicket({ receiver: receiver, amount: amountOwed });
            queueLength++; //@audit could queueLength overwrite itself??
            if (!_isZero(availableBalance)) {
                emit Withdraw(msg.sender, owner, receiver, availableBalance, convertToShares(availableBalance));
                WETH.deposit{ value: availableBalance }();
                ERC20(address(WETH)).safeTransfer(receiver, availableBalance); //@audit-issue this doesnt get subtracted from amountOwed
            }                                                                    // leading to receiver getting this amount twice!
        }                                                                        // whale could drain the vault this way by 
    }  
    function withdraw(uint256 assets, address receiver, address owner) external returns (uint256 shares) {
        // Convert the assets to shares and check if the owner has the allowance to withdraw the shares.
        shares = convertToShares(assets);

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

        // Update the elastic and base
        fraction.elastic -= uint128(assets);
        fraction.base -= uint128(shares);

        // If the base is less than the minimum deposit, revert
        if (fraction.base < MIN_DEPOSIT) {
            revert MevEthErrors.BelowMinimum();
        }

        // Burn the shares and emit a withdraw event for offchain listeners to know that a withdraw has occured
        _burn(owner, shares);

        // Withdraw the assets from the Mevth contract
        _withdraw(receiver, owner, assets, shares);
    }
    function createValidator(IStakingModule.ValidatorData calldata newData) external onlyOperator stakingUnpaused {
        IStakingModule _stakingModule = stakingModule;
        // Determine how big deposit is for the validator
        // *Note this will change if Rocketpool or similar modules are used
        uint256 depositSize = _stakingModule.VALIDATOR_DEPOSIT_SIZE();
        // if balance < VALIDATOR_DEPOSIT_SIZE + (elastic*bufferNumerator/100) OR 31 eth, whatever is biggest. 
        if (address(this).balance < depositSize + calculateNeededEtherBuffer()) {
            revert MevEthErrors.NotEnoughEth();
        }

        // Deposit the Ether into the staking contract
        _stakingModule.deposit{ value: depositSize }(newData);

        emit ValidatorCreated(address(_stakingModule), newData);
    }

Remediation

We would recommend to implement a minimum withdrawal amount, which should be equal to the minimum deposit amount (0.01 ETH). This ensures that the user would actually have deposited that amount and so a minimum withdrawal amount would not limit anyone in withdrawing.

In addition to this, we would also recommend to have a maximum withdrawal queue length and/or a maximum loop length in processWithdrawalQueue so that calls would not revert by going out-of-gas.

Description


Root cause: queueLength can be inflated to cause a gas out of bounds DOS, consequently preventing grantRewards() and grantValidatorWithdraw() to be called successfully.

Conditions necessary:

MevEth address(this).balance must be low, making the exploit more likely at the early stages of the protocol, but could happen at any time, subject to the ratio between withdraws, deposits and number of validators being created. If validators are being created as soon as the contract balance reaches 32 ETH, than this exploit can be performed extremely often.

For simplicity lets assume validators are created as soon as the check in L307 passes and attacker is the first depositor.

L307 check reverts if (address(this).balance < depositSize + calculateNeededEtherBuffer())

Obs: Values below could differ based on VALIDATOR_DEPOSIT_SIZE(), so attackers first deposit could be much lower.

- Attacker deposits 63 ETH → MevEth balance = 63 ETH
- createValidator() is called → MevEth balance = 31 ETH
- Attacker withdraws 31 ETH -> MevEth balance = 0 ETH
- Attackers shares are still worth 32 ETH.
- Attacker withdraws 0.01 ETH worth of shares, but because contract has no balance left the queueLength increases by 1.
- Attacker repeats step above many times.
- Others come and deposit().
- createValidator() is called again.
- Who ever deposited will never be able to withdraw() or redeem() their assets because grantRewards() and grantValidatorWithdraw()}}will always revert due to{{processWithdrawalQueue() suffering a gas out of bounds DOS. Meaning the ETH won’t be able to be deposited back into MevEth.

It is important to highlight that the attacker doesn’t need to be the first depositor, as this can be performed anytime someone is owed more assets than what the contract has in its balance.

manifoldfinance / mevETH2