`managementFee` is unfair and can be used to steal stakers deposits

[code423n4]

Lines of code

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

Vulnerability details

Description

managementFee is a fee that is taken on TVL and calculated per year:

File: Vault.sol

429:    function accruedManagementFee() public view returns (uint256) {
430:        uint256 managementFee = fees.management;
431:        return
432:            managementFee > 0
433:                ? managementFee.mulDiv(
434:                    totalAssets() * (block.timestamp - feesUpdatedAt),
435:                    SECONDS_PER_YEAR,
436:                    Math.Rounding.Down
437:                ) / 1e18
438:                : 0;
439:    }

feesUpdatedAt is set at feeCollection. However, if no one calls this at every deposit the last depositor before collection will be unfairly charged.

Impact

feeRecipient can take an unfair fee from new stakers.

A new staker could look at the feesUpdatedAt and see that the fee hasn't been collected for a long time and call takeManagementAndPerformanceFees before their deposit. But it is not reasonable to expect a regular user to check these things before staking in a vault.

An existing staker is incentivized to not collect fees as that lessens the value of their shares (as fee collection mints shares without adding assets) so you cannot expect them to collect fees either.

And lastly the feeRecipient wants to collect when the most assets are in the contract with as much time as possible since last fee collection.

Hence no one involved is incentivized to collect fees often at the expense of new users staking a lot of assets.

Proof of Concept

PoC test in Vault.t.sol

  function test__take_managementFee_directly_after_deposit() public {
    _setFees(0, 0, 1e17, 0); // 10% mgmt fee

    // a year passes
    vm.warp(block.timestamp + 356 days);

    asset.mint(alice, 1e18);    
    vm.startPrank(alice);
    asset.approve(address(vault), 1e18);

    // alice deposits into the vault
    vault.deposit(1e18);
    vm.stopPrank();

    // fees are charged, managementFee is 10% of shares
    console.log("managementFee ",vault.accruedManagementFee());
    vault.takeManagementAndPerformanceFees();

    // feeReceiver got 10%
    vm.startPrank(feeRecipient);
    vault.redeem(vault.balanceOf(feeRecipient));
    vm.stopPrank();
    console.log("feeRecipient",asset.balanceOf(feeRecipient));

    vm.startPrank(alice);
    vault.redeem(vault.balanceOf(alice));
    vm.stopPrank();

    // alice got charged for a year of staking
    console.log("alice",asset.balanceOf(alice));
  }

Tools Used

manual audit, vs code, forge

Recommended Mitigation Steps

At every deposit, mint, redeem and withdraw, take fees before adding or removing the new users shares and assets.

[c4-sponsor]

RedVeil marked the issue as sponsor acknowledged

[c4-judge]

dmvt marked the issue as unsatisfactory: Overinflated severity