code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Description

managementFee is a fee that is taken on the TVL. performanceFee is a fee taken on the yield received. Both as a percentage of assets.

However, they are used directly to hand out shares:

File: Vault.sol

480:    modifier takeFees() {
481:        uint256 managementFee = accruedManagementFee();
482:        uint256 totalFee = managementFee + accruedPerformanceFee();

...

488:        if (managementFee > 0) feesUpdatedAt = block.timestamp;
489:
490:        if (totalFee > 0 && currentAssets > 0)
491:            _mint(feeRecipient, convertToShares(totalFee));
492:
493:        _;
494:    }

Since the share minting will lessen the value per share, feeRecipient will receive shares worth less in asset than what the fees are configured for.

Impact

feeRecipient will receive a less than expected managementFee and performanceFee.

Proof of Concept

PoC test in Vault.t.sol:

  function test__fee_calculation() public {
    _setFees(0, 0, 1e17, 1e17); // 10% fees

    // reset feesUpdatedAt
    vault.takeManagementAndPerformanceFees(); 

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

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

    asset.mint(address(adapter),1e18); // fake yield

    // fees are 10%
    console.log("managementFee, 10%% of assets",vault.accruedManagementFee());
    console.log("performanceFee, 10%% of price increase",vault.accruedPerformanceFee());

    vault.takeManagementAndPerformanceFees();

    console.log("shares minted for feeRecipient",vault.balanceOf(feeRecipient));

    // 10% of 2e18 = 2e17 (mgmt), 10% of 1e18 (yield) = 1e17 should give ~3e17 in fees
    console.log("assets received by feeRecipient",vault.convertToAssets(vault.balanceOf(feeRecipient)));
  }

Tools Used

manual audit, vs code, forge

Recommended Mitigation Steps

diff --git a/src/vault/Vault.sol b/src/vault/Vault.sol
index 7a8f941..1799a06 100644
--- a/src/vault/Vault.sol
+++ b/src/vault/Vault.sol
@ -488,7 +488,7 @@ contract Vault is
         if (managementFee > 0) feesUpdatedAt = block.timestamp;

         if (totalFee > 0 && currentAssets > 0)
-            _mint(feeRecipient, convertToShares(totalFee));
+            _mint(feeRecipient, totalFee.mulDiv(totalSupply(),currentAssets - totalFee,Math.Rounding.Up));

         _;
     }

Some explanation of the equation totalFee.mulDiv(totalSupply(),currentAssets - totalFee,Math.Rounding.Up):

you get the increase in shares you need from this equation, which is the end state you want to achieve:

$$fees = \Delta shares * \frac{assets}{shares + \Delta shares}$$

Which you can re-arrange to:

$$\Delta shares (assets - fees) = fees supply$$

which gives you the equation in the code above:

$$\Delta shares = fees * \frac{supply}{assets - fees}$$

c4-judge commented 1 year ago

dmvt marked the issue as duplicate of #491

c4-sponsor commented 1 year ago

RedVeil marked the issue as sponsor confirmed