Renzo - KupiaSec's results

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Delegation/OperatorDelegator.sol#L327-L335

Vulnerability details

Impact

The getTokenBalanceFromStrategy() function returns a small value than it should be, leading to falling down the exchange rate of ezETH, which is loss of funds to the ezETH holders.

Proof of Concept

Let's consider L329 of the getTokenBalanceFromStrategy() function. Since queuedShares is a mapping of token shares in the withdrawal queue, queuedShares[address(this)] is always 0. Actually, it should be queuedShares[address(token)]. As a result, the function returns only tokenStrategyMapping[token].userUnderlyingView(address(this)), not tokenStrategyMapping[token].sharesToUnderlyingView(queuedShares[address(token)]), even though there are some queued amounts of token. Consequently, this will impact the TVL and cause the exchange rate of ezETH to be lower than it should be, leading to incorrect actions of the entire protocol, such as a loss of funds for the ezETH holders.

    function getTokenBalanceFromStrategy(IERC20 token) external view returns (uint256) {
        return
329         queuedShares[address(this)] == 0
                ? tokenStrategyMapping[token].userUnderlyingView(address(this))
                : tokenStrategyMapping[token].userUnderlyingView(address(this)) +
                    tokenStrategyMapping[token].sharesToUnderlyingView(
                        queuedShares[address(token)]
                    );
    }

Tools Used

Manual review

Recommended Mitigation Steps

At L329, address(this) should be replaced to address(token).

    function getTokenBalanceFromStrategy(IERC20 token) external view returns (uint256) {
        return
-           queuedShares[address(this)] == 0
+           queuedShares[address(token)] == 0
                ? tokenStrategyMapping[token].userUnderlyingView(address(this))
                : tokenStrategyMapping[token].userUnderlyingView(address(this)) +
                    tokenStrategyMapping[token].sharesToUnderlyingView(
                        queuedShares[address(token)]
                    );
    }

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

ETH withdrawal does not work, withdrawn ETH assets are stuck in EigenLayer.

Proof of Concept

When withdrawals happen, completeQueuedWithdrawal is called to claim withdrawn assets from EigenLayer, which calls completeQueuedWithdrawal function of EigenLayer as well. If native tokens(ETH) are included in the withdrawal batch, the EigenLayer tries to send ETH to the OperatorDelegator contract, which will call receive function of the contract.

The problem here is that both completeQueuedWithdrawal function and receive function has nonReentrant modifier, thus receiving ETH through receive function will revert.

function completeQueuedWithdrawal(
    IDelegationManager.Withdrawal calldata withdrawal,
    IERC20[] calldata tokens,
    uint256 middlewareTimesIndex
@> ) external nonReentrant onlyNativeEthRestakeAdmin {
    // ...
}

@> receive() external payable nonReentrant {
    // ...
}

Tools Used

Manual Review

Recommended Mitigation Steps

nonReentrant modifier should not be used in receive function.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Withdraw/WithdrawQueue.sol#L206-L263 https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Withdraw/WithdrawQueue.sol#L279-L312 https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/RestakeManager.sol#L274-L358

Vulnerability details

Impact

Assets that are included in queued withdrawals still participate in TVL, leading to incorrect exchange rate. Consequently, later withdrawers could take fewer assets, even none at all.

Proof of Concept

Let's consider following scenario:

1. The price of stETH is 1. And following users deposit assets and receive ezETH.
   • Alice: 100 stETH, 100 ezETH,
   • Bob: 96 ETH, 96 ezETH,
   • Charlie: 4 ETH, 4 ezETH. Then the state is:
   • total supply of ezETH: 200,
   • TVL: 200,
   • price of ezETH: 1.
2. Alice makes a transaction to withdraw stETH with her 100 ezETH, then she can claim 100 stETH after a delay. And now, the state doesn't change yet.
3. The price of stETH increases to 1.1. Then the state becomes:
   • total supply of ezETH: 200,
   • TVL: 100 * 1.1 + 100 = 210,
   • price of ezETH: 210 / 200 = 1.05.
4. Bob makes a transaction to withdraw ETH with his 96 ezETH, then he can claim 1.05 * 96 = 100 ETH after a delay.
5. After the cooldown periods, Alice claims 100 stETH and Bob claims 100 ETH. Then the state becomes:
   • total supply of ezETH: 4,
   • TVL: 0.

Finally, Charlie can take nothing even though he has 4 ezETH.

This problem occurs because the burnning ezETH is at L299 of the claim() function, not in the withdraw function so that assets to be claimed still participate to TVL and impact the exchange rate.

Tools Used

Manual review

Recommended Mitigation Steps

Burnning ezETH should be in the withdraw function, not in the claim() function. And, RestakeManager.calculateTVLs() function should consider the requested withdrawal amounts.

    function withdraw(uint256 _amount, address _assetOut) external nonReentrant {
        [...]

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

        [...]
    }

    function claim(uint256 withdrawRequestIndex) external nonReentrant {
        [...]

        // burn ezETH locked for withdraw request
-       ezETH.burn(address(this), _withdrawRequest.ezETHLocked);

        [...]
    }

    function calculateTVLs() public view returns (uint256[][] memory, uint256[] memory, uint256) {
        [...]

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

        [...]

        // Add native ETH help in withdraw Queue and totalWithdrawalQueueValue to totalTVL
-       totalTVL += (address(withdrawQueue).balance + totalWithdrawalQueueValue);
+       totalTVL += (
+                   IWithdrawQueue(withdrawQueue).getAvailableToWithdraw(IS_NATIVE) +
+                   totalWithdrawalQueueValue
+               );

        return (operatorDelegatorTokenTVLs, operatorDelegatorTVLs, totalTVL);
    }

Assessed type

Context

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/RestakeManager.sol#L274-L358

Vulnerability details

Impact

Incorrect calculation of totalWithdrawalQueueValue results in incorrect return value of the calculateTVLs() function, will impact the exchange rate of ezETH and result in incorrect actions throughout the entire protocol.

Proof of Concept

The totalWithdrawalQueueValue is added by the value of collateral tokens in WithdrawQueue contract by looping all collateral tokens. However as you can see at L318, at the j th loop, the first parameter of the function lookupTokenValue() is the i th collateral token, not the j th collateral token. As a result, totalWithdrawalQueueValue is differ from it should be, leading to incorrect return value of calculateTVLs() and incorrect action of the entire system.

    function calculateTVLs() public view returns (uint256[][] memory, uint256[] memory, uint256) {
        [...]

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

        [...]
    }

Tools Used

Manual review

Recommended Mitigation Steps

The index i should be replaced to j.

    function calculateTVLs() public view returns (uint256[][] memory, uint256[] memory, uint256) {
        [...]

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

        [...]
    }

Assessed type

Context

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/RestakeManager.sol#L491-L576

Vulnerability details

Impact

No zero amount check could result in unfair reversal of the transaction.

Proof of Concept

At L562 of the RestakeManager.deposit() function, if _amount = 0 then it will be reverted. It is quite possible for the _amount to become 0. In fact, at the begining of the RestakeManager.deposit() function, the _amount is not 0. However, during filling buffer, the _amount could become 0 at L549. Then, the whole transaction will be reverted.

    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
549         _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
562     operatorDelegator.deposit(_collateralToken, _amount);

        [...]
    }

Tools Used

Manual review

Recommended Mitigation Steps

There should be a zero amount check before depositting to operatorDelegator.

    function deposit(
        IERC20 _collateralToken,
        uint256 _amount,
        uint256 _referralId
    ) public nonReentrant notPaused {
        [...]

        // Call deposit on the operator delegator
+       if(_amount > 0) {
562         operatorDelegator.deposit(_collateralToken, _amount);
+       }

        [...]
    }

Assessed type

DoS

Judge has assessed an item in Issue #50 as 2 risk. The relevant finding follows:

[L-06] When removing a collateral token, it should validate if the token is not used across all operator delegators

[L-01] _refundGas function in DepositQueue contract should use tx.origin instead of msg.sender

If the caller is the contract, the actual gas payer is tx.origin, thus the gas should be refunded to tx.origin

[L-02] _recordGas function in OperatorDelegator contract should use tx.origin instead of msg.sender

Same reason as L-01

[L-03] RenzoOracleL2 should check for negative price returned

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Bridge/L2/Oracle/RenzoOracleL2.sol#L54 In getMintRate function, it only checks price staleness without negativity. If price is negative, it returns a huge number because it is converted into uint256.

[L-04] Incorrect total rewards calculation in RestakeManager contract

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/RestakeManager.sol#L700 In the code snippet above, the totalRewards adds the ETH balance of EigenLayer pods, this is incorrect because the ETH balance also includes the withdrawn balance of exited operators. It should be operatorDelegators[i].eigenPod().nonRestakedBalance

[L-05] When removing an operator delegator, it should validate if the operator delegator has no assets staked.

Even though this is called by an admin, non validating if operator delegator is completely exit leads to incorrect calculation of TVL

[L-06] When removing a collateral token, it should validate if the token is not used across all operator delegators

Same reason as L-05

[L-07] Missing zero address check in initialize function of RestakeManager contract

[L-08] sweepERC20 function of DepositQueue contract does not fill withdraw buffer

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Deposits/DepositQueue.sol#L254-L277 The function DepositQueue.sweepERC20() use reward tokens only to redeposit to the operator delegators, not to fill buffer.

[L-09] No token match check in OperatorDelegator.setTokenStrategy() function

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/RestakeManager.sol#L106-L114 There is no check for the _token to match the underlying token of the _strategy in the setTokenStrategy() function. If the strategy of the given token is set incorrectly, then depositting that collateral token will always be reverted.