math rounding in `ETHmuiltivault.sol` is not ERC-4626 compliant `convertToAssets` should roundUp

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x69efd6ce9145587679c855fec3375f417e24c61d707caf6028a4b896a4842f0d Severity: medium

Description: Description

Per EIP 4626's 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.

so as the docs said the convertToAssets should roundUp

Impact

Other protocols that integrate with intuition and might wrongly assume that the functions handle rounding as per ERC4626 expectation. Thus, it might cause some integration problem in the future that can lead to wide range of issues for both parties.

Proof of Concept (PoC) File

as we see here this will calculate the amount of assets that would user get in exchange for amount of given share supply and doesnt roundup

    /// @notice returns amount of assets that would be exchanged by vault given amount of 'shares' provided
    ///
    /// @param shares amount of shares to calculate assets on
    /// @param id vault id to get corresponding assets for
    ///
    /// @return assets amount of assets that would be exchanged by vault given amount of 'shares' provided
    function convertToAssets(uint256 shares, uint256 id) public view returns (uint256) {
        uint256 supply = vaults[id].totalShares;
        uint256 assets = supply == 0 ? shares : shares.mulDiv(vaults[id].totalAssets, supply);
        return assets;
    }

Reccomendation

so as the docs said the convertToAssets should roundUp so use mulDivUp instead of mulDiv

[0xarshia]

hey this comment is the correction for the typo of the issue I submitted above the function meant to be convertToShares reported which contains the issue

The current implementation of convertToShares function will round down the number of shares returned due to how solidity handles Integer Division. ERC4626 expects the returned value of convertToShares to be rounded down. Thus, this function behaves as expected.

and issue is in convertToShares not in convertToAsset

[mihailo-maksa]

The ERC-4626 standard states: “If (1) it’s calculating how many shares to issue to a user for a certain amount of the underlying tokens they provide, it should round down.” Our function convertToShares correctly rounds down as intended, complying with the standard.

Therefore, this issue is invalid.