Popcorn contest - bin2chen's results

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/beefy/BeefyAdapter.sol#L61-L65

Vulnerability details

Impact

malicious vault owner can use Malicious _beefyBooster to steal the adapter's token

Proof of Concept

When creating a BeefyAdapter, the vault owner can specify the _beefyBooster The current implementation does not check if the _beefyBooster is legitimate or not, and worse, it _beefyVault.approve to the _beefyBooster during initialization The code is as follows:

contract BeefyAdapter is AdapterBase, WithRewards {
...
    function initialize(
        bytes memory adapterInitData,
        address registry,
        bytes memory beefyInitData
    ) external initializer {
       
        (address _beefyVault, address _beefyBooster) = abi.decode(
            beefyInitData,   //@audit <--------- beefyInitData comes from the owner's input: adapterData.data
            (address, address)
        );


        //@audit <-------- not check _beefyBooster is legal
        if (
            _beefyBooster != address(0) &&
            IBeefyBooster(_beefyBooster).stakedToken() != _beefyVault  
        ) revert InvalidBeefyBooster(_beefyBooster);   

...
      
        if (_beefyBooster != address(0))
            IERC20(_beefyVault).approve(_beefyBooster, type(uint256).max);     //@audit <---------  _beefyVault approve _beefyBooster    

}

    function _protocolDeposit(uint256 amount, uint256)
        internal
        virtual
        override
    {
        beefyVault.deposit(amount);
        if (address(beefyBooster) != address(0)) 
            beefyBooster.stake(beefyVault.balanceOf(address(this)));  //@audit <--------- A malicious beefyBooster can transfer the token
    }

As a result, a malicious user can pass a malicious _beefyBooster contract, and when the user deposits to the vault, the vault is saved to the _beefyVault, This malicious _beefyBooster can execute _beefyVault.transferFrom(BeefyAdapter), and take all the tokens stored by the adapter to _beefyVault

Tools Used

Recommended Mitigation Steps

Check _beefyBooster just like check _beefyVault

    function initialize(
        bytes memory adapterInitData,
        address registry,
        bytes memory beefyInitData
    ) external initializer {
...
        if (!IPermissionRegistry(registry).endorsed(_beefyVault))
            revert NotEndorsed(_beefyVault);
...            
+       if (!IPermissionRegistry(registry).endorsed(_beefyBooster))
+           revert NotEndorsed(_beefyBooster);

        if (
            _beefyBooster != address(0) &&
            IBeefyBooster(_beefyBooster).stakedToken() != _beefyVault
        ) revert InvalidBeefyBooster(_beefyBooster);

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L695 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L635-L637

Vulnerability details

Impact

Malicious users can pause/unpause other people's vault Causes vault to not work properly

Proof of Concept

deployVault() is used to deploy a new vault This method supports passing an adapter address as the adapter of the vault It checks whether the adapter is legal, and the rules are as follows:

  function _verifyAdapterConfiguration(address adapter, bytes32 adapterId) internal view {
    if (adapter != address(0)) {
      if (adapterId > 0) revert AdapterConfigFaulty();
     
      if (!cloneRegistry.cloneExists(adapter)) revert AdapterConfigFaulty();  //@audit <------ only check cloneExists, So if you give an other people's vault  address , it's ok
    }
  }

contract Vault is
    ERC20Upgradeable,
    ReentrancyGuardUpgradeable,
    PausableUpgradeable,
    OwnedUpgradeable
{
    function initialize(
        IERC20 asset_,
        IERC4626 adapter_,
        VaultFees calldata fees_,
        address feeRecipient_,
        address owner
    ) external initializer {
...
        if (address(asset_) != adapter_.asset()) revert InvalidAdapter();   //@audit <------- only check adapter_.asset()

        adapter = adapter_;    

1._verifyAdapterConfiguration() just check cloneRegistry.cloneExists(adapter)), if we give another people's vault address,it's ok 2.Vault.initialize() just check if (address(asset_) != adapter_.asset()) revert InvalidAdapter();

so if we give another people's vault address (this vault's asset() must equal to new vault.asset()) new vault will create success

at this time adapter == another people's vault address when we call pauseAdapters(newVault),will pause other people's vault because vault also have an unpause() function

  function unpauseAdapters(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(),   //@audit <-------  adapter is actually another people's vault address
        abi.encodeWithSelector(IPausable.unpause.selector)
      );
      if (!success) revert UnderlyingError(returnData);
    }
  }

