code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Description

Popcorn vaults have a variety of fees that can be configured. Once deployed, the owner of the vault has the ability to change the fees with a timelock, 3 days by default. When the timelock has passed anyone can apply the changes.

The issue is that the call to change the fees does not check that there are any proposed fees:

File: vault/Vault.sol

540:    function changeFees() external {
541:        if (block.timestamp < proposedFeeTime + quitPeriod)
542:            revert NotPassedQuitPeriod(quitPeriod);
543:
544:        emit ChangedFees(fees, proposedFees);
545:        fees = proposedFees;
546:    }

proposedFeeTime is initialized to 0 so proposedFeeTime + quitPeriod is 1970-01-03. proposedFees is also initialized to 0 hence calling changeFees after deploy, before any proposed fee changes are done, will set all fees to 0.

This can be fixed by proposing a new fee, but that has to wait quitPeriod (default 3 days) until it can be applied.

Impact

Anyone can set all fees to 0 after deploy. This can be fixed after quitPeriod which by default is 3 days but can be changed to 1 day. Until then fees will be 0.

Proof of Concept

PoC test in Vault.t.sol:

  function test__change_fees_after_deploy() public {
    // must jump forward a bit since test timestamp starts at 0
    vm.warp(block.timestamp + 3 days + 1);

    // create a vault with fees
    address vaultAddress = address(new Vault());
    Vault _vault = Vault(vaultAddress);
    _vault.initialize(
      IERC20(address(asset)),
      IERC4626(address(adapter)),
      VaultFees({ deposit: 1e17, withdrawal: 1e17, management: 1e17, performance: 1e17 }),
      feeRecipient,
      address(this)
    );

    // fees before
    (uint64 deposit, uint64 withdrawal, uint64 management, uint64 performance) = _vault.fees();
    assertEq(1e17,deposit);
    assertEq(1e17,withdrawal);
    assertEq(1e17,management);
    assertEq(1e17,performance);

    // anyone can call change fees and set them to 0
    vm.prank(alice);
    _vault.changeFees();

    // all fees are set to 0 after
    (deposit, withdrawal, management, performance) = _vault.fees();
    assertEq(0,deposit);
    assertEq(0,withdrawal);
    assertEq(0,management);
    assertEq(0,performance);
  }

Tools Used

manual audit, vs code, forge

Recommended Mitigation Steps

Check that propsedFeeTime != 0 and set it to 0 after fees are changed:

diff --git a/src/vault/Vault.sol b/src/vault/Vault.sol
index 7a8f941..d7fc47d 100644
--- a/src/vault/Vault.sol
+++ b/src/vault/Vault.sol
@@ -538,11 +538,15 @@ contract Vault is

     /// @notice Change fees to the previously proposed fees after the quit period has passed.
     function changeFees() external {
+        if (proposedFeeTime == 0)
+            revert("No proposed fees"); // replace with a proper error
+
         if (block.timestamp < proposedFeeTime + quitPeriod)
             revert NotPassedQuitPeriod(quitPeriod);

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

     /**

c4-judge commented 1 year ago

dmvt marked the issue as duplicate of #78

c4-sponsor commented 1 year ago

RedVeil marked the issue as sponsor confirmed