Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Execution Layer rewards are lost #499

Open
howlbot-integration bot opened this issue May 10, 2024 · 2 comments
Open

Execution Layer rewards are lost #499

howlbot-integration bot opened this issue May 10, 2024 · 2 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-498 grade-a QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_230_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality

Comments

@howlbot-integration
Copy link

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Deposits/DepositQueue.sol#L158-L184
https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Delegation/OperatorDelegator.sol#L494-L525

Vulnerability details

Impact

According to comment in DepositQueue: "The Deposit Queue contract has the ability to receive ETH via the Ethereum Execution Layer.However, this is only true for EigenPod rewards. Execution Layer rewards are not accounted for and lost.

This should receive ETH from scenarios like Execution Layer Rewards and MEV from native staking

Proof of Concept

Execution Layer rewards are not distributed through plain ETH transfers. Instead the balance of the block proposer fee recipient's address is directly updated. If the fee recipient getting the EL rewards is a smart contract, this means that the fallback/receive function is not called. Actually, a smart contract could receive EL rewards even if these functions are not defined.

    /// @dev Handle ETH sent to this contract from outside the protocol - e.g. rewards
    /// ETH will be stored here until used for a validator deposit
@-> /// This should receive ETH from scenarios like Execution Layer Rewards and MEV from native staking
    /// Users should NOT send ETH directly to this contract unless they want to donate to existing ezETH holders
    /// Checks the ETH withdraw Queue and fills up if required
    receive() external payable nonReentrant {
        uint256 feeAmount = 0;
        // Take protocol cut of rewards if enabled
        if (feeAddress != address(0x0) && feeBasisPoints > 0) {
            feeAmount = (msg.value * feeBasisPoints) / 10000;
            (bool success, ) = feeAddress.call{ value: feeAmount }("");
            if (!success) revert TransferFailed();

            emit ProtocolFeesPaid(IERC20(address(0x0)), feeAmount, feeAddress);
        }
        // update remaining rewards
        uint256 remainingRewards = msg.value - feeAmount;
        // Check and fill ETH withdraw buffer if required
        _checkAndFillETHWithdrawBuffer(remainingRewards);

        // Add to the total earned
        totalEarned[address(0x0)] = totalEarned[address(0x0)] + remainingRewards;

        // Emit the rewards event
        emit RewardsDeposited(IERC20(address(0x0)), remainingRewards);
    }

This is only true for EigenPod rewards and there is a receive function OperatorDelegator contract to handle eigenPod rewards. Execution Layer rewards are not accounted for and lost.

    /**
     * @notice Users should NOT send ETH directly to this contract unless they want to donate to existing ezETH holders.
     *        This is an internal protocol function.
     * @dev Handle ETH sent to this contract - will get forwarded to the deposit queue for restaking as a protocol reward
     * @dev If msg.sender is eigenPod then forward ETH to deposit queue without taking cut (i.e. full withdrawal from beacon chain)
     */
    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);
        }
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Modify the receive() function in the DepositQueue contract to handle EL rewards. This can be achieved by checking the transaction context and identifying EL reward deposits.

Assessed type

Other

@howlbot-integration howlbot-integration bot added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value 🤖_230_group AI based duplicate group recommendation bug Something isn't working sufficient quality report This report is of sufficient quality labels May 10, 2024
howlbot-integration bot added a commit that referenced this issue May 10, 2024
@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels May 17, 2024
@c4-judge
Copy link
Contributor

alcueca changed the severity to QA (Quality Assurance)

@c4-judge
Copy link
Contributor

alcueca marked the issue as grade-a

@C4-Staff C4-Staff reopened this Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-498 grade-a QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_230_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

4 participants