Popcorn contest - rvierdiiev's results

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/beefy/BeefyAdapter.sol#L46-L84

Vulnerability details

Impact

Malicious beefy booster can be provided to BeefyAdapter that can steal funds, as there is no check that it's endorsed in registry.

Proof of Concept

When user deploys BeefyAdapter he can provide beefyVault and beefyBooster addresses that will be used by contract.

Later beefyVault is checked by template registry in order if it's endorsed. This is done to prevent providing malicious contract that can steal funds.

However, beefyBooster is not checked and this allows user to deploy adapter with malicious contract as beefyBooster which can steal user funds. And once beefyBooster is provided then all moo token that are controlled by adapter after deposit to vault will be send to malicious beefyBooster contract.

Tools Used

VsCode

Recommended Mitigation Steps

Add check for the beefyBooster contract if it's endorsed.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L170-L188 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L413-L432

Vulnerability details

Impact

User can drain all erc777 token balance of MultiRewardStaking using reentrancy in claimRewards function.

Proof of Concept

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L170-L188

  function claimRewards(address user, IERC20[] memory _rewardTokens) external accrueRewards(msg.sender, user) {
    for (uint8 i; i < _rewardTokens.length; i++) {
      uint256 rewardAmount = accruedRewards[user][_rewardTokens[i]];


      if (rewardAmount == 0) revert ZeroRewards(_rewardTokens[i]);


      EscrowInfo memory escrowInfo = escrowInfos[_rewardTokens[i]];


      if (escrowInfo.escrowPercentage > 0) {
        _lockToken(user, _rewardTokens[i], rewardAmount, escrowInfo);
        emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, true);
      } else {
        _rewardTokens[i].transfer(user, rewardAmount);
        emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, false);
      }


      accruedRewards[user][_rewardTokens[i]] = 0;
    }
  }

This function allow any user to receive collected rewards. It first call accrueRewards modifier, which will update user's reward amount. Then it's looping by each reward token and sends amount to the caller in case when no escrow is used.

The problem is that accruedRewards[user][_rewardTokens[i]] amount is reset only in the end of loop. That allows user to drain all balance of reward token by reentrancy attack in case if erc777 token is used as reward token.

Example. 1.Reward token is erc777 token. 2.User calls claimRewards from the contract which has hook. 3.In that hook user calls claimRewards again and again while MultiRewardStaking contract transfers all balance to attacker. 4.As result attacker received not his portion of rewards only, but all balance of MultiRewardStaking contract.

Tools Used

VsCode

Recommended Mitigation Steps

Update rewards amount at the beginning of function.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L605-L615 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L575

Vulnerability details

Impact

Anyone who uses same adapter have ability to pause it. As result you have ability to pause any vault by creating your vault with same adapter.

When user creates vault he has ability to deploy new adapter or reuse already created adapter.

VaultController gives ability to pause adapter. https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L605-L615

  function pauseAdapters(address[] calldata vaults) external {
    uint8 len = uint8(vaults.length);
    for (uint256 i = 0; i < len; i++) {
      _verifyCreatorOrOwner(vaults[i]);
      (bool success, bytes memory returnData) = adminProxy.execute(
        IVault(vaults[i]).adapter(),
        abi.encodeWithSelector(IPausable.pause.selector)
      );
      if (!success) revert UnderlyingError(returnData);
    }
  }

As you can see _verifyCreatorOrOwner is used to determine if msg.sender can pause adapter. https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L667-L670

  function _verifyCreatorOrOwner(address vault) internal returns (VaultMetadata memory metadata) {
    metadata = vaultRegistry.getVault(vault);
    if (msg.sender != metadata.creator || msg.sender != owner) revert NotSubmitterNorOwner(msg.sender);
  }

So in case if you are creator of vault that uses adaptor that you want to pause, then you are able to pause it.

This is how it can be used in order to stop the vault, i don't like. 1.Someone created vault that uses adapterA. 2.Attacker creates own vault and set adapterA as well. 3.Now attacker is able to pause adapterA and as result it's not possible to deposit anymore. Also vault is not earning fees now, as pausing withdraws all from strategy. 4.And it can pause it as many times as he wants(in case if someone else will try to unpause it).

So this attack allows to stop all vaults that use same adapter from earning yields.

Tools Used

VsCode

Recommended Mitigation Steps

I think that it's better to create a clone of adapter for the vault, so each vault has separate adaptor.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L138 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L215 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L480-L499 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L447-L460

