Incorrect Share Calculation in Withdraw Function Violates EIP-4626 Specification

[hats-bug-reporter[bot]]

Github username: -- Twitter username: 0xgreed_ Submission hash (on-chain): 0x491b772c9c55628b7640934f10d433f9b9b41b37af2bf59898687244f3886add Severity: high

Description:

Description

The withdraw() function in the InvestToken contract incorrectly calculates shares by rounding down instead of rounding up when converting assets to shares. This violates the EIP-4626 specification which requires rounding up in the withdraw function to ensure the vault remains solvent and users cannot exploit rounding to extract more assets than they should be entitled to.

When users withdraw assets, the contract calculates the required shares by calling convertToShares(assets). However, this calculation rounds down, meaning users could potentially withdraw slightly more assets than they should be able to based on their share balance. Over many transactions, this rounding error could accumulate and lead to insolvency of the vault.

The EIP-4626 specification explicitly states that share calculations in withdraw operations must round up to prevent these types of exploits. This is because rounding down could allow users to game the system by performing many small withdrawals instead of one large withdrawal, potentially extracting more assets than their shares should allow.

Recommendation

The share calculation in the withdraw() function should be modified to round up instead of down. This can be implemented by adding 1 to the result of the division if there is any remainder.

Here's how the calculation could be modified:

shares = (assets * totalSupply() + (totalAssets() - 1)) / totalAssets();

Alternatively, you could create a separate convertToSharesRoundUp() function specifically for withdraw operations that handles the rounding up logic.

[AndreiMVP]

EIP-4626 specification which requires rounding up in the withdraw function to ensure the vault remains solvent

It's not exactly the EIP-4626 spec logic we have here since the price is not calculated based on the onchain assets, thus the vault is in no danger of running into solvency issues. The YieldOracle is central here.