Anyone can steal assets from a vault by abusing the deposit logic

[code423n4]

Lines of code

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

Vulnerability details

Impact

Loss of funds from any vault created through VaultFactory, the funds can be swept by anyone after a legit deposit performed by a direct transfer.

Proof of Concept

There are two main ways to deposit into a vault, according to the sponsor:

1. A contract can transfer assets to the Vault then call the deposit() function to deposit the additional balance.

2. A contract can approve a Vault allowance and call the deposit() function. The Vault will use safeTransferFrom to transfer-in the required tokens.

Due to point 1, the vault is vulnerable to being swept by anyone. This is part of the deposit logic:

// We only need to deposit new assets if there is not enough assets in the vault to fulfill the deposit
if (_assets > _vaultAssets) {
  uint256 _assetsDeposit;

  unchecked {
    if (_vaultAssets != 0) {
      _assetsDeposit = _assets - _vaultAssets;
    }
  }

  SafeERC20.safeTransferFrom(
    _asset,
    _caller,
    address(this),
    _assetsDeposit != 0 ? _assetsDeposit : _assets
  );
}

_yieldVault.deposit(_assets, address(this));
_mint(_receiver, _shares);

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

Basically, if there are already some funds in the vault, and the deposited _assets are less or equal to _vaultAssets, the safeTransferFrom will be skipped, but the shares will still be minted to the _receiver.

An attack would look like this:

1. Alice transfers USDC directly to the vault
2. Alice calls the deposit function to get the shares
3. Bob sees this transaction and front-runs it with a deposit call with _assets <= _vaultAssets
4. The transfer logic is skipped, so Bob doesn't lose any assets, but he gets shares for free
5. Bob can withdraw his shares to get a profit

Tools Used

Manual review

Recommended Mitigation Steps

Consider always performing the transfer, even when there are enough funds inside the vault.

Assessed type

Invalid Validation

[Picodes]

I assume this is not the intended behavior and user are not supposed to send tokens in advance, unless they go through a router and do both actions in the same transaction.

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[PierrickGT]

Exactly, this is definitely not the intended behavior.