Vulnerability details

Impact

Protocol loses fees because highWaterMark is updated every time someone deposit, withdraw, mint.

Proof of Concept

This bug is related to the fees accruing design. It was disscussed with the sponsor in order to understand how it should work.

Protocol has such thing as performance fee. Actually this is fee from accrued yields. If user deposited X assets and after some time he can withdraw X+Y assets for that minted amount of shares, that means that startegy has earned some Y amount of yields. Then protocol is able to get some part of that Y amount as a performance fee.

takeFees modifier is responsible for taking fees. It calls accruedPerformanceFee function to calculate fees amount. https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L447-L460

    function accruedPerformanceFee() public view returns (uint256) {
        uint256 highWaterMark_ = highWaterMark;
        uint256 shareValue = convertToAssets(1e18);
        uint256 performanceFee = fees.performance;


        return
            performanceFee > 0 && shareValue > highWaterMark
                ? performanceFee.mulDiv(
                    (shareValue - highWaterMark) * totalSupply(),
                    1e36,
                    Math.Rounding.Down
                )
                : 0;
    }

As you can see, protocol has such variable as highWaterMark. This variable actually should store convertToAssets(1e18) amount at the time when last fee were accrued or after first deposit. Then after some time when strategy earned some yields, convertToAssets(1e18) will return more assets than highWaterMark, so protocol will take fees.

But currently updating of highWaterMark is done incorrectly. Deposit, mint, withdraw function use syncFeeCheckpoint modifier. https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L496-L499

    modifier syncFeeCheckpoint() {
        _;
        highWaterMark = convertToAssets(1e18);
    }

This modifier will update highWaterMark to current assets amount that you can receive for 1e18 of shares. That means that every time when deposit, mint, withdraw is called, highWaterMark is updated to the new state, so protocol doesn't track yield progress anymore.

In case if protocol accrued some performance fees, which can be possible if noone called deposit, withdraw, mint for a long time, then anyone can frontrun takeFees and deposit small amount of assets in order to update highWaterMark, so protocol will not get any fees.

Tools Used

VsCode

Recommended Mitigation Steps

I believe that you need to store highWaterMark = convertToAssets(1e18) at the time of first deposit, or when totalShares is 0, this will be the value that protocol started with and then at time, when takefee was called you can calculate current convertToAssets(1e18) in case if it's bigger, than previous stored, then you can mint fees for protocol and update highWaterMark to current value.

Lines of code

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

Vulnerability details

Impact

Management fees should be accrued on any user action with Vault. Currently some users can avoid management fee payment, while another can pay fee for what they didn't use.

Proof of Concept

Protocol takes management fee for using of Vault. https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L429-L439

    function accruedManagementFee() public view returns (uint256) {
        uint256 managementFee = fees.management;
        return
            managementFee > 0
                ? managementFee.mulDiv(
                    totalAssets() * (block.timestamp - feesUpdatedAt),
                    SECONDS_PER_YEAR,
                    Math.Rounding.Down
                ) / 1e18
                : 0;
    }

This fee is calculated as percent from total assets multiplied by time elapsed from last fee colecting. Currently to collect fee, someone can call takeManagementAndPerformanceFees function.

This is incorrect design as it can make such situations. 1.Fee was taken last time at T1, so feesUpdatedAt = T1. At T100 user deposits big amount of assets. Then at T101 takeManagementAndPerformanceFees function is called, which sliced some amount of fees from depositor, however he didn't use that vault yet, he just deposited. 2.Now similar situation. Fee was taken last time at T1, so feesUpdatedAt = T1. At T2 user deposits big amount of assets. At T100, user withdraws that amount and pays no fee. At T101 takeManagementAndPerformanceFees function is called and collects fee, but user already left pool without payment.

Tools Used

VsCode

Recommended Mitigation Steps

I think that takeFees modifier should be set to deposit, withdraw, mint, redeem functions, so you collect management fee according to the user's usage of vault.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L438-L450

Vulnerability details

Impact

AdapterBase.lastHarvest is not updated inside harvest function.

Proof of Concept

harvest function allows strategy to collect yields. It's designed by protocol to be called at most 1 time is a harvestCooldown period. https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L438-L450

    function harvest() public takeFees {
        if (
            address(strategy) != address(0) &&
            ((lastHarvest + harvestCooldown) < block.timestamp)
        ) {
            // solhint-disable
            address(strategy).delegatecall(
                abi.encodeWithSignature("harvest()")
            );
        }


        emit Harvested();
    }

