Depositors might lose funds due to the lack of zero share check

[code423n4]

Lines of code

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

Vulnerability details

Impact

Depositors might lose funds due to the lack of checking whether the shares to be minted is equal to zero. When this happens, the assets will be deposited into the vault, but the depositors will receive zero shares. This is independent from the initial depositor attack issue, which means, even the first depositor attack issue is resolved, the issue here still needs to be addressed.

Proof of Concept

Suppose the exchange rate is 1.1e18. Alice calls deposit() with assets = 1.

  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 number of shares calculated will be 1 * 1e18 / 1.1e18 = 0

  function _convertToShares(
    uint256 _assets,
    Math.Rounding _rounding
  ) internal view virtual override returns (uint256) {
    uint256 _exchangeRate = _currentExchangeRate();

    return
      (_assets == 0 || _exchangeRate == 0)
        ? _assets
        : _assets.mulDiv(_assetUnit, _exchangeRate, _rounding);
  }

Normally, when shares = 0; the deposit() function should revert, but in this case it does not. As a result, the depositor will lose 1 asset tokens and return zero shares.

Tools Used

Manual Review

Recommended Mitigation Steps

It is critical to have a zero share check and revert when the number of shares is zero.

  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);

+   if(_shares == 0) revert ZeroSharesDeposit();
    _deposit(msg.sender, _receiver, _assets, _shares);

    return _shares;
  }

Assessed type

ERC4626

[Picodes]

The loss of funds is minimal and is due to rounding. A loss of this magnitude can happen even with a safety check.

[c4-judge]

Picodes marked the issue as unsatisfactory: Overinflated severity