Popcorn contest - ustas's results

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L666-L670 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L448 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L608 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L621 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L634 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L647

Vulnerability details

Impact

Modifier VaultController._verifyCreatorOrOwner does not work. Instead of checking the condition msg.sender is creator OR owner, it makes msg.sender is creator AND owner. This would block access to all created Vaults for creators and the owner (if he did not create them). Specifically, the following functions in the VaultController are affected:

• addStakingRewardsTokens();
• deployVault(), which has a call to addStakingRewardsTokens(), cannot be executed if the argument rewardsData.length != 0;
• pauseAdapters();
• pauseVaults();
• unpauseAdapters();
• unpauseVaults().

Proof of Concept

To check this concept, we can make a truth table for the main condition in the modifier msg.sender != metadata.creator || msg.sender != owner. The table shows that the condition will equal false only in the one case where msg.sender is both creator and owner.

The correct condition should be the following: msg.sender != metadata.creator && msg.sender != owner.

In this case, a revert will only happen when msg.sender is neither a creator nor the owner, as it should be according to the documentation.

You can also use the following test to check; add it to the file test\vault\VaultController.t.sol:

function testFail__deployVault_creator_is_not_owner_audit() public {
    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(bob, 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.prank(bob);
    address vaultClone = controller.deployVault(
        VaultInitParams({
            asset: iAsset,
            adapter: IERC4626(address(0)),
            fees: VaultFees({
                deposit: 100,
                withdrawal: 200,
                management: 300,
                performance: 400
            }),
            feeRecipient: feeRecipient,
            owner: bob
        }),
        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: bob,
            metadataCID: metadataCid,
            swapTokenAddresses: swapTokenAddresses,
            swapAddress: address(0x5555),
            exchange: uint256(1)
        }),
        0
    );
}

In the test's log (forge test --match-test "testFail__deployVault_creator_is_not_owner" -vvvv), you can see that the call ended with revert NotSubmitterNorOwner(0x000000000000000000000000000000000000000000000000DCbA).

Tools Used

Manual Review, VSCodium, Forge

Recommended Mitigation Steps

Change the condition to msg.sender != metadata.creator && msg.sender != owner.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L472-L477 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L479-L494 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L496-L499

Vulnerability details

Impact

The performanceFee logic in Vault allows users to evade it. To do this, recalculate highWaterMark before calling takeManagementAndPerformanceFees() which is responsible for issuing owner commissions.

Proof of Concept

Recalculation of highWaterMark is possible with a call to a function with syncFeeCheckpoint modifier:

• deposit()
• mint()
• withdraw()

In one of the scenarios, users can front-run every transaction to takeManagementAndPerformanceFees().

Use the following test to check; add it to the file test\vault\VaultController.t.sol:

function test__evadePerformanceFee_audit() public {
  uint256 assetAmount = 1 ether;
  uint256 depositAmount = 1 ether;

  _setFees(0, 0, 0, 1e17);

  // Deposit some funds to the vault
  asset.mint(alice, depositAmount + 1);
  vm.startPrank(alice);
  asset.approve(address(vault), depositAmount);
  vault.deposit(depositAmount, alice);
  vm.stopPrank();

  // highWaterMark is equal to standard value
  assertEq(vault.highWaterMark(), 1e18);
  assertEq(vault.accruedPerformanceFee(), 0);

  // Increase balance of the adapter to trigger performanceFee
  asset.mint(address(adapter), assetAmount);

  assertEq(vault.highWaterMark(), 1e18);
  assertEq(vault.accruedPerformanceFee(), 1e17);

  // Withdraw minimal amount to trigger syncFeeCheckpoint modifier
  vm.prank(alice);
  vault.withdraw(1);

  // highWaterMark is updated
  assertEq(vault.highWaterMark(), 2e18 - 1);
  // Fee is now equal to 0
  assertEq(vault.accruedPerformanceFee(), 0);
}

Tools Used

Manual Review, VSCodium, Forge

Recommended Mitigation Steps

Review of the fees logics.

Lines of code

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

Vulnerability details

Impact

The Vault contract contains two functions for swap shares for funds:

• withdraw(uint256 assets, ...)
• redeem(uint256 shares, ...)

It is assumed that the difference between these functions is only in the arguments. In withdraw(), we specify how many final assets we want to get, and in redeem(), we specify how many shares to burn. But redeem(), unlike withdraw(), has no syncFeeCheckpoint modifier, which allows the user to perform the same action (withdrawal) with different consequences, thus manipulating the performanceFee commission.

Proof of Concept

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L204-L240 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L246-L278 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L496-L499

Tools Used

Manual Review, VSCodium, Forge

Recommended Mitigation Steps

Apply the syncFeeCheckpoint modifier to the redeem().

QA report

Summary

The contract code quality is average, and the documentation is incomplete and often confusing.

List of non-critical findings:

1. @notice states that the function's "caller must be owner", but there is no onlyOwner modifier, and it must be open (check the docs). https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/DeploymentController.sol#L59-L72 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/README.md?plain=1#L33
2. With changeVaultAdapters() and changeVaultFees() in the VaultController contract, it is possible to call fallback or selector functions changeAdapter() and changeFees() from any contract in the network without any checks on behalf of AdminProxy. It possibly can be a medium-severity issue. https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L331-L344 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L368-L381
3. Comment from another function. https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L369
4. Unused variable. https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L448
5. @notice states that the function's "caller must be owner", but there is canCreate modifier instead. https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L79 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L181 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L274
6. @dev states that there are 525600 minutes per year, but the function uses SECONDS_PER_YEAR constants, which is 365.25 days or 525960 minutes. https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L422-L439 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L35
7. There's no check on the success of the call. https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L238
8. block.timestamp is, by default, added to event information. It is redundant to add it as an additional field. https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L536 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L585