But because, lastHarvest is never updated in the contract, this function will allow strategy to harvest anytime after first harvestCooldown is passed.

Tools Used

VsCode

Recommended Mitigation Steps

Add lastHarvest = block.timestamp at the end of function.

Lines of code

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

Vulnerability details

Impact

Vault.accruedManagementFee starts accruing interests even when no assets in the vault. User pay more fees because of that.

Proof of Concept

accruedManagementFee function calculates amount of fees for management. https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L429-L439

    function accruedManagementFee() public view returns (uint256) {
        uint256 managementFee = fees.management;
        return
            managementFee > 0
                ? managementFee.mulDiv(
                    totalAssets() * (block.timestamp - feesUpdatedAt),
                    SECONDS_PER_YEAR,
                    Math.Rounding.Down
                ) / 1e18
                : 0;
    }

As you can see it uses feesUpdatedAt variable in order to calculate amount of seconds that has elapsed, since last update. When Vault is initialized, this variable is set to block.timestamp. That actually means that Vault starts accruing management fee at the deploying time, when actually noone using it yet.

Because of that such situation is possible. 1.Vault is created. 2.No one use it for very long time(let's say half of year, just as example) 3.User deposits some big amount of funds. 4.Someone calls takeManagementAndPerformanceFees which takes huge part as fee, so depositor actually lost half of his funds

Tools Used

VsCode

Recommended Mitigation Steps

Initialize feesUpdatedAt when first deposit was made.

Lines of code

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

Vulnerability details

Impact

Vault.withdraw doesn't rounds up burnt shares amount and burns less shares of user, then it should.

Proof of Concept

When user wants to withdraw some amount of assets, he calls Vault.withdraw function. According to the amount of assets, that user wants to receive that function calculates shares amount that need to be burnt from user balance.

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

    function convertToShares(uint256 assets) public view returns (uint256) {
        uint256 supply = totalSupply(); // Saves an extra SLOAD if totalSupply is non-zero.


        return
            supply == 0
                ? assets
                : assets.mulDiv(supply, totalAssets(), Math.Rounding.Down);
    }

It uses convertToShares function which rounds down shares amount, however it should round up in this case.

Tools Used

VsCode

Recommended Mitigation Steps

You need to round up shares amount.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L158 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L456-L472

Vulnerability details

Impact

Strategy can't earn yields for user as underlyingBalance is not updated when strategy deposits.

Proof of Concept

When someone deposits/withdraws from adapter, then underlyingBalance variable is updated with deposited/withdrawn to the vault shares amount. Only when user deposits or withdraws, then AdapterBase changes totalSupply(it mints or burns shares).

This is how shares of users are calculated inside BeffyAdapter https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/beefy/BeefyAdapter.sol#L122-L133

    function convertToUnderlyingShares(uint256, uint256 shares)
        public
        view
        override
        returns (uint256)
    {
        uint256 supply = totalSupply();
        return
            supply == 0
                ? shares
                : shares.mulDiv(underlyingBalance, supply, Math.Rounding.Up);
    }

As you can see, when user provides amount of shares that he wants to withdraw, then these shares are recalculated in order to receive shares amount inside vault. This depends on underlyingBalance and totalSupply.

Each adapter can have a strategy that can withdraw harvest and then redeposit it inside the vault. In this case users should earn new shares. When adapter wants to deposit to vault it should call strategyDeposit function. https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L456-L461

    function strategyDeposit(uint256 amount, uint256 shares)
        public
        onlyStrategy
    {
        _protocolDeposit(amount, shares);
    }

This function is just sending all amount to the vault. But actually it should also increase underlyingBalance with shares amount that it will receive by depositing. Because this is not happening, underlyingBalance always equal to totalSupply and users do not earn any yields using strategy as convertToUnderlyingShares function will just return same value as provided shares. So currently users can't earn any yields using strategy.

Note: this was discussed with protocol developer and he explained me how it should work.

Tools Used

VsCode

Recommended Mitigation Steps

Increase underlyingBalance with shares amount that it will receive by depositing. But do not mint shares)

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L438-L450 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L162 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L232

Vulnerability details

Impact

AdpaterBase.harvest should be called before deposit and withdraw.

Proof of Concept

Function harvest is called in order to receive yields for the adapter. It calls strategy, which handles that process. Depending on strategy it can call strategyDeposit function in order to deposit earned amount through the adaptor. That actually means that in case if totalAssets was X before harvest call, then after it becomes X+Y, in case if Y yields were earned by adapter and strategy deposited it. So for the same amount of shares, user can receive bigger amount of assets.

