Vault.sol is not EIP-4626 compliant

[code423n4]

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/Vault.sol#L244-L252
https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/SemiFungibleVault.sol#L205-L213
https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/SemiFungibleVault.sol#L237-L239
https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/SemiFungibleVault.sol#L244-L246
https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/SemiFungibleVault.sol#L251-L258
https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/SemiFungibleVault.sol#L263-L270

Vulnerability details

Impact

Other protocols that integrate with Y2K may wrongly assume that the functions are EIP-4626 compliant. Thus, it might cause integration problems in the future that can lead to wide range of issues for both parties.

Proof of Concept

All official EIP-4626 requirements can be found on it's official page. Non-compliant functions are listed below along with the reason they are not compliant:

The following functions are missing but should be present:

1. mint(uint256, address) returns (uint256)
2. redeem(uint256, address, address) returns (uint256)

The following functions are non-compliant because they don't account for withdraw and deposit locking:

1. maxDeposit
2. maxMint
3. maxWithdraw
4. maxRedeem

All of the above functions should return 0 when their respective functions are disabled (i.e. maxDeposit should return 0 when deposits are disabled)

previewDeposit is not compliant because it must account for fees which it does not

totalAssets is not compliant because it does not always return the underlying managed by the vault because it fails to include the assets paid out during a depeg or the end of the epoch.

Tools Used

Recommended Mitigation Steps

All functions listed above should be modified to meet the specifications of EIP-4626

[HickupHH3]

The premise is valid because it's stated in the README:

Y2K leverages ERC4626 Vault standard for this protocol, this contract is a fork of that standard, although we replaced all uses of ERC20 to ERC1155.

As per the ruling in a previous contest regarding EIP4626.

Judging this and all duplicate regarding EIP4626 implementation as High Risk.
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 funds. It is counterproductive to implement EIP4626 but does not conform to it fully.

The missing functions are the most problematic; one expects the mint() and redeem() to be present, but they're absent instead.

Disagree on the max*() functions issues; SemiFungibleVault is not pausable, functions can't be disabled / paused. Perhaps the inheriting contracts should override these functions, but the way I see it, they can be arbitrarily set in the template.

Agree on subsequent points mentioned.