Popcorn contest - ktg's results

Lines of code

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

Vulnerability details

Impact

Permitted user can't deploy vault because of wrong logic issues.

Proof of Concept

In contract VaultController, function deployVault will deploy a new vault; in the process, it will use function _verifyCreatorOrOwner to verify if the caller is the creator of the vault or owner of VaultController, as it stated in the comment:

  /// @notice Verify that the caller is the creator of the vault or owner of `VaultController` (admin rights).
  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);
  }

The problem is that the line (msg.sender != metadata.creator || msg.sender != owner) is not verifying if the caller is the creator of the vault or owner of VaultController but instead verify if the caller is both the creator of the vault and the owner of VaultController. This makes a normal user unable to deploy Vault since they are not the owner of VaultController This is a short explanation of the propositional logic in that:

 (msg.sender != metadata.creator || msg.sender != owner) == false
  => msg.sender != metadata.creator == false && msg.sender != owner == false
  => msg.sender == metadata.creator && msg.sender == owner

This is a POC for this issue, I just write the test case here without setup, so you need to add my test case to file VaultController.t.sol and run only my test case:

function test__user_cannot_deploy_vault() public {
        address user1 = address(0x01);
        addTemplate("Adapter", templateId, adapterImpl, true, true);
        addTemplate("Strategy", "MockStrategy", strategyImpl, false, true);
        addTemplate("Vault", "V1", vaultImpl, true, true);
        controller.setPerformanceFee(uint256(1000));
        controller.setHarvestCooldown(1 days);
        rewardToken.mint(address(this), 10 ether);
        rewardToken.approve(address(controller), 10 ether);

        swapTokenAddresses[0] = address(0x9999);
        address adapterClone = 0xD6C5fA22BBE89db86245e111044a880213b35705;
        address strategyClone = 0xe8a41C57AB0019c403D35e8D54f2921BaE21Ed66;
        address stakingClone = 0xE64C695617819cE724c1d35a37BCcFbF5586F752;

        uint256 callTimestamp = block.timestamp;
        vm.startPrank(user1);
        vm.expectRevert();
        address vaultClone  = controller.deployVault(
            VaultInitParams({
        asset: iAsset,
        adapter: IERC4626(address(0)),
        fees: VaultFees({
        deposit: 100,
        withdrawal: 200,
        management: 300,
        performance: 400
        }),
        feeRecipient: feeRecipient,
        owner: address(user1)
        }),
            DeploymentArgs({id: templateId, data: abi.encode(uint256(100))}),
            DeploymentArgs({id: "MockStrategy", data: ""}),
            address(0),
            abi.encode(
                address(rewardToken),
                0.1 ether,
                1 ether,
                true,
                10000000,
                2 days,
                1 days
            ),
            VaultMetadata({
        vault: address(0),
        staking: address(0),
        creator: address(user1),
        metadataCID: metadataCid,
        swapTokenAddresses: swapTokenAddresses,
        swapAddress: address(0x5555),
        exchange: uint256(1)
        }),
            0
        );
        vm.stopPrank();
    }

Command to run forge test --match-path test/vault/VaultController.t.sol --match-test test__user_cannot_deploy_vault -vvvv If you look at the end of the log, you will see the log (0x949DEa045FE979a11F0D4A929446F83072D81095, 0xE64C695617819cE724c1d35a37BCcFbF5586F752, 0x0000000000000000000000000000000000000001.... This is the result of function VaultRegistry::getVault, the 0x0000000000000000000000000000000000000001 is the address of user1 that submit the deploy. So the deployVault actually reverts with error NotSubmitterNorOwner even though user1 is the submitter.

Tools Used

Manual review.

Recommended Mitigation Steps

I recommend changing the logic to reflect the requirement Verify that the caller is the creator of the vault or owner of VaultController (admin rights).For example: (msg.sender != metadata.creator&&| msg.sender != owner)