The transaction of the Vault#`withdraw()` will be reverted if a user assign `0` into the `_assets` parameter and the yield source of the yieldVault would be the Aave V3

[code423n4]

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L518 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L1026 https://github.com/aave/aave-v3-core/blob/master/contracts/protocol/libraries/logic/ValidationLogic.sol#L101

Vulnerability details

Impact

The transaction of the Vault#withdraw() will be reverted if a user assign 0 into the _assets parameter (of the Vault#withdraw()) and the yield source of the yieldVault would be the Aave V3.

Proof of Concept

When a user withdraw their deposited-asset from the Vault, the user call the Vault#withdraw().

Within the Vault#withdraw(), the Vault#_withdraw() would be called like this: https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L518

  /// @inheritdoc ERC4626
  function withdraw(
    uint256 _assets,
    address _receiver,
    address _owner
  ) public virtual override returns (uint256) {
    if (_assets > maxWithdraw(_owner))
      revert WithdrawMoreThanMax(_owner, _assets, maxWithdraw(_owner));

    uint256 _shares = _convertToShares(_assets, Math.Rounding.Up);
    _withdraw(msg.sender, _receiver, _owner, _assets, _shares);  /// @audit

    return _shares;
  }

Within the Vault#_withdraw(), yieldVault#withdraw() would be called like this: https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L1026

  /**
   * @inheritdoc ERC4626
   * @notice Withdraw assets and burn shares
   * @param _caller Address of the caller
   * @param _receiver Address of the receiver of the assets
   * @param _owner Owner of the shares
   * @param _assets Assets to send to the receiver
   * @param _shares Shares to burn
   */
  function _withdraw(
    address _caller,
    address _receiver,
    address _owner,
    uint256 _assets,
    uint256 _shares
  ) internal virtual override {
    if (_caller != _owner) {
      _spendAllowance(_owner, _caller, _shares);
    }
    ...
    _burn(_owner, _shares);

    _yieldVault.withdraw(_assets, address(this), address(this));   /// @audit
    SafeERC20.safeTransfer(IERC20(asset()), _receiver, _assets);

However, within the Vault#withdraw(), if a user assign 0 into the _assets parameter and the yield source of the yieldVault would be the Aave V3, this transaction will be reverted. Because Aave V3 does not allow to withdraw 0 amount like this: https://github.com/aave/aave-v3-core/blob/master/contracts/protocol/libraries/logic/ValidationLogic.sol#L101

  function validateWithdraw(
    DataTypes.ReserveCache memory reserveCache,
    uint256 amount,
    uint256 userBalance
  ) internal pure {
    require(amount != 0, Errors.INVALID_AMOUNT);  /// @audit

So the Vault#withdraw() will be reverted if the user assign zero into the _assets parameter in the Vault#withdraw().

Tools Used

• Manual review

Recommended Mitigation Steps

Within the Vault#withdraw(), consider adding an input validation in order to check whether or not a user assign more than 0 (zero) into the _assets parameter like this:

  function withdraw(
    uint256 _assets,
    address _receiver,
    address _owner
  ) public virtual override returns (uint256) {
+   require(_assets > 0, "_assets to be withdrawn must be greater than 0");    

    if (_assets > maxWithdraw(_owner))
      revert WithdrawMoreThanMax(_owner, _assets, maxWithdraw(_owner));

    uint256 _shares = _convertToShares(_assets, Math.Rounding.Up);
    _withdraw(msg.sender, _receiver, _owner, _assets, _shares);

    return _shares;
  }

Assessed type

Other

[c4-sponsor]

asselstine marked the issue as disagree with severity

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[Picodes]

No funds would be lost, and why would anyone withdraw 0

[c4-judge]

Picodes marked the issue as grade-c

[PierrickGT]

This fix has been implemented in the following PR: https://github.com/GenerationSoftware/pt-v5-vault/pull/18/files#diff-97c974f5c3c03a0cfcbc52a5b8b9ae2196d535754ff2034e2903de1fec23508aR1057 _assets could be 0 if the exchange rate has been manipulated and the conversion from shares to assets returns 0. In this case, we should not burn shares in exchange of 0 assets and we revert with the custom error: WithdrawZeroAssets