When user deposits or withdraw then harvest function is called, but it's called after shares amount calculation. Because of that in case of deposit, all previous depositors lose some part of yields as they share it with new depositor. And in case of withdraw, user loses his yields.

Tools Used

VsCode

Recommended Mitigation Steps

Call harvest before shares amount calculation.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L170-L202

Vulnerability details

Impact

MultiRewardStaking.claimRewards will fail when reward token doesn't allow 0 tranfers and escrow percentage is 100%.

Proof of Concept

MultiRewardStaking.claimRewards will transfer rewards for toker to the sender. In case if escrow is used, then some percentage of rewards will be locked.

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L191-L202

  function _lockToken(
    address user,
    IERC20 rewardToken,
    uint256 rewardAmount,
    EscrowInfo memory escrowInfo
  ) internal {
    uint256 escrowed = rewardAmount.mulDiv(uint256(escrowInfo.escrowPercentage), 1e18, Math.Rounding.Down);
    uint256 payout = rewardAmount - escrowed;


    rewardToken.safeTransfer(user, payout);
    escrow.lock(rewardToken, user, escrowed, escrowInfo.escrowDuration, escrowInfo.offset);
  }

In case if escrowPercentage is 100%, that means that payout will be 0. And if rewardToken is not supporting 0 value transfers, then function will revert and user will not be able to claim his rewards and send them to the escrow.

Tools Used

VsCode

Recommended Mitigation Steps

Add check that payout is bigger than 0.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L296-L315 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L351-L360

Vulnerability details

Impact

MultiRewardStaking.changeRewardSpeed calculates rewardsEndTimestamp incorrectly. Because of that contract supposes that it has more balance for rewarding, so some users will not receive rewards.

Proof of Concept

MultiRewardStaking.changeRewardSpeed allows owner to update speed of rewarding. https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L296-L315

  function changeRewardSpeed(IERC20 rewardToken, uint160 rewardsPerSecond) external onlyOwner {
    RewardInfo memory rewards = rewardInfos[rewardToken];


    if (rewardsPerSecond == 0) revert ZeroAmount();
    if (rewards.lastUpdatedTimestamp == 0) revert RewardTokenDoesntExist(rewardToken);
    if (rewards.rewardsPerSecond == 0) revert RewardsAreDynamic(rewardToken);


    _accrueRewards(rewardToken, _accrueStatic(rewards));


    uint256 remainder = rewardToken.balanceOf(address(this));


    uint32 prevEndTime = rewards.rewardsEndTimestamp;
    uint32 rewardsEndTimestamp = _calcRewardsEnd(
      prevEndTime > block.timestamp ? prevEndTime : block.timestamp.safeCastTo32(),
      rewardsPerSecond,
      remainder
    );
    rewardInfos[rewardToken].rewardsPerSecond = rewardsPerSecond;
    rewardInfos[rewardToken].rewardsEndTimestamp = rewardsEndTimestamp;
  }

The most interesting for us is this part.

 uint256 remainder = rewardToken.balanceOf(address(this));


    uint32 prevEndTime = rewards.rewardsEndTimestamp;
    uint32 rewardsEndTimestamp = _calcRewardsEnd(
      prevEndTime > block.timestamp ? prevEndTime : block.timestamp.safeCastTo32(),
      rewardsPerSecond,
      remainder
    );

Here function takes full balance of contract as remainder, previous time when rewards should be ended and new rewardsPerSecond. Then it passes it to _calcRewardsEnd function in order to calculate the amount of time, when this funds will be rewarded. This amount is calculated not correct. https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L351-L360

  function _calcRewardsEnd(
    uint32 rewardsEndTimestamp,
    uint160 rewardsPerSecond,
    uint256 amount
  ) internal returns (uint32) {
    if (rewardsEndTimestamp > block.timestamp)
      amount += uint256(rewardsPerSecond) * (rewardsEndTimestamp - block.timestamp);


    return (block.timestamp + (amount / uint256(rewardsPerSecond))).safeCastTo32();
  }

This function is working like that. In case if rewardsEndTimestamp > block.timestamp it calculates amount that will be distributed during the remaining time. Then it adds this amount to amount provided as parameter. This is not correct in our case for the reason that param amount is full balance of reward token. And there is no need to calculate additional amount. Such approach is used inside fundReward function, but is not working in this case.

As result contract adds additional amount to the full balance of contract and calculates time for distribution. Because this amount is bigger than contract's balance, time will be also bigger. But that time is not backed by reward tokens, so some users will not receive rewards, when they will be claiming them.

Scenario. 1.Owner adds new reward with some amount. 2.After some time (when rewarding is still in progress) he wants to change speed of rewards for the token. 3.After update rewardsEndTimestamp is bigger than amount of rewards that it should pay for that time.

Tools Used

VsCode

Recommended Mitigation Steps

In this case calculate rewardsEndTimestamp like this:

 uint32 rewardsEndTimestamp = _calcRewardsEnd(
      0,
      rewardsPerSecond,
      remainder
    );

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L371-L384

Vulnerability details

Impact

MultiRewardStaking will stop working when rewardTokens list will be huge enough because of out of gas.

Proof of Concept

When user deposits, withdraws, mints, redeems in MultiRewardStaking contract, then accrueRewards modifier is used which calculates rewards for user.

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L371-L384

  modifier accrueRewards(address _caller, address _receiver) {
    IERC20[] memory _rewardTokens = rewardTokens;
    for (uint8 i; i < _rewardTokens.length; i++) {
      IERC20 rewardToken = _rewardTokens[i];
      RewardInfo memory rewards = rewardInfos[rewardToken];


      if (rewards.rewardsPerSecond > 0) _accrueRewards(rewardToken, _accrueStatic(rewards));
      _accrueUser(_receiver, rewardToken);


      // If a deposit/withdraw operation gets called for another user we should accrue for both of them to avoid potential issues like in the Convex-Vulnerability
      if (_receiver != _caller) _accrueUser(_caller, rewardToken);
    }
    _;
  }

Pls, note that this loop uses uint8 i value in order to go through _rewardTokens array. The loop will end when i == _rewardTokens.length. Because i is uint8, then _rewardTokens.length will be also casted to uint8 and the max value will be 2^8. That means that once array of reward tokens will be bigger, than 2^8, then this new tokens will not be handled by contract and user will not receive rewards for them. Also it will be not possible for owner to withdraw that funds, so they will actually stuck inside contract.

2^8 = 256, which is not small value, but still it's possible that more reward tokens will be used. And there is no ability to remove reward token from list.

Tools Used

VsCode

Recommended Mitigation Steps

If you want to restrict that amount to 2^8, then do not allow to add next reward tokens, once that size is filled.

Lines of code

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

Vulnerability details

Impact

Anyone can reset fees to 0 value when Vault is deployed. As result protocol will stop collecting fees.

Proof of Concept

Anyone can call changeFees function in order to change fees variable to proposedFees. https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L540-L546

    function changeFees() external {
        if (block.timestamp < proposedFeeTime + quitPeriod)
            revert NotPassedQuitPeriod(quitPeriod);


        emit ChangedFees(fees, proposedFees);
        fees = proposedFees;
    }

There is a check that should not allow anyone to call function before quitPeriod has passed after fees changing was proposed. However function doesn't check that proposedFeeTime is not 0, so that means that after Vault has deployed, anyone can call this function the check will pass. That means that fees will be set to the proposedFees, which is 0. As result protocol will stop collecting fees.

Use this test inside Vault.t.sol. Here you can see that noone proposed fee changing, but it was changed and set fees to 0.

function test__changeFees() public {
    VaultFees memory newVaultFees = VaultFees({ deposit: 0, withdrawal: 0, management: 0, performance: 0 });
    //noone proposed
    //vault.proposeFees(newVaultFees);

    vm.warp(block.timestamp + 3 days);

    vm.expectEmit(false, false, false, true, address(vault));
    emit ChangedFees(VaultFees({ deposit: 0, withdrawal: 0, management: 0, performance: 0 }), newVaultFees);

    vault.changeFees();

    (uint256 deposit, uint256 withdrawal, uint256 management, uint256 performance) = vault.fees();
    assertEq(deposit, 0);
    assertEq(withdrawal, 0);
    assertEq(management, 0);
    assertEq(performance, 0);
  }

Tools Used

VsCode

Recommended Mitigation Steps

    function changeFees() external {
        if (proposedFeeTime == 0 || block.timestamp < proposedFeeTime + quitPeriod)
            revert NotPassedQuitPeriod(quitPeriod);


        emit ChangedFees(fees, proposedFees);
        fees = proposedFees;
    }