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

DepositQueue doesn't collect fees from execution layer rewards #498

Open
howlbot-integration bot opened this issue May 10, 2024 · 6 comments
Open
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-a primary issue Highest quality submission among a set of duplicates QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_primary AI based primary recommendation 🤖_230_group AI based duplicate group recommendation sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue 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#L163-L183

Vulnerability details

The DepositQueue::receive() function is supposed to:

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

The function is expected to collect a fee from the rewards, fill the WithdrawQueue withdraw buffer if necessary and keep the remaining of the rewards in the contract:

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);
}

However, execution layer rewards don't trigger the receive() functions of smart contracts, the balance just gets updated and no code gets executed. Because of this the DepositQueue contract will not take a fee from execution layer rewards.

Impact

The DepositQueue contract will not collect a fee on execution layer rewards.

Recommended Mitigation Steps

Use a dedicated contract to receive execution layer rewards. In that contract add an external function that allows to send the collected rewards to the DepositQueue, which will trigger the receive() function as expected and collect the appropriate fees.

Assessed type

ETH-Transfer

@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 🤖_primary AI based primary 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
@jatinj615 jatinj615 added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label May 13, 2024
@jatinj615
Copy link

The execution layer rewards come in the RewardsHandler contract which are then forwarded to depositQueue periodically by NativeEthRestakeAdmin.

@C4-Staff C4-Staff added the primary issue Highest quality submission among a set of duplicates label May 15, 2024
@C4-Staff
Copy link

CloudEllie marked the issue as primary issue

@alcueca
Copy link

alcueca commented May 17, 2024

The README specifies that the DepositQueue contract is supposed to receive the Execution Layer Rewards. It doesn't, but the codebase has an alternative way to allow the functionality to happen. The issue is then downgraded to Low (QA) because it is just a deviation from specifications with no lasting impact.

@c4-judge
Copy link
Contributor

alcueca changed the severity to QA (Quality Assurance)

@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 grade-a 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 marked the issue as grade-a

@c4-judge c4-judge removed the grade-a label May 24, 2024
@c4-judge
Copy link
Contributor

alcueca marked the issue as grade-b

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 grade-a primary issue Highest quality submission among a set of duplicates QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_primary AI based primary recommendation 🤖_230_group AI based duplicate group recommendation sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

4 participants