SemiFungibleVault's previewWithdraw/previewRedeem implementation is not ERC-4626 compliant

[code423n4]

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/SemiFungibleVault.sol#L205-L214
https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/SemiFungibleVault.sol#L221-L228

Vulnerability details

Impact

ERC-4626 defines the previewWithdraw() and previewRedeem() functions with the intention of being used as a way to "simulate" a withdraw. Thus, the previewWithdraw() and previewRedeem() functions should revert when passed a set of parameters that would cause withdraw() to revert.

In addition, previewWithdraw() and previewRedeem() must be inclusive of withdraw/redemption fees, as defined in the ERC-4626 spec. SemiFungibleVault's implementation of previewWithdraw() and previewRedeem() do not factor in fees, making the contract non-compliant with ERC-4626.

Proof of Concept

previewWithdraw() implementation
previewRedeem() implementation

previewWithdraw() and previewMint() fail to revert under the following conditions:

• When the epoch provided has not started yet
• When the market referenced by the id does not exist

previewWithdraw() and previewMint() fail to include fee calculations that are performed by withdraw() here.

Tools Used

Manual Review

Recommended Mitigation Steps

The previewWithdraw() and previewMint() functions should be modified so they revert whenever the withdraw() function would revert.

In addition, the previewWithdraw() and previewMint() functions must be modified so their calculations are inclusive of protocol fees.

[HickupHH3]

dup #47, considered to be part of #41