Missing check for zero shares minted when users deposit into a vault

[code423n4]

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L960 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L873

Vulnerability details

Impact

When users deposit their assets, the resulting shares might be zero due to rounding.

If this happens the transaction will not revert and no shares will be minted, but users will still lose their funds and gain nothing.

Proof of Concept

There are no checks for zero shares minted in deposit, nor in _deposit:

function deposit(uint256 _assets, address _receiver) public virtual override returns (uint256) {
    if (_assets > maxDeposit(_receiver))
      revert DepositMoreThanMax(_receiver, _assets, maxDeposit(_receiver));

    uint256 _shares = _convertToShares(_assets, Math.Rounding.Down);
    _deposit(msg.sender, _receiver, _assets, _shares);

    return _shares;
}

The issue can be replicated with the following steps:

1. Bob increased his allowance to the vault, and calls the deposit function.
2. The exchange rate is currently very high, so this line will round to zero:

_assets.mulDiv(_assetUnit, _exchangeRate, _rounding)

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L873

3. Resulting shares are zero as _rounding is down (this is correct and EIP-4626 compliant), but then the transaction doesn't revert.

4. Bob transfered his _assets but he gains zero shares, losing his funds.

Tools Used

Manual review

Recommended Mitigation Steps

Consider reverting the transaction if a deposit would result in zero shares minted.

Assessed type

Invalid Validation

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

Picodes marked the issue as grade-b

[PierrickGT]

Fixed in the following PR: https://github.com/GenerationSoftware/pt-v5-vault/pull/17