Popcorn contest - yellowBirdy's results

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L496-L499 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L134-L140 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L170-L176 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L170-L176

Vulnerability details

Accrued performance fees are calculated of the difference between current value and the value recorded in highWaterMark.

DEPOSIT, MINT and WITHDRAW use syncFeeCheckpoint modifier which updates highWaterMark to current share value. If one of the above functions is called when there are any not yet taken performance fees they will be reset to 0 by virtue of setting highWaterMark to current share value.

As users will DEPOSIT, MINT and WITHDRAW possibly significantly more often than fees being taken most of the performance fees will be lost to the vault creator. It also be used to front run the takeManagementAndPerformanceFees calls.

Proof of Concept

Take the following test case where a deposit is performed after fee accrual event but before fee being taken.

  function test__performanceFeeDisapearing() public {
    uint128 amount = 315576000;
    uint256 depositAmount = 1 ether;

    _setFees(0, 0, 0, 1e17);

    asset.mint(alice, depositAmount * 2);
    vm.startPrank(alice);
    asset.approve(address(vault), depositAmount);
    vault.deposit(depositAmount, alice);
    vm.stopPrank();

    // Increase asset assets to trigger performanceFee
    asset.mint(address(adapter), amount);

    uint256 expectedFeeInAsseBefore = vault.accruedPerformanceFee();

    vm.startPrank(alice);
    asset.approve(address(vault), depositAmount);
    vault.deposit(depositAmount, alice);
    vm.stopPrank();

    uint256 expectedFeeInAssetAfter = vault.accruedPerformanceFee();
    
    assertEq(expectedFeeInAssetAfter, expectedFeeInAsseBefore);
  }

it will with the following error:

  Error: a == b not satisfied [uint]
    Expected: 31557600
      Actual: 0

Tools Used

Forge

Recommended Mitigation Steps

I can see two possible solutions:

1. Take performance fees on every DEPOSIT, MINT, WITHDRAW and REDEEM.
2. Move the performance fees to the adapter

I'm leaning towards the first as a more elegant one, keeping the separation of concerns between adapter and vault intact.