Tools Used

Recommended Mitigation Steps

Many parts of the protocol just restrict cloneRegistry.cloneExists(), but don't distinguish if it's the right type or even if you created the adapter yourself Suggest adding the determination of clone type and creator

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L667-L670

Vulnerability details

Impact

_verifyCreatorOrOwner() Conditional logic error resulting cannot pause vault, which may result in not being able to stop the malicious withdraw() due to bugs, thus lose funds

Proof of Concept

_verifyCreatorOrOwner() is used to check if msg.sender is the creator of the vault or the administrator (owner of the VaultController)

The implementation code is as follows:

  function _verifyCreatorOrOwner(address vault) internal view returns (VaultMetadata memory metadata) {
    metadata = vaultRegistry.getVault(vault);
    //@audit correct: msg.sender != metadata.creator && msg.sender != owner?
    if (msg.sender != metadata.creator || msg.sender != owner) revert NotSubmitterNorOwner(msg.sender);
  }

There is a spelling error here using if (msg.sender ! = metadata.creator || msg.sender ! = owner) revert NotSubmitterNorOwner(msg.sender); This results in that only one case is legal: (msg.sender == metadata.creator == VaultController.owner) But the normal case metadata.creator will not equal VaultController.owner, resulting in no one being able to pass the verification (note: VaultController.owner will not pass either)

The following method will use _verifyCreatorOrOwner() for legitimacy verification.

1.addStakingRewardsTokens() 2.pauseAdapters() 3.unpauseAdapters() 3.pauseVaults() 4.unpauseVaults()

This results in the user not being able to call the above 5 methods, especially pauseVaults(), which is an important operation and cannot pause vault, which may result in not being able to stop the malicious withdraw() due to bugs, thus lose funds

Tools Used

Recommended Mitigation Steps

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

  }

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L473-L499

Vulnerability details

Impact

Wrong call time of takeFees() leads to wrong totalFee

Proof of Concept

Vault.takeFees() is used to get the totalFee and transfer it to feeRecipient

Contains the managementFee:

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

PerformanceFee:

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

From the implementation, if totalAssets and totalSupply have changed, both need to calculate the fees right away. But currently in vault.deposit()/mint()/withdraw()/redeem() is not called immediately, need to manually call takeManagementAndPerformanceFees() This leads to the call to takeManagementAndPerformanceFees() when the totalAssets and totalSupply may have changed, so the totalFee is not correct

Tools Used

Recommended Mitigation Steps

Before the execution of deposit()/mint()/withdraw()/redeem(), managementFee and PerformanceFee should be accumulated in real time. then takeManagementAndPerformanceFee() to retrieve the accumulated fees, and then clear them after retrieval.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/DeploymentController.sol#L121-L125

Vulnerability details

Impact

Unable to switch to a new deploymentController

Proof of Concept

The current protocol supports the replacement of the new DeploymentController, which can be switched by VaultController.setDeploymentController()

Normally, when switching, the owner of the cloneFactory/cloneRegistry/templateRegistry in the old DeploymentController should also be switched to the new DeploymentController DeploymentController's nominateNewDependencyOwner() implementation is as follows:

  /**
   * @notice Nominates a new owner for dependency contracts. Caller must be owner. (`VaultController` via `AdminProxy`)
   * @param _owner The new `DeploymentController` implementation
   */
  function nominateNewDependencyOwner(address _owner) external onlyOwner {
    IOwned(address(cloneFactory)).nominateNewOwner(_owner);
    IOwned(address(cloneRegistry)).nominateNewOwner(_owner);
    IOwned(address(templateRegistry)).nominateNewOwner(_owner);
  }

But there is a problem here, VaultConttroler.sol does not implement the code to call old_Deployerment.nominateNewDependencyOwner(), resulting in DeploymentController can not switch properly, nominateNewDependencyOwner()'s Remarks: Caller must be owner. (`VaultController` via `AdminProxy`) But in fact the VaultController does not have any code to call the nominateNewDependencyOwner

  function setDeploymentController(IDeploymentController _deploymentController) external onlyOwner {
    _setDeploymentController(_deploymentController);
  }

  function _setDeploymentController(IDeploymentController _deploymentController) internal {
    if (address(_deploymentController) == address(0) || address(deploymentController) == address(_deploymentController))
      revert InvalidDeploymentController(address(_deploymentController));

    emit DeploymentControllerChanged(address(deploymentController), address(_deploymentController));

    deploymentController = _deploymentController;
    cloneRegistry = _deploymentController.cloneRegistry();
    templateRegistry = _deploymentController.templateRegistry();
  }

Tools Used

Recommended Mitigation Steps

setDeploymentController() need call nominateNewDependencyOwner()

contract VaultController is Owned {
  function setDeploymentController(IDeploymentController _deploymentController) external onlyOwner {
    
+    //1. old deploymentController nominateNewDependencyOwner
+    (bool success, bytes memory returnData) = adminProxy.execute(
+       address(deploymentController),
+       abi.encodeWithSelector(
+         IDeploymentController.nominateNewDependencyOwner.selector,
+         _deploymentController
+       )
+     );
+     if (!success) revert UnderlyingError(returnData); 
+
+    //2. new deploymentController acceptDependencyOwnership
+    _deploymentController.acceptDependencyOwnership(); 

     _setDeploymentController(_deploymentController);        
  }

}

Lines of code

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

Vulnerability details

Impact

The harvestCooldown mechanism fails , use can call harvest() without time interval limit

Proof of Concept

AdapterBase.harvest() is used to execute the operation of claim etc. The normal method will have a time interval limit, currently is through harvestCooldown, in this time interval, can not be executed again

harvest() is implemented as follows:

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

        emit Harvested();
    }

The current implementation has a problem that after harvest(), it does not modify lastHarvest, resulting in a complete failure of the harvestCooldown mechanism, which can be repeatedly executed without time interval restrictions

Tools Used

Recommended Mitigation Steps

update lastHarvest to block.timestamp

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

        emit Harvested();
    }

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L253-L257

Vulnerability details

Impact

after redeem(), highWaterMark still old, accruedPerformanceFee will wrong

Proof of Concept

The normal logic is that Vault performs syncFeeCheckpoint before deposit/mint/withdraw/redeem The syncFeeCheckpoint will update the highWaterMark = convertToAssets(1e18);

highWaterMark is used in accruedPerformanceFee() to calculate performanceFee and transfer it to feeRecipient

But currently only deposit/mint/withdraw has, redeem() does not add modifier syncFeeCheckpoint resulting in accruedPerformanceFee keeps using the old one, performanceFee will be calculated incorrectly

    function redeem(
        uint256 shares,
        address receiver,
        address owner
    ) public nonReentrant returns (uint256 assets) {   //<------------without syncFeeCheckpoint
  

Tools Used

Recommended Mitigation Steps

redeem() add syncFeeCheckpoint

    function redeem(
        uint256 shares,
        address receiver,
        address owner
-   ) public nonReentrant returns (uint256 assets) {
+   ) public nonReentrant syncFeeCheckpoint returns (uint256 assets) {

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L480-L494

Vulnerability details

Impact

in takeFees() ,totalFee calculation wrong

Proof of Concept

takeFees() will calculate ManagementFee and PerformanceFee, and then combine them to transfer to feeRecipient

ManagementFee is the value of asset, but PerformanceFee is the value of shares.

The code is as follows:

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


    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(),  //<-------- use totalSupply
                    1e36,
                    Math.Rounding.Down
                )
                : 0;
    }    

and

    modifier takeFees() {
        uint256 totalFee = accruedManagementFee() + accruedPerformanceFee();
..        

        if (totalFee > 0 && currentAssets > 0)
            _mint(feeRecipient, convertToShares(totalFee));                 

As mentioned above, this is an incorrect summation of data types

suggest accruedPerformanceFee() convert to assets

Tools Used

Recommended Mitigation Steps

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

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

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L170 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L211

Vulnerability details

Impact

mint and withdraw using round down does not conform to the eip-4626 specification, which also leads to insufficient asset due to the accuracy of the relationship

Proof of Concept

Per EIP 4626’s Security Considerations (https://eips.ethereum.org/EIPS/eip-4626)

Finally, ERC-4626 Vault implementers should be aware of the need for specific, opposing rounding directions across the different mutable and view methods, as it is considered most secure to favor the Vault itself during calculations over its users:

If (1) it’s calculating how many shares to issue to a user for a certain amount of the underlying tokens they provide or (2) it’s determining the amount of the underlying tokens to transfer to them for returning a certain amount of shares, it should round down. If (1) it’s calculating the amount of shares a user has to supply to receive a given amount of the underlying tokens or (2) it’s calculating the amount of underlying tokens a user has to provide to receive a certain amount of shares, it should round up. Thus, the result of the previewMint and previewWithdraw should be rounded up.

The current implementation of convertToShares function will round down the number of shares returned due to how solidity handles Integer Division.

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

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

in mint/withdraw() call convertToAssets() use Rounding.Down

    function mint(uint256 shares, address receiver)
        public
        nonReentrant
        whenNotPaused
        syncFeeCheckpoint
        returns (uint256 assets)
    {
...
        if (receiver == address(0)) revert InvalidReceiver();

        uint256 depositFee = uint256(fees.deposit);

        uint256 feeShares = shares.mulDiv(
            depositFee,
            1e18 - depositFee,
            Math.Rounding.Down
        );

        //@audit **** this place will round down
        assets = convertToAssets(shares + feeShares);



    function withdraw(
        uint256 assets,
        address receiver,
        address owner
    ) public nonReentrant syncFeeCheckpoint returns (uint256 shares) {
        if (receiver == address(0)) revert InvalidReceiver();

        //@audit **** this place will round down
        shares = convertToShares(assets);        

Tools Used

Recommended Mitigation Steps

Following is the OpenZeppelin’s vault implementation for rounding reference:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/ERC20TokenizedVault.sol

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/abstracts/AdapterBase.sol#L479-L483

Vulnerability details

Impact

_verifyAndSetupStrategy() does not work properly

Proof of Concept

Adapter can optionally configure strategy, configure _strategy is will execute _verifyAndSetupStrategy() for verification The code is as follows:

    /**
     * @notice Verifies that the Adapter and Strategy are compatible and sets up the strategy.
     * @dev This checks EIP165 compatibility and potentially other strategy specific checks (matching assets...).
     * @dev It aftwards sets up anything required by the strategy to call `harvest()` like approvals etc.
     */
    function _verifyAndSetupStrategy(bytes4[8] memory requiredSigs) internal {
        strategy.verifyAdapterSelectorCompatibility(requiredSigs);
        strategy.verifyAdapterCompatibility(strategyConfig);
        strategy.setUp(strategyConfig);
    }

The corresponding method verifyAdapterSelectorCompatibility() is implemented in Pool2SingleAssetCompounder:

  function verifyAdapterCompatibility(bytes memory data) public override {
    address router = abi.decode(data, (address));   
    address asset = IAdapter(address(this)).asset();   //@audit  <-----------this place use address(this)

    // Verify Trade Path exists
    address[] memory tradePath = new address[](2);
    tradePath[1] = asset;

    address[] memory rewardTokens = IWithRewards(address(this)).rewardTokens();
    uint256 len = rewardTokens.length;
    for (uint256 i = 0; i < len; i++) {
      tradePath[0] = rewardTokens[i];

      uint256[] memory amountsOut = IUniswapRouterV2(router).getAmountsOut(ERC20(asset).decimals() ** 10, tradePath);
      if (amountsOut[amountsOut.length] == 0) revert NoValidTradePath();
    }
  }

_verifyAndSetupStrategy Verifies that the Adapter and Strategy are compatible and sets up the strategy

There is a problem with the current _verifyAndSetupStrategy implementation: the strategy contract is called directly, the correct method should be executed with delegatecall

Because:

1. harvest() is also used in delegatecall
2. the current Pool2SingleAssetCompounder/StrategyBase are using address(this)

If strategy do not use delegatecall, then Pool2SingleAssetCompounder/StrategyBase need to modify address(this) to address(msg.sender)

Tools Used

Recommended Mitigation Steps

    function _verifyAndSetupStrategy(bytes4[8] memory requiredSigs) internal {
-       strategy.verifyAdapterSelectorCompatibility(requiredSigs);
-       strategy.verifyAdapterCompatibility(strategyConfig);
-       strategy.setUp(strategyConfig);
+       require(address(strategy).delegatecall(
+              abi.encodeWithSelector(IStrategy.verifyAdapterSelectorCompatibility,requiredSigs);
+          ));
+        require(address(strategy).delegatecall(
+              abi.encodeWithSelector(IStrategy.verifyAdapterCompatibility,strategyConfig);
+          ));     
+        require(address(strategy).delegatecall(
+              abi.encodeWithSelector(IStrategy.setUp,strategyConfig);
+          ));                  
    }

Lines of code

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

Vulnerability details

Impact

Gave the reward to the newly deposited shares

Proof of Concept

Adapter gets the reward by executing harvest()

Currently, it is used in deposit()/mint()/withdraw()/redeem() Take deposit() for example, the code is as follows:

    function deposit(uint256 assets, address receiver)
        public
        virtual
        override
        returns (uint256)
    {
        if (assets > maxDeposit(receiver)) revert MaxError(assets);

        uint256 shares = _previewDeposit(assets);
        _deposit(_msgSender(), receiver, assets, shares);

        return shares;
    }

    function _deposit(
        address caller,
        address receiver,
        uint256 assets,
        uint256 shares
    ) internal nonReentrant virtual override {
        IERC20(asset()).safeTransferFrom(caller, address(this), assets);
        
        uint256 underlyingBalance_ = _underlyingBalance();
        _protocolDeposit(assets, shares);
        // Update the underlying balance to prevent inflation attacks
        underlyingBalance += _underlyingBalance() - underlyingBalance_;

        _mint(receiver, shares);
        
        
        harvest(); //@audit  <----------- after  _previewDeposit call harvest()

        emit Deposit(caller, receiver, assets, shares);
    }

The current implementation is to deposit first and then calculate reward by harvest() This is problematic, the current reward should belong to the previous user deposit, so it makes sense to calculate the reward first and then execute deposit()

Tools Used

Recommended Mitigation Steps

call harvest() first and then exceute deposit()/withdraw() logic

Lines of code

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

Vulnerability details

Impact

vault's FEES can be set to 0 by any malicious user, causing the vault's owner to lose fee

Proof of Concept

The vault will set the fees when it is created, including the type: deposit/withdrawal/management/performance

In the user deposit()/withdraw()/takeManagementAndPerformanceFees() will charge a corresponding fee

If the subsequent owner wants to adjust the Fees, he can modify the Fees of the vault by using vault.proposeFees() and changeFees()

The code is as follows:

    function proposeFees(VaultFees calldata newFees) external onlyOwner {
        if (
            newFees.deposit >= 1e18 ||
            newFees.withdrawal >= 1e18 ||
            newFees.management >= 1e18 ||
            newFees.performance >= 1e18
        ) revert InvalidVaultFees();

        proposedFees = newFees;
        proposedFeeTime = block.timestamp;

        emit NewFeesProposed(newFees, block.timestamp);
    }

    /// @notice Change fees to the previously proposed fees after the quit period has passed.
    function changeFees() external {
        if (block.timestamp < proposedFeeTime + quitPeriod)
            revert NotPassedQuitPeriod(quitPeriod);

        emit ChangedFees(fees, proposedFees);
        //@audit ***** Did not determine whether proposedFeeTime, proposedFees is empty
        fees = proposedFees;
    }

The current implementation of changeFees() has a problem: there is no check whether the owner has submitted proposed, i.e.: whether proposedFeeTime and proposedFees are empty, and worse yet, changeFees() can be called by anyone!

This leads to, assuming that the vault is created with the following settings:

fees = {deposit = 0.01,withdrawal = 0.01 , management = 0.01 ,performance = 0.01}

at this time proposedFeeTime , proposedFees is default:

proposedFeeTime = 0 , proposedFees = {deposit = 0,withdrawal = 0, management = 0,performance = 0}

Subsequent malicious user calls changeFees(),changeFees() is executable because: block.timestamp > proposedFeeTime(=0) + quitPeriod(=3 days)

So fees is modified to:

fees = {deposit = 0,withdrawal = 0, management = 0,performance = 0}

This way vault's fees are cancelled by any malicious users

Although the owner can change it back, it is possible that the user did not find out, or found out but need to wait for 3 days to take effect, the damage has been caused

Tools Used

Recommended Mitigation Steps

changeFees add check proposedFeeTime

    function changeFees() external {
+       require(proposedFeeTime!=0,"not proposed");
        if (block.timestamp < proposedFeeTime + quitPeriod)
            revert NotPassedQuitPeriod(quitPeriod);

        emit ChangedFees(fees, proposedFees);
        fees = proposedFees;
+       delete proposedFees;
+       delete proposedFeeTime;
    }