Audit: MANETH-37 - Githubissues

Status

Reported

Type

Vulnerability

Severity

Highest

Code Snippet:

    function previewWithdraw(uint256 assets) external view returns (uint256 shares) {
        shares = convertToShares(assets);
    }
    function convertToShares(uint256 assets) public view returns (uint256 shares) {
        // So if there are no shares, then they will mint 1:1 with assets
        // Otherwise, shares will mint proportional to the amount of assets
        if (_isZero(uint256(fraction.elastic)) || _isZero(uint256(fraction.base))) {
            shares = assets;
        } else {
            shares = (assets * uint256(fraction.base)) / uint256(fraction.elastic);
        }
    }
    function previewMint(uint256 shares) external view returns (uint256 assets) {
        return convertToAssets(shares);
    }
    function convertToAssets(uint256 shares) public view returns (uint256 assets) {
        // So if there are no shares, then they will mint 1:1 with assets
        // Otherwise, shares will mint proportional to the amount of assets
        if (_isZero(uint256(fraction.elastic)) || _isZero(uint256(fraction.base))) {
            assets = shares;
        } else {
            assets = (shares * uint256(fraction.elastic)) / uint256(fraction.base);
        }
    }

Remediation

We recommend rounding up the result of previewMint and previewWithdraw as this example from the solmate library: https://github.com/transmissions11/solmate/blob/main/src/mixins/ERC4626.sol

Description


According to EIP-4626 Security Considerations https://eips.ethereum.org/EIPS/eip-4626:

“ Finally, ERC-4626 Vault implementers should be aware of the need for specific, opposing rounding directions across the different mutable and view methods, as it is considered most secure to favor the Vault itself during calculations over its users:

- If (1) it’s calculating how many shares to issue to a user for a certain amount of the underlying tokens they provide or (2) it’s determining the amount of the underlying tokens to transfer to them for returning a certain amount of shares, it should round down.
- If (1) it’s calculating the amount of shares a user has to supply to receive a given amount of the underlying tokens or (2) it’s calculating the amount of underlying tokens a user has to provide to receive a certain amount of shares, it should round up. ”
Therefore, EIP-4626 expects previewWithdraw and previewMint to have their results rounded up.

1. previewWithdraw returns the amount of shares to be burned to withdraw assets, rounding up the amount of shares ensures the vault won’t be short of assets.
2. previewMint returns the amount of assets to be minted given some shares, rounding up the amount of assets ensures the vault won’t be short of shares.
convertToShares and convertToAssets round down as they should, however the current implementation uses them previewMint and previewWithdraw, when it should not.

Similarly to MANETH-36 the biggest concerns arise from other protocols expecting MevEth to follow EIP-4626, causing integration problems, leading to a wide range of accounting issues. For example protocols being able to deposit but not withdraw, which amounts to a user loss of funds.

manifoldfinance / mevETH2