EIP4626 violated by Vault

[code423n4]

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L226
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/SemiFungibleVault.sol#L205

Vulnerability details

Impact & Proof Of Concept

EIP-4626 states the following:

The convertTo functions serve as rough estimates that do not account for operation specific details like withdrawal fees, etc. They were included for frontends and applications that need an average value of shares or assets, not an exact value possibly including slippage or other fees. For applications that need an exact value that attempts to account for fees and slippage we have included a corresponding preview function to match each mutable function. These functions must not account for deposit or withdrawal limits, to ensure they are easily composable, the max functions are provided for that purpose.

However, this is not true for the vaults previewWithdraw. This function returns the amount that the user will get plus the withdrawal fees that are tranferred to the treasury.

Rationale for high risk: In a previous contest, a similar issue was marked high risk with the following reasoning:

EIP4626 is aimed to create a consistent and robust implementation patterns for Tokenized Vaults. A slight deviation from 4626 would broke composability and potentially lead to loss of fund (POC in code-423n4/2022-06-notional-coop-findings#88 can be an example). It is counterproductive to implement EIP4626 but does not conform to it fully.

I agree with that reasoning. When other smart contracts try to integrate with these vaults (which is the motiviation for having these standards, after all), this could lead to significant problems, including a loss of funds. For instance, a third-party contract could transfer the amount that is returned by previewWithdraw to the user that requested the withdrawal. But because the smart contract receives less assets, this will lead to one of two situations:
1.) The asset balance of the smart contract is enough to cover the withdrawal fee: In such a situation, the user would steal assets that belong to other users.
2.) The asset balance is not enough: No withdrawals are possible, the funds are stuck.

Recommended Mitigation Steps

Subtract the fees in previewWithdraw.

[3xHarry]

@MiguelBits i agree with this

[HickupHH3]

dup of #47