Deposit transfers shares not assets

[code423n4]

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L167

Vulnerability details

Impact

When depositing to the Vault, it transfers the wrong amount of tokens from the user. Deposit mistakenly transfers calculated shares, not assets:

    function deposit(
        uint256 id,
        uint256 assets,
        address receiver
    )
        ...
        require((shares = previewDeposit(id, assets)) != 0, "ZeroValue");
        asset.transferFrom(msg.sender, address(this), shares);

Users will actually deposit less than intended this way because as far as I understand shares are supposed to include accrued interest.

It overrides the deposit function from SemiFungibleVault, there it works as expected:

  asset.safeTransferFrom(msg.sender, address(this), assets);

Recommended Mitigation Steps

It should transfer from the user assets, not shares.

[3xHarry]

@MiguelBits this seems valid, users should transfer as much as they specified in WETH and the contract should mint the calculated shares?

[3xHarry]

however this will not cause any harm and no funds will be lost as, assets are the same as shares downgrade to low

[HickupHH3]

Honestly, the severity rating depends on which statement is true:

as I understand shares are supposed to include accrued interest.
vs
assets are the same as shares

Looking at the code, it seems like the latter is true.

shares
= previewDeposit(id, assets)
= convertToShares(id, assets)
= totalSupply(id) == 0 ? assets : assets.mulDivDown(totalSupply(id), totalAssets(id));
// for the latter case
= assets.mulDivDown(totalAssets(id), totalAssets(id))
= assets

Hence, I agree that the severity should be downgraded to QA.

[HickupHH3]

part of warden's QA: #486