code-423n4 / 2024-04-renzo-findings

12 stars 8 forks source link

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

Open howlbot-integration[bot] opened 6 months ago

howlbot-integration[bot] commented 6 months ago

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

jatinj615 commented 6 months ago

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

C4-Staff commented 6 months ago

CloudEllie marked the issue as primary issue

alcueca commented 6 months ago

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 commented 6 months ago

alcueca changed the severity to QA (Quality Assurance)

c4-judge commented 6 months ago

alcueca marked the issue as grade-a

c4-judge commented 6 months ago

alcueca marked the issue as grade-b