Renzo - tapir's results

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Withdraw/WithdrawQueue.sol#L206-L263 https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Withdraw/WithdrawQueue.sol#L279-L312

Vulnerability details

Impact

When users request a withdraw and claim, the claimed amount and the burned ezETH amount is predetermined. If user waits for some time before actually claim (which is where burn and claim happens) the exchange rate of ezETH will inflated depending on how much the exchange rate changed and how much the user burns/receives. This will lead to a spike on ezETH exchange rate.

Proof of Concept

When users request withdraw they can call withdraw function in withdraw queue. If there are enough tokens in withdraw queue available, then the function will not revert and after a cool down period user can call claim to receive the underlying tokens.

Now, let's do an example to see what can go wrong: Assume that there are 100 WBETH in strategy where as 1 WBETH is 1 ETH and there are 100 total supply (100 ezETH) meaning 1 ezETH = 1 ETH = 1 WBETH initially. Also, 1 EigenLayer WBETH = 1 WBETH

Assume that the buffer for WBETH is 50 WBETH and 50 WBETH is in withdraw queue idle available for withdrawals and the rest of the 50 WBETH is in EigenLayer strategy. Note that the WBETH deposited in EigenLayer strategy can accrue more WBETH in future.

Alice, who holds 50 ezETH requests a withdraw by calling withdraw function and since the exchange rate is 1:1 she will be able to claim 50 WBETH after cooldown period is over by burning 50 ezETH. The rate will be guaranteed to be 1:1 for Alice after cooldown period.

After the cooldown period, only Alice can claim 50 WBETH and burn 50 ezETH because function is only care about "msg.sender". Now, this can be abused by Alice:

1- Assume that 6 months passes but Alice still didnt claim. Now, the 50 EigenLayer-WBETH Renzo holds say worth 55 WBETH. There are total of 100 ezETH still hence, 1 ezETH = 1.05 (50 WBETH in withdraw queue + 55 WBETH in EigenLayer) If Alice now decides to claim, since her burn amount and receive amount already determined (50 WBETH, 50 ezETH, 1:1) the new supply and assets will be: 55 WBETH and 50 ezETH. Which the new price of ezETH will be 1.1 ETH!

The reason why above increased the ezETH price is that the EigenLayer deposited WBETH earns in terms of WBETH hence, 1 EigenLayer WBETH shares will worth more WBETH in future. However, what Alice will get by burning some amount of ezETH in exchange for WBETH is pre determined in withdraw function and Alice can withdraw anytime, she does not have any incentive to withdraw further. If she waits for a prolonged time her claim will inflate the ezETH price.

Another scenario with stETH from same root cause Since the stETH rebases, the EigenLayer stETH shares are always gets bigger in terms of underlying stETH.

Hence, if there are 50 stETH idle in the withdraw queue which will be claimed 1:1 with 50 ezETH and if there are 100 stETH in EigenLayer strategy then at t=0 there are total 150 stETH and 150 ezETH price of ezETH = 1

at t=1000 there will be total of say 102 stETH in EigenLayer and 51 stETH in withdraw queue. However, the user will burn 50 ezETH to 50 stETH when its claimable regardless. The new price of ezETH before the claim is: 153 / 150 = 1.02

at t=1020 user claims, which will burn 50ezETH and withdraws 50 stETH. New balances will be: 1 stETH in withdraw queue + 102 stETH in EigenLayer = 103 stETH total supply will be = 150 - 50 = 100 new price of ezETH after the claim is: 103/100 = 1.03 !

Alternatively, slashings will have the reverse effect on the above scenarios.

Coded PoC:

function test_ClaimingLate_ChangesExchangeRate() external {
        // my setup only hve wBETH as collateral so I am adding steth
        vm.startPrank(management);
        ro.setOracleAddress(STETH, STETH_ORACLE);

        od1.setTokenStrategy(STETH, STETH_STRATEGY);
        od2.setTokenStrategy(STETH, STETH_STRATEGY);

        rsm.addCollateralToken(STETH);
        
        WithdrawQueueStorageV1.TokenWithdrawBuffer[] memory twb = new WithdrawQueueStorageV1.TokenWithdrawBuffer[](1);
        twb[0] = WithdrawQueueStorageV1.TokenWithdrawBuffer(address(STETH), 10 * 1e18);

        wq.updateWithdrawBufferTarget(twb);
        vm.stopPrank();
        // 
        
        // first add some wbeth, 10 wbeth is the buffer 
        vm.startPrank(tapir);
        uint amount = 100 * 1e18;
        deal(address(WBETH), tapir, amount);
        WBETH.approve(address(rsm), type(uint256).max);
        rsm.deposit(WBETH, amount);
        console2.log("Withdrawal queue bal", WBETH.balanceOf(address(wq)));
        vm.stopPrank();
        
        // basically a "deal" 
        vm.prank(STETH_WHALE);
        STETH.transfer(tapir, amount);

        // add some steth, 10 steth is the buffer
        vm.startPrank(tapir);
        STETH.approve(address(rsm), type(uint256).max);
        eet.approve(address(wq), type(uint256).max);
        uint diff = eet.balanceOf(tapir);
        rsm.deposit(STETH, amount);
        diff = eet.balanceOf(tapir) - diff;
        console2.log("Withdrawal queue bal", STETH.balanceOf(address(wq)));
        console2.log("Tapir adds 100 stETH and receives this much ezETH", diff);

        // tapir burns 9e18 ezETH for stETH
        wq.withdraw(9e18, address(STETH));
        console2.log("stETH will be withdrawn to tapir after cooldown period", wq.claimReserve(address(STETH)));

        (, , uint tvl) = rsm.calculateTVLs();
        uint ts = eet.totalSupply();
        uint256 priceOfEzethBeginning = tvl * 1e18 / ts;
        console2.log("Price of ezETH", priceOfEzethBeginning);
        vm.stopPrank();

        // mimick steth rebase yield to eigenlayer strategy
        vm.prank(STETH_WHALE);
        STETH.transfer(address(STETH_STRATEGY), 50e18);

        vm.prank(STETH_WHALE);
        STETH.transfer(address(wq), 0.1e18);

        // check the tvl again
        (, , tvl) = rsm.calculateTVLs();
        ts = eet.totalSupply();
        uint256 priceOfEzethAfterYield = tvl * 1e18 / ts;
        console2.log("Price of ezETH after some yield", priceOfEzethAfterYield);

        // because of yield exchange rate should be higher
        assertGe(priceOfEzethAfterYield, priceOfEzethBeginning);

        skip(WITHDRAWAL_COOLDOWN + 2);
        vm.prank(tapir);
        wq.claim(0);

        // check the tvl one last time after claim
        (, , tvl) = rsm.calculateTVLs();
        ts = eet.totalSupply();
        uint256 priceOfEzethAfterClaim = tvl * 1e18 / ts;
        console2.log("Price of ezETH after claiming steth", priceOfEzethAfterClaim);

        // priceOfEzethAfterClaim can be higher or lesser depending on the steth yield rebasing and how much 
        // is in EigenLyaer strategy and how much is in withdraw queue idle. Overall, the claimable tokens 
        // should be claimed as soon as possible to not create a price discrepany. 
    }

Tools Used

Recommended Mitigation Steps

Make claim function public. I don't see any incentive or attack path for someone claiming for someone else. Or, make it kickable by an admin role.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Withdraw/WithdrawQueue.sol#L206-L263 https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Withdraw/WithdrawQueue.sol#L279-L312

Vulnerability details

Impact

When users initiate the withdrawal flow with the withdraw function, the funds will be reserved for the claim for that specific user. After the cooldown period the user will be able to claim the tokens. Meanwhile, if the tokens are yield bearing like WBETH, wBETH user will not lose out the yield that will be generated in these times. However, if user picks to withdraw stETH since stETH is rebasing, user will lose the yield between the time spanned between the withdraw request and cool down period. This will disincentivize users to not withdraw to stETH and pick other LST's.

Proof of Concept

As we can see in the withdraw function the amount of LST tokens that will be claimed will be reserved:

function withdraw(uint256 _amount, address _assetOut) external nonReentrant {
        .
        .
        // Calculate amount to Redeem in ETH
        uint256 amountToRedeem = renzoOracle.calculateRedeemAmount(
            _amount,
            ezETH.totalSupply(),
            totalTVL
        );

        // update amount in claim asset, if claim asset is not ETH
        if (_assetOut != IS_NATIVE) {
            // Get ERC20 asset equivalent amount
            amountToRedeem = renzoOracle.lookupTokenAmountFromValue(
                IERC20(_assetOut),
                amountToRedeem
            );
        }
        .
        .
        .
        // add redeem amount to claimReserve of claim asset
        claimReserve[_assetOut] += amountToRedeem;
        .
    }

Say the amount of stETH calculated as amountToRedeem is 10 stETH. Now, user can claim the 10 stETH after the cooldown period via calling claim function. However, when user claims user won't be able to receive the yield accrued in the cooldown period. If user would've withdrawn 10 WBETH, since WBETH yield accrues to underlying token when WBETH redeemed, user wouldn't be entitled to that yield loss. This will disincentivize stETH withdrawals in general.

Tools Used

Recommended Mitigation Steps

Acknowledge this or make a custom withdrawal flow for stETH.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Withdraw/WithdrawQueue.sol#L206-L263 https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Rewards/RewardHandler.sol#L58-L60

Vulnerability details

Impact

When the Renzo validators earns execution layer rewards, the rewards will be sent to the RewardHandler contract. Since these rewards are not a traditional eth transfer the receive function will not automatically send the rewards to deposit queue. Instead, all the rewards will be accumulated and the admin will forward them to deposit queue. Anyone can deposit ETH before the rewards sent to deposit queue and immediately withdraw to pocket an atomic profit. Though the funds has to be claimed delayed because of the 2 step withdrawal process in Renzo.

Proof of Concept

Since the attack vector is simple I will not provide a coded poc but explain textually.

Assume there are 1 ETH rewards which comes from the execution layer rewards that the Renzo validators run. These rewards are currently standing idle in RewardHandler contract waiting for admin to call forwardRewards.

When attacker sees the forwardRewards function call, attacker quickly deposits some ether to Renzo before the forwardRewards tx. Then right after the forwardRewards tx, attacker starts withdrawal process by calling withdraw. Since the TVL increased when the forwardRewards tx placed in, attacker will receive more ether in return when the cooldown period is over.

Tools Used

Recommended Mitigation Steps

I think there can't be an easy code solution to this. Don't let rewards accumulate too much such that sandwich is not attractable.

Assessed type

MEV

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Deposits/DepositQueue.sol#L134-L145 https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Delegation/OperatorDelegator.sol#L265-L314

Vulnerability details

Impact

When the queued withdrawal is completed by the native eth stake admin in the operator delegator, the complete withdrawal function can revert if there is a deficit to fill. The revert reason is simply that the call to the deposit queue contracts fillERC20withdrawBuffer assumes the caller will be always RestakeManager however in this case the caller will be OperatorDelegator contract. Completing a withdrawal queue can be impossible if there is a deficit and there are no further deposits.

Proof of Concept

When the complete withdrawal is called, if there are any deficit to fill in withdraw queue contract the amount up to the deficit amount will be transferred to deposit queue as follows:

function completeQueuedWithdrawal(
        IDelegationManager.Withdrawal calldata withdrawal,
        IERC20[] calldata tokens,
        uint256 middlewareTimesIndex
    ) external nonReentrant onlyNativeEthRestakeAdmin {
            .
            .
            // check if token is not Native ETH
            if (address(tokens[i]) != IS_NATIVE) {
                // Check the withdraw buffer and fill if below buffer target
                uint256 bufferToFill = withdrawQueue.getBufferDeficit(address(tokens[i]));

                // get balance of this contract
                uint256 balanceOfToken = tokens[i].balanceOf(address(this));
                if (bufferToFill > 0) {
                    bufferToFill = (balanceOfToken <= bufferToFill) ? balanceOfToken : bufferToFill;
                    .
                    .
                    // fill Withdraw Buffer via depositQueue
                    -> restakeManager.depositQueue().fillERC20withdrawBuffer(
                        address(tokens[i]),
                        bufferToFill
                    );
                    .
        }

However, the deposit queues fillERC20withdrawBuffer is not expecting the OperatorDelegator contract to call itself and has a only restake manager modifier as follows:

function fillERC20withdrawBuffer(
        address _asset,
        uint256 _amount
    -> ) external nonReentrant onlyRestakeManager {
        if (_amount == 0 || _asset == address(0)) revert InvalidZeroInput();
        // safeTransfer from restake manager to this address
        IERC20(_asset).safeTransferFrom(msg.sender, address(this), _amount);
        // approve the token amount for withdraw queue
        IERC20(_asset).safeApprove(address(withdrawQueue), _amount);
        // call the withdraw queue to fill up the buffer
        withdrawQueue.fillERC20WithdrawBuffer(_asset, _amount);
    }

Hence, the function reverts.

Coded PoC:

// forge test --fork-url {mainnet_rpc} -vv
function test_OnlyRestakeManagerModifierRevertsWhenCompletingWithdrawals() external {
        uint amount = 100 * 1e18;
        deal(address(WBETH), tapir, amount);
        assertEq(WBETH.balanceOf(tapir), amount);

        vm.startPrank(tapir);
        WBETH.approve(address(rsm), type(uint256).max);
        rsm.deposit(WBETH, amount);
        vm.stopPrank();

        console2.log("Buffer deficit before", wq.getBufferDeficit(address(WBETH)));

        vm.startPrank(management);
        WithdrawQueueStorageV1.TokenWithdrawBuffer[] memory twb = new WithdrawQueueStorageV1.TokenWithdrawBuffer[](1);
        twb[0] = WithdrawQueueStorageV1.TokenWithdrawBuffer(address(WBETH), 20 * 1e18);
        wq.updateWithdrawBufferTarget(twb);

        console2.log("Buffer deficit after", wq.getBufferDeficit(address(WBETH)));

        // withdraw 10e18 WBETH
        IERC20[] memory _tokens = new IERC20[](1);
        _tokens[0] = WBETH;
        uint256[] memory _amounts = new uint256[](1);
        _amounts[0] = 10 * 1e18;
        uint32 blockNo = uint32(block.number);
        od1.queueWithdrawals(_tokens, _amounts);
        assertEq(block.number, blockNo);
        console2.log("block no", blockNo);

        
        IStrategy[] memory _strats = new IStrategy[](1);
        _strats[0] = WBETH_STRATEGY;
        _amounts[0] = WBETH.STRATEGY.underlyingToSharesView(10e18);
        IDelegationManager.Withdrawal memory w = IDelegationManager.Withdrawal(address(od1), address(0), address(od1), 0, blockNo, _strats, _amounts); 
        
        vm.roll(block.number + 216000 + 1);
        od1.completeQueuedWithdrawal(w, _tokens, 0);
    }

Tools Used

Recommended Mitigation Steps

Modify the modifier in the DepositQueue and allow the operator delegator to call it as well.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Deposits/DepositQueue.sol#L134-L145 https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Delegation/OperatorDelegator.sol#L265-L314

Vulnerability details

Impact

When the queued withdrawal is completed by the native eth stake admin in the operator delegator, the complete withdrawal function can revert if there is a deficit to fill. The revert reason is simply that the call to the deposit queue contracts fillERC20withdrawBuffer assumes the caller will be always RestakeManager however in this case the caller will be OperatorDelegator contract. Completing a withdrawal queue can be impossible if there is a deficit and there are no further deposits.

Proof of Concept

When the complete withdrawal is called, if there are any deficit to fill in withdraw queue contract the amount up to the deficit amount will be transferred to deposit queue as follows:

function completeQueuedWithdrawal(
        IDelegationManager.Withdrawal calldata withdrawal,
        IERC20[] calldata tokens,
        uint256 middlewareTimesIndex
    ) external nonReentrant onlyNativeEthRestakeAdmin {
            .
            .
            // check if token is not Native ETH
            if (address(tokens[i]) != IS_NATIVE) {
                // Check the withdraw buffer and fill if below buffer target
                uint256 bufferToFill = withdrawQueue.getBufferDeficit(address(tokens[i]));

                // get balance of this contract
                uint256 balanceOfToken = tokens[i].balanceOf(address(this));
                if (bufferToFill > 0) {
                    bufferToFill = (balanceOfToken <= bufferToFill) ? balanceOfToken : bufferToFill;
                    .
                    .
                    // fill Withdraw Buffer via depositQueue
                    -> restakeManager.depositQueue().fillERC20withdrawBuffer(
                        address(tokens[i]),
                        bufferToFill
                    );
                    .
        }

However, the deposit queues fillERC20withdrawBuffer is not expecting the OperatorDelegator contract to call itself and has a only restake manager modifier as follows:

function fillERC20withdrawBuffer(
        address _asset,
        uint256 _amount
    -> ) external nonReentrant onlyRestakeManager {
        if (_amount == 0 || _asset == address(0)) revert InvalidZeroInput();
        // safeTransfer from restake manager to this address
        IERC20(_asset).safeTransferFrom(msg.sender, address(this), _amount);
        // approve the token amount for withdraw queue
        IERC20(_asset).safeApprove(address(withdrawQueue), _amount);
        // call the withdraw queue to fill up the buffer
        withdrawQueue.fillERC20WithdrawBuffer(_asset, _amount);
    }

Hence, the function reverts.

Coded PoC:

// forge test --fork-url {mainnet_rpc} -vv
function test_OnlyRestakeManagerModifierRevertsWhenCompletingWithdrawals() external {
        uint amount = 100 * 1e18;
        deal(address(WBETH), tapir, amount);
        assertEq(WBETH.balanceOf(tapir), amount);

        vm.startPrank(tapir);
        WBETH.approve(address(rsm), type(uint256).max);
        rsm.deposit(WBETH, amount);
        vm.stopPrank();

        console2.log("Buffer deficit before", wq.getBufferDeficit(address(WBETH)));

        vm.startPrank(management);
        WithdrawQueueStorageV1.TokenWithdrawBuffer[] memory twb = new WithdrawQueueStorageV1.TokenWithdrawBuffer[](1);
        twb[0] = WithdrawQueueStorageV1.TokenWithdrawBuffer(address(WBETH), 20 * 1e18);
        wq.updateWithdrawBufferTarget(twb);

        console2.log("Buffer deficit after", wq.getBufferDeficit(address(WBETH)));

        // withdraw 10e18 WBETH
        IERC20[] memory _tokens = new IERC20[](1);
        _tokens[0] = WBETH;
        uint256[] memory _amounts = new uint256[](1);
        _amounts[0] = 10 * 1e18;
        uint32 blockNo = uint32(block.number);
        od1.queueWithdrawals(_tokens, _amounts);
        assertEq(block.number, blockNo);
        console2.log("block no", blockNo);

        
        IStrategy[] memory _strats = new IStrategy[](1);
        _strats[0] = WBETH_STRATEGY;
        _amounts[0] = WBETH.STRATEGY.underlyingToSharesView(10e18);
        IDelegationManager.Withdrawal memory w = IDelegationManager.Withdrawal(address(od1), address(0), address(od1), 0, blockNo, _strats, _amounts); 
        
        vm.roll(block.number + 216000 + 1);
        od1.completeQueuedWithdrawal(w, _tokens, 0);
    }

Tools Used

Recommended Mitigation Steps

Modify the modifier in the DepositQueue and allow the operator delegator to call it as well.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Deposits/DepositQueue.sol#L134-L145 https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Delegation/OperatorDelegator.sol#L265-L314

Vulnerability details

Impact

When the queued withdrawal is completed by the native eth stake admin in the operator delegator, the complete withdrawal function can revert if there is a deficit to fill. The revert reason is simply that the call to the deposit queue contracts fillERC20withdrawBuffer assumes the caller will be always RestakeManager however in this case the caller will be OperatorDelegator contract. Completing a withdrawal queue can be impossible if there is a deficit and there are no further deposits.

Proof of Concept

When the complete withdrawal is called, if there are any deficit to fill in withdraw queue contract the amount up to the deficit amount will be transferred to deposit queue as follows:

function completeQueuedWithdrawal(
        IDelegationManager.Withdrawal calldata withdrawal,
        IERC20[] calldata tokens,
        uint256 middlewareTimesIndex
    ) external nonReentrant onlyNativeEthRestakeAdmin {
            .
            .
            // check if token is not Native ETH
            if (address(tokens[i]) != IS_NATIVE) {
                // Check the withdraw buffer and fill if below buffer target
                uint256 bufferToFill = withdrawQueue.getBufferDeficit(address(tokens[i]));

                // get balance of this contract
                uint256 balanceOfToken = tokens[i].balanceOf(address(this));
                if (bufferToFill > 0) {
                    bufferToFill = (balanceOfToken <= bufferToFill) ? balanceOfToken : bufferToFill;
                    .
                    .
                    // fill Withdraw Buffer via depositQueue
                    -> restakeManager.depositQueue().fillERC20withdrawBuffer(
                        address(tokens[i]),
                        bufferToFill
                    );
                    .
        }

However, the deposit queues fillERC20withdrawBuffer is not expecting the OperatorDelegator contract to call itself and has a only restake manager modifier as follows:

function fillERC20withdrawBuffer(
        address _asset,
        uint256 _amount
    -> ) external nonReentrant onlyRestakeManager {
        if (_amount == 0 || _asset == address(0)) revert InvalidZeroInput();
        // safeTransfer from restake manager to this address
        IERC20(_asset).safeTransferFrom(msg.sender, address(this), _amount);
        // approve the token amount for withdraw queue
        IERC20(_asset).safeApprove(address(withdrawQueue), _amount);
        // call the withdraw queue to fill up the buffer
        withdrawQueue.fillERC20WithdrawBuffer(_asset, _amount);
    }

Hence, the function reverts.

Coded PoC:

// forge test --fork-url {mainnet_rpc} -vv
function test_OnlyRestakeManagerModifierRevertsWhenCompletingWithdrawals() external {
        uint amount = 100 * 1e18;
        deal(address(WBETH), tapir, amount);
        assertEq(WBETH.balanceOf(tapir), amount);

        vm.startPrank(tapir);
        WBETH.approve(address(rsm), type(uint256).max);
        rsm.deposit(WBETH, amount);
        vm.stopPrank();

        console2.log("Buffer deficit before", wq.getBufferDeficit(address(WBETH)));

        vm.startPrank(management);
        WithdrawQueueStorageV1.TokenWithdrawBuffer[] memory twb = new WithdrawQueueStorageV1.TokenWithdrawBuffer[](1);
        twb[0] = WithdrawQueueStorageV1.TokenWithdrawBuffer(address(WBETH), 20 * 1e18);
        wq.updateWithdrawBufferTarget(twb);

        console2.log("Buffer deficit after", wq.getBufferDeficit(address(WBETH)));

        // withdraw 10e18 WBETH
        IERC20[] memory _tokens = new IERC20[](1);
        _tokens[0] = WBETH;
        uint256[] memory _amounts = new uint256[](1);
        _amounts[0] = 10 * 1e18;
        uint32 blockNo = uint32(block.number);
        od1.queueWithdrawals(_tokens, _amounts);
        assertEq(block.number, blockNo);
        console2.log("block no", blockNo);

        
        IStrategy[] memory _strats = new IStrategy[](1);
        _strats[0] = WBETH_STRATEGY;
        _amounts[0] = WBETH.STRATEGY.underlyingToSharesView(10e18);
        IDelegationManager.Withdrawal memory w = IDelegationManager.Withdrawal(address(od1), address(0), address(od1), 0, blockNo, _strats, _amounts); 
        
        vm.roll(block.number + 216000 + 1);
        od1.completeQueuedWithdrawal(w, _tokens, 0);
    }

Tools Used

Recommended Mitigation Steps

Modify the modifier in the DepositQueue and allow the operator delegator to call it as well.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Withdrawal queue token balances are counted with the wrong token price feed due to usage of "i" instead of "j" in the loop. This will lead to miscalculated TVL of the protocol hence, wrong exchange rate of ezETH.

Proof of Concept

The TVL of the Renzo protocol is calculated as follows:

• All the operator delegators token balances and eth balances +
• All the idle token and eth balances of withdrawal queue +
• Deposit queue ETH balance

The calculateTVLs function uses this method to account the idle withdrawal queue balances:

function calculateTVLs() public view returns (uint256[][] memory, uint256[] memory, uint256) {
        uint256[][] memory operatorDelegatorTokenTVLs = new uint256[][](operatorDelegators.length);
        uint256[] memory operatorDelegatorTVLs = new uint256[](operatorDelegators.length);
        uint256 totalTVL = 0;

        // Iterate through the ODs
        uint256 odLength = operatorDelegators.length;

        // flag for withdrawal queue balance set
        bool withdrawQueueTokenBalanceRecorded = false;
        address withdrawQueue = address(depositQueue.withdrawQueue());

        // withdrawalQueue total value
        uint256 totalWithdrawalQueueValue = 0;

        for (uint256 i = 0; i < odLength; ) {
            // Track the TVL for this OD
            uint256 operatorTVL = 0;

            // Track the individual token TVLs for this OD - native ETH will be last item in the array
            uint256[] memory operatorValues = new uint256[](collateralTokens.length + 1);
            operatorDelegatorTokenTVLs[i] = operatorValues;

            // Iterate through the tokens and get the value of each
            uint256 tokenLength = collateralTokens.length;
            for (uint256 j = 0; j < tokenLength; ) {
                // Get the value of this token

                uint256 operatorBalance = operatorDelegators[i].getTokenBalanceFromStrategy(
                    collateralTokens[j]
                );

                // Set the value in the array for this OD
                operatorValues[j] = renzoOracle.lookupTokenValue(
                    collateralTokens[j],
                    operatorBalance
                );

                // Add it to the total TVL for this OD
                operatorTVL += operatorValues[j];

                // record token value of withdraw queue
                -> if (!withdrawQueueTokenBalanceRecorded) {
                ->  totalWithdrawalQueueValue += renzoOracle.lookupTokenValue(
                ->      collateralTokens[i],
                ->      collateralTokens[j].balanceOf(withdrawQueue)
                ->  );
                }

                unchecked {
                    ++j;
                }
            }
            .
            .
            .
    }

So we can see that first the "i" counter is looping for every delegator and "j" counter is for the each collateral token.

If there are 3 operator delegators and 5 collateral tokens, then the "i" would be maximum "2" and "j" would be maximum "4". Since the idle balances in withdraw queue will always be same they are accounted only in the first loop when "i" was equal to "0". However, there is an obvious mistake when valuing the idle withdraw queue balances. As we can observe on the "->" lines pointed above snippet, collateralTokens[i] is used instead of collateralTokens[j]. Which will result the accounting of collateral token in terms of the first collateral token always although the balance is correctly chosen. In result, all the idle tokens in withdraw queue will be valued in collateralTokens[0] regardless of the actual token.

This issue is the first ever issue I am submitting for this contest. Hence, I will add my test suite code here in the gist: https://gist.github.com/tapired/42622ec4bd589f4f48e70c3d01d8afd2

All the other PoC's I will provide in other issues will base the above setup.

PoC:

// forge test --fork-url {mainnet_rpc} -vv
function test_InaccurateAccounting_InWithdrawQueue() external {
        // my setup only hve wBETH as collateral so I am adding steth
        vm.startPrank(management);
        ro.setOracleAddress(STETH, STETH_ORACLE);

        od1.setTokenStrategy(STETH, STETH_STRATEGY);
        od2.setTokenStrategy(STETH, STETH_STRATEGY);

        rsm.addCollateralToken(STETH);
        
        WithdrawQueueStorageV1.TokenWithdrawBuffer[] memory twb = new WithdrawQueueStorageV1.TokenWithdrawBuffer[](1);
        twb[0] = WithdrawQueueStorageV1.TokenWithdrawBuffer(address(STETH), 10 * 1e18);

        wq.updateWithdrawBufferTarget(twb);
        vm.stopPrank();
        // 
        
        // first add some wbeth, 10 wbeth is the buffer 
        vm.startPrank(tapir);
        uint amount = 100 * 1e18;
        deal(address(WBETH), tapir, amount);
        WBETH.approve(address(rsm), type(uint256).max);
        rsm.deposit(WBETH, amount);
        console2.log("Withdrawal queue bal", WBETH.balanceOf(address(wq)));
        vm.stopPrank();
        
        // basically a "deal" 
        vm.prank(STETH_WHALE);
        STETH.transfer(tapir, amount);

        // add some steth, 10 steth is the buffer
        vm.startPrank(tapir);
        STETH.approve(address(rsm), type(uint256).max);
        rsm.deposit(STETH, amount);
        console2.log("Withdrawal queue bal", STETH.balanceOf(address(wq)));

        // calculate tvl using Renzo function, steth will be valued as its wbeth which we expect
        // this tvl to be higher than actual tvl! 
        (, , uint TVL) = rsm.calculateTVLs();
        console2.log("TVL", TVL);
        vm.stopPrank();

        // we know that 90 stETH in operator, 90 wBETH in operator
        // 10 stETH withdraw queue, 10 wBETH in withdraw queue
        (, int256 p, , ,) = WBETH_ORACLE.latestRoundData();
        uint totalCBETH = uint256(100e18 * p / 1e18);

        (, p, , ,) = STETH_ORACLE.latestRoundData();
        uint totalSTETH = uint256(100e18 * p / 1e18);

        // correct tvl
        uint totalTVLManual = totalSTETH + totalCBETH;
        console2.log("Total TVL Manual", totalTVLManual);
    }

Test Logs: Ran 1 test for test/TapirTests.sol:TapirTests [PASS] test_InaccurateAccounting_InWithdrawQueue() (gas: 1622649) Logs: Withdrawal queue bal 10000000000000000000 Withdrawal queue bal 9999999999999999999 TVL 207477601783187653995 Total TVL Manual 206786446425764060000

Tools Used

Recommended Mitigation Steps

change collateralTokens[i] to collateralTokens[j]

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Withdraw/WithdrawQueue.sol#L13 https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Withdraw/WithdrawQueue.sol#L139-L149

Vulnerability details

Impact

Withdraw queue is a pausable contract and also it has pause and unpause functions indicating that some functions should be pausable. Also, in Renzo C4 audit page it says: "DEPOSIT_WITHDRAW_PAUSER allows the admin to pause/unpause deposits and withdraws" so it is intended that there will be a role such that can pause withdrawals which is consistent with pause/unpause and Pausable contract inheritance in withdraw queue. However, pausable modifier is not used in any of the functions, especially in withdraw function in WithdrawQueue contract.

Proof of Concept

As we can see the withdraw function has no whenNotPaused modifier which means pausing or unpausing has no impact in contract

function withdraw(uint256 _amount, address _assetOut) external nonReentrant {
        // check for 0 values
        if (_amount == 0 || _assetOut == address(0)) revert InvalidZeroInput();

        // check if provided assetOut is supported
        if (withdrawalBufferTarget[_assetOut] == 0) revert UnsupportedWithdrawAsset();

        // transfer ezETH tokens to this address
        IERC20(address(ezETH)).safeTransferFrom(msg.sender, address(this), _amount);

        // calculate totalTVL
        (, , uint256 totalTVL) = restakeManager.calculateTVLs();

        // Calculate amount to Redeem in ETH
        uint256 amountToRedeem = renzoOracle.calculateRedeemAmount(
            _amount,
            ezETH.totalSupply(),
            totalTVL
        );

        // update amount in claim asset, if claim asset is not ETH
        if (_assetOut != IS_NATIVE) {
            // Get ERC20 asset equivalent amount
            amountToRedeem = renzoOracle.lookupTokenAmountFromValue(
                IERC20(_assetOut),
                amountToRedeem
            );
        }

        // revert if amount to redeem is greater than withdrawBufferTarget
        if (amountToRedeem > getAvailableToWithdraw(_assetOut)) revert NotEnoughWithdrawBuffer();

        // increment the withdrawRequestNonce
        withdrawRequestNonce++;

        // add withdraw request for msg.sender
        withdrawRequests[msg.sender].push(
            WithdrawRequest(
                _assetOut,
                withdrawRequestNonce,
                amountToRedeem,
                _amount,
                block.timestamp
            )
        );

        // add redeem amount to claimReserve of claim asset
        claimReserve[_assetOut] += amountToRedeem;

        emit WithdrawRequestCreated(
            withdrawRequestNonce,
            msg.sender,
            _assetOut,
            amountToRedeem,
            _amount,
            withdrawRequests[msg.sender].length - 1
        );
    }

Tools Used

Recommended Mitigation Steps

Add whenNotPaused modifier to withdraw function.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Bridge/L2/xRenzoDeposit.sol#L168-L191 https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Bridge/L1/xRenzoBridge.sol#L139-L201 https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/RestakeManager.sol#L592-L616

Vulnerability details

Impact

Other chains where xEzETH minted collects the ETH and admin sweeps it to L1. When funds received in L1, funds are deposited into restake manager to get ezETH. However, minting ezETH in L2 does not care about the max TVL in restake manager but the L1 does. When funds are bridged, if the TVL limit is reached then the sweeping will fail and ezETH in L1 will not be minted. This will lead to existence of ezETH in other chains that are not backed in L1. Also, if the maximum tvl is reached in L1 anyone can keep minting ezETH in L2's bypassing the tvl limit.

Proof of Concept

Users can deposit ETH/WETH to other chains deposit contract to receive xezETH which can be exchanged to ezETH in mainnet by connext bridge.

When funds are gathered in L2 deposit contract, admin can call the sweep function to send ETH/WETH to L1 bridge contract where the ETH/WETH will be deposited to restake manager to get ezETH.

As we can see in the xReceive function in the bridge contract of L1 which is the function will be called by connext when funds are sweeped from L2 to L1, the funds are deposited to restaking manager in bulk.

function xReceive(
        bytes32 _transferId,
        uint256 _amount,
        address _asset,
        address _originSender,
        uint32 _origin,
        bytes memory
    ) external nonReentrant returns (bytes memory) {
        .
        .
        // Deposit it into Renzo RestakeManager
        -> restakeManager.depositETH{ value: ethAmount }();
        .
        .
        .
    }

However, as we can see in restake manager depositETH function, if the max TVL is reached the function can revert:

function depositETH(uint256 _referralId) public payable nonReentrant notPaused {
        .
        // Enforce TVL limit if set
        -> if (maxDepositTVL != 0 && totalTVL + msg.value > maxDepositTVL) {
            revert MaxTVLReached();
        }
        .
        .
    }

If the function revert because of the maxDepositTVL is reached, then the sweep function will fail. However, the xEzETH that is minted by the L2 users can still bridge their L2 ezETH to L1 ezETH but since the L1 ezETH is not minted because of maxTVL the bridged xEzETH will have no collateral backing in L1.

Also, users will be able to mint xezETH in L2 although the maxTVL is reached in L1, breaking one of the core functionality of the protocol

Tools Used

Recommended Mitigation Steps

Send the maximum tvl limit to L2 via oracle just like price.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/RestakeManager.sol#L491-L576 https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Delegation/OperatorDelegator.sol#L143-L154

Vulnerability details

Impact

When users deposits if the target buffer amount is not satisfied in withdraw queue contract the user deposit sent to withdraw queue. If the amount user deposits is lesser than the amount that fills the withdraw queue token balance amount to target buffer amount then the transaction will revert due to moving with the function execution with "0" amount. Users can't deposit an amount that is lesser than the target buffer amount.

Proof of Concept

When users deposits some amount might go to withdraw queue depending on the target buffer amount and how much the withdraw queue currently has.

function deposit(
        IERC20 _collateralToken,
        uint256 _amount,
        uint256 _referralId
    ) public nonReentrant notPaused {
        .
        .
        // Check the withdraw buffer and fill if below buffer target
        -> uint256 bufferToFill = depositQueue.withdrawQueue().getBufferDeficit(
            address(_collateralToken)
        );
        
        -> if (bufferToFill > 0) {
            bufferToFill = (_amount <= bufferToFill) ? _amount : bufferToFill;
            
            // update amount to send to the operator Delegator
            _amount -= bufferToFill;

            // safe Approve for depositQueue
            _collateralToken.safeApprove(address(depositQueue), bufferToFill);

            // fill Withdraw Buffer via depositQueue
            depositQueue.fillERC20withdrawBuffer(address(_collateralToken), bufferToFill);
        }

        // Approve the tokens to the operator delegator
        -> _collateralToken.safeApprove(address(operatorDelegator), _amount);

        // Call deposit on the operator delegator
        -> operatorDelegator.deposit(_collateralToken, _amount);
        .
        .
    }

As we can see in the above snippet of deposit function, if the amount user deposits is lesser than the bufferToFill then the entire amount user deposits sends to the withdraw queue and the new _amount will be "0".

When the function moves on with the execution after the if check, since the _amount is "0" the operator delegators deposit function will be called with value "0" which the operator delegator contract will throw a revert because of this check in the deposit function:

function deposit(
        IERC20 token,
        uint256 tokenAmount
    ) external nonReentrant onlyRestakeManager returns (uint256 shares) {
        -> if (address(tokenStrategyMapping[token]) == address(0x0) || tokenAmount == 0)
         .
    }

Coded PoC:

// forge test --fork-url {mainnet_rpc} -vv
function test_invalidZeroInput() external {
        // the buffer is 10e18, user tries to deposit 9e18 which will revert.
        uint amount = 9e18;
        deal(address(WBETH), tapir, amount);
        assertEq(WBETH.balanceOf(tapir), amount);

        vm.startPrank(tapir);
        WBETH.approve(address(rsm), type(uint256).max);
        vm.expectRevert(bytes4(keccak256("InvalidZeroInput()")));
        rsm.deposit(WBETH, amount);

        console2.log("eet bal", eet.balanceOf(tapir));
        console2.log("Withdrawal queue bal", WBETH.balanceOf(address(wq)));
    }

Tools Used

Recommended Mitigation Steps

early exit if _amount <= bufferToFill

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Delegation/OperatorDelegator.sol#L481-L525

Vulnerability details

Impact

Validators execution rewards will be sent to operator delegator contract from the withdrawal router. When the ether is received in OperatorDelegator as a result of this, the tx.origin will be refunded as gas considering the tx.origin is the admin. However, initiating a claim in delayed withdrawal router in EigenLayer is permissionless which anyone can claim behalf of the operator delegator. Since the address that will claim is not the restaking admin of the operator delegator, all the funds will be sent to the deposit queue and admin will not be able to take the fees from it.

Proof of Concept

Admin records fees to be taken when:

• queueWithdrawals
• completeQueuedWithdrawal
• verifyWithdrawalCredentials
• verifyAndProcessWithdrawals

the recording of the fee is as follows:

function verifyAndProcessWithdrawals(
        uint64 oracleTimestamp,
        BeaconChainProofs.StateRootProof calldata stateRootProof,
        BeaconChainProofs.WithdrawalProof[] calldata withdrawalProofs,
        bytes[] calldata validatorFieldsProofs,
        bytes32[][] calldata validatorFields,
        bytes32[][] calldata withdrawalFields
    ) external onlyNativeEthRestakeAdmin {
        -> uint256 gasBefore = gasleft();
        eigenPod.verifyAndProcessWithdrawals(
            oracleTimestamp,
            stateRootProof,
            withdrawalProofs,
            validatorFieldsProofs,
            validatorFields,
            withdrawalFields
        );
        // update the gas spent for RestakeAdmin
        -> _recordGas(gasBefore);
    }

and this is the _recordGas implementation:

function _recordGas(uint256 initialGas) internal {
        uint256 gasSpent = (initialGas - gasleft() + baseGasAmountSpent) * tx.gasprice;
        adminGasSpentInWei[msg.sender] += gasSpent;
        emit GasSpent(msg.sender, gasSpent);
    }

since the admin can only call these functions, msg.sender will have the role restake admin role guaranteed.

Now, when and how the validator rewards claimed from EigenPod? Beacon chain effective balance is capped to 32 ether anything that is above this can be partially withdrawn to the EigenPod owner with a delayed router. The funds will be sent to DelayedRouter and once the delay passes anyone can claim behalf of the account.

When the amounts are claimed from EigenLayer delayed router, ether will sent to the pod owner which is the OperatorDelegator contract as follows: https://github.com/Layr-Labs/eigenlayer-contracts/blob/98685285e5e504fa6180c010d2835cd506c4ecc6/src/contracts/pods/DelayedWithdrawalRouter.sol#L216-L218

Since this is a plain ether transfer, the receive() function will be triggered in operator delegator contract:

receive() external payable nonReentrant {
        // check if sender contract is EigenPod. forward full withdrawal eth received
        if (msg.sender == address(eigenPod)) {
            restakeManager.depositQueue().forwardFullWithdrawalETH{ value: msg.value }();
        } else {
            // considered as protocol reward
            uint256 gasRefunded = 0;
            uint256 remainingAmount = msg.value;
            -> if (adminGasSpentInWei[tx.origin] > 0) {
                gasRefunded = _refundGas();
                // update the remaining amount
                remainingAmount -= gasRefunded;
                // If no funds left, return
                if (remainingAmount == 0) {
                    return;
                }
            }
            // Forward remaining balance to the deposit queue
            address destination = address(restakeManager.depositQueue());
            (bool success, ) = destination.call{ value: remainingAmount }("");
            if (!success) revert TransferFailed();

            emit RewardsForwarded(destination, remainingAmount);
        }

since the tx.origin will be the random address that calls the claim in delayed router behalf of the operator delegator contract the adminGasSpentInWei[tx.origin] will be 0. Hence, there won't be any fees taken and all the claimed amount will be sent to deposit queue.

In result, admin can never receive its gas fees since users are incentivized to call claim themselves to bypass the admin fee and receive more rewards which increases ezETH exchange rate further.

Tools Used

Recommended Mitigation Steps

Regardless of the tx.origin, take the fees. Otherwise, users are well encouraged and incentivized to call claim themselves.

Assessed type

Other