Renzo - Sabit's results

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Deposits/DepositQueue.sol#L193 https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Deposits/DepositQueue.sol#L284 https://github.com/code-423n4/2024-04-renzo/blob/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Deposits/DepositQueue.sol#L217

Vulnerability details

Impact

The contract would refund more Ether to the external caller (admin) than they actually spent on gas. This overpayment of gas refunds would effectively drain the contract's balance unnecessarily.

Proof of Concept

The stakeEthFromQueue function in the provided code has a bug in the way it calculates the gas used by the admin caller (msg.sender) for the purpose of refunding gas. The current implementation does not accurately measure the gas used by the admin caller and instead refunds gas based on the contract's internal gas consumption.

 function stakeEthFromQueue(
        IOperatorDelegator operatorDelegator,
        bytes calldata pubkey,
        bytes calldata signature,
        bytes32 depositDataRoot
    ) external onlyNativeEthRestakeAdmin {
        uint256 gasBefore = gasleft();
        // Send the ETH and the params through to the restake manager
        restakeManager.stakeEthInOperatorDelegator{ value: 32 ether }(
            operatorDelegator,
            pubkey,
            signature,
            depositDataRoot
        );

        emit ETHStakedFromQueue(operatorDelegator, pubkey, 32 ether, address(this).balance);

        // Refund the gas to the Admin address if enough ETH available
        _refundGas(gasBefore);
    }


function _refundGas(uint256 initialGas) internal {
    uint256 gasUsed = (initialGas - gasleft()) * tx.gasprice;
    uint256 gasRefund = address(this).balance >= gasUsed ? gasUsed : address(this).balance;
    (bool success, ) = payable(msg.sender).call{ value: gasRefund }("");
    if (!success) revert TransferFailed();
    emit GasRefunded(msg.sender, gasRefund);
}

The issue lies in the way the gas used by the admin caller (msg.sender) is calculated for the refund. The current implementation assigns gasBefore before the call to restakeManager.stakeEthInOperatorDelegator, and then passes this value to the _refundGas function. Inside _refundGas, the gas used is calculated as initialGas - gasleft().

However, this calculation does not accurately reflect the gas used by the admin caller (msg.sender). Instead, it calculates the gas used by the contract itself when executing the internal _refundGas function, which is not the intended behavior.

Here's a practical scenerio:

• The external caller (admin) provides an initial gas allowance when calling the stakeEthFromQueue function.

• This gas allowance is used to execute the external function call up until the point where the contract's internal operations begin (e.g., the call to restakeManager.stakeEthInOperatorDelegator).

• After that point, any gas consumed by the contract's internal operations and function calls (e.g., _refundGas) is paid for by the contract itself, using its own balance.

• The gas refund is intended to compensate the external caller (admin) for the gas used to initiate the external function call, not for the gas consumed by the contract's internal operations.

Similarly issue is in stakeEthFromQueueMulti function.

Tools Used

Manual review

Recommended Mitigation Steps

To accurately calculate the gas used by the admin caller (msg.sender) and refund the correct amount, the following changes should be made:

Move the assignment of gasBefore to the beginning of the stakeEthFromQueue function, before any other operations.

Capture the value of gasleft() immediately after the event emission (emit ETHStakedFromQueue) and store it in a new variable, let's call it gasAfter. Calculate the gas used by the admin caller (msg.sender) as gasBefore - gasAfter. Refund the calculated gas amount to msg.sender by sending (gasBefore - gasAfter) * tx.gasprice Ether from the contract's balance.

Here's the corrected code:

function stakeEthFromQueue(
    IOperatorDelegator operatorDelegator,
    bytes calldata pubkey,
    bytes calldata signature,
    bytes32 depositDataRoot
) external onlyNativeEthRestakeAdmin {
    uint256 gasBefore = gasleft();

    // Send the ETH and the params through to the restake manager
    restakeManager.stakeEthInOperatorDelegator{ value: 32 ether }(
        operatorDelegator,
        pubkey,
        signature,
        depositDataRoot
    );

    uint256 gasAfter = gasleft();
    emit ETHStakedFromQueue(operatorDelegator, pubkey, 32 ether, address(this).balance);

    // Refund the gas to the Admin address if enough ETH available
    uint256 gasUsedByAdmin = gasBefore - gasAfter * tx.gasprice;
    (bool success, ) = payable(msg.sender).call{ value: gasUsedByAdmin }("");
    if (!success) revert TransferFailed();
    emit GasRefunded(msg.sender, gasUsedByAdmin);
}

Assessed type

Context