#42539 [SC-Low] Incorrect `maxWithdraw()` returns lead to user failed withdrawals of returned maximum amount

Submitted on Mar 24th 2025 at 15:17:25 UTC by @merlinboii for Audit Comp | Yeet

• Report ID: #42539

• Report Type: Smart Contract

• Report severity: Low

• Target: https://github.com/immunefi-team/audit-comp-yeet/blob/main/src/contracts/MoneyBrinter.sol

• Impacts:

  • Contract fails to deliver promised returns, but doesn't lose value

Description

Brief/Intro

The MoneyBrinter.maxWithdraw() function returns an overestimated value that doesn't account for withdrawal fees, causing withdrawals at the reported maximum to always revert due to insufficient shares balance to burn.

Vulnerability Details

The ERC4626 implementation's maxWithdraw calculates withdrawable assets as:

function maxWithdraw(address owner) public view virtual returns (uint256) {
    return _convertToAssets(balanceOf(owner), Math.Rounding.Floor);
}

However, the previewWithdraw() implementation adds a fee:

function previewWithdraw(uint256 assets) public view override returns (uint256) {
    uint256 fee = _feeOnRaw(assets, exitFeeBasisPoints);
    return super.previewWithdraw(assets + fee);
}

This creates a mathematical inconsistency where the maxWithdraw() is returned the assets from the shares balance of owner, but the previewWithdraw() returns the shares amount from the assets + fee.

Moreover, this is not compile with EIP-4626 as according to EIP-4626 specifications, maxWithdraw MUST:

1. Return the maximum amount of assets that could be transferred from owner through withdraw and not cause a revert, which MUST NOT be higher than the actual maximum that would be accepted (it should underestimate if necessary).

2. MUST factor in both global and user-specific limits, like if withdrawals are entirely disabled (even temporarily) it MUST return 0.

3. MUST NOT revert.

Impact Details

Low (Contract fails to deliver promised returns, but doesn't lose value)

While no funds are directly at risk, this issue:

1. Cause failed transactions and wasted gas when users attempt to withdraw their reported maximum

2. Breaks core EIP-4626 functionality and compliance

References

https://github.com/immunefi-team/audit-comp-yeet/blob/main/src/contracts/MoneyBrinter.sol

https://github.com/immunefi-team/audit-comp-yeet/blob/main/src/contracts/MoneyBrinter.sol#L24

Proof of Concept

Proof of Concept

Initial state:

• Assume a user has 100 shares in the vault

• Assume the exchange rate is 1:1 (1 share = 1 asset)

• Assume exitFeeBasisPoints = 100 (1%)

1. User queries their maximum withdrawable amount

uint256 maxAssets = vault.maxWithdraw(user);
// maxAssets = _convertToAssets(balanceOf(user): 100 shares) = 100 assets

1. User attempts to withdraw this maximum amount

// maxAssets = 100 assets
vault.withdraw(100 assets, user, user);

1. The withdraw() function will revert because:

uint256 maxAssets = maxWithdraw(owner);
// First check PASSES because assets == maxAssets
if (assets > maxAssets) { 
    revert ERC4626ExceededMaxWithdraw(owner, assets, maxAssets);
}

// Calculate shares needed
uint256 shares = previewWithdraw(assets); // Calculate shares for 100 assets

// Inside previewWithdraw():
uint256 fee = _feeOnRaw(100, 100); // 100 * 100 / 10000 = 1 (with ceiling rounding)
return super.previewWithdraw(100 + 1); // Calculate shares for 101 assets
// shares = 101

// Try to burn 101 shares, but user only has 100
// REVERTS here
@> _withdraw(_msgSender(), receiver, owner, assets: 100, shares: 101);

The transaction reverts because the user needs 101 shares to withdraw 100 assets (query from maxWithdraw()), but they only have 100 shares.