Initialization functions can be front-run

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/RSETH.sol#L14 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L19

Vulnerability details

Impact

kelp protocol makes extensive use of openzepllin Initializable contract such as in RSETH and LRTDepositPool contracts. The _disableInitializers function of Initializable contract is invoked in construcotrs of the contracts and later initialize function is called to set the parameters & state variables of the contracts. However, the initialize function does not have any access control and can be called by anyone. This also means that initialize function can be front-run, allowing an attacker to incorrectly initialize the contracts. Thus, malicious user may initialize the deployed contracts to wrong values, forcing the deployer of to redeploy these contracts. The process can be repeated, which costs the malicious user less (only a small transaction fee) than it would to the owner of the kelp protocol (cost of contract re-deployment), potentially unnecessarily draining funds from the development team.

Proof of Concept

Following test shows that any user can invoke initialize function on deployed contracts and configure the state to desired values. Include following test in RSETHInitialize contract in RSETHTest.t.sol file for testing :-

    function test_InitializeContractsVariablesByAnyOne() external {   
        vm.startPrank(bob);        //bob is not admin
        rseth.initialize(address(alice), address(lrtConfig));     

        assertTrue(rseth.hasRole(rseth.DEFAULT_ADMIN_ROLE(), alice), "Alice not set as Admin");
        assertEq(address(lrtConfig), address(rseth.lrtConfig()), "LRT config address is not set");        

        assertEq(rseth.name(), "rsETH", "Name is not set");
        assertEq(rseth.symbol(), "rsETH", "Symbol is not set");
        vm.stopPrank();
    }

Similarly, include following test in LRTDepositPoolInitialize contract in LRTDepositPoolTest.t.sol file for testing :-

    function test_InitializeContractsVariablesByAnyOne() external {
        vm.startPrank(bob);         //bob is not admin
        address dummy = makeAddr("dummy");
        lrtDepositPool.initialize(address(dummy));

        assertEq(lrtDepositPool.maxNodeDelegatorCount(), 10, "Max node delegator count is not set");
        assertEq(address(dummy), address(lrtDepositPool.lrtConfig()), "LRT config address is not set");
        vm.stopPrank();
    }

similar issue applicable to other contracts such as NodeDelegator.

Tools Used

Manual Review

Recommended Mitigation Steps

Implement valid access control such that only the relevant/ authorize deployer can initialize().

Assessed type

Access Control

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #273

[c4-judge]

fatherGoose1 marked the issue as unsatisfactory: Invalid