code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Description

Similar to front running performanceFee but slightly different.

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:    }

When fees are collected feesUpdatedAt is set. However, since a user can withdraw at any time, they can choose to withdraw just before the fees are collected. The users assets will then be out of the pool and not calculated as part of the fee collection.

After the fees are taken the user can re-deposit their assets to continue staking.

Impact

A staker can deny feeRecipient their managementFee by "sandwiching" the fee collection with a redeem and deposit. If the deposit/withdraw fee is low enough in comparison to managementFee this can be profitable for the staker.

Proof of Concept

PoC test in Vault.t.sol

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

    asset.mint(alice, 1e18);    

    vm.startPrank(alice);
    asset.approve(address(vault), 2e18);
    // user stakes
    vault.deposit(1e18);
    vm.stopPrank();

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

    console.log("alice shares before",vault.balanceOf(alice));

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

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

    vault.takeManagementAndPerformanceFees();

    // feeRecipient got nothing
    console.log("fee collected",vault.balanceOf(feeRecipient));

    // user can stake again directly after
    vm.prank(alice);
    vault.deposit(1e18);

    // with no change in shares
    console.log("alice shares after",vault.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 commented 1 year ago

RedVeil marked the issue as sponsor acknowledged

c4-judge commented 1 year ago

dmvt changed the severity to QA (Quality Assurance)