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

Admin refundable fees can be bypassed and all rewards can be sent to deposit queue #149

Open
howlbot-integration bot opened this issue May 9, 2024 · 10 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-148 edited-by-warden grade-b Q-47 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_primary AI based primary recommendation 🤖_36_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons 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/519e518f2d8dec9acf6482b84a181e403070d22d/contracts/Delegation/OperatorDelegator.sol#L481-L525

Vulnerability details

Impact

Validators execution rewards will be sent to operator delegator contract from the withdrawal router. When the ether is received in OperatorDelegator as a result of this, the tx.origin will be refunded as gas considering the tx.origin is the admin. However, initiating a claim in delayed withdrawal router in EigenLayer is permissionless which anyone can claim behalf of the operator delegator. Since the address that will claim is not the restaking admin of the operator delegator, all the funds will be sent to the deposit queue and admin will not be able to take the fees from it.

Proof of Concept

Admin records fees to be taken when:

  • queueWithdrawals
  • completeQueuedWithdrawal
  • verifyWithdrawalCredentials
  • verifyAndProcessWithdrawals

the recording of the fee is as follows:

function verifyAndProcessWithdrawals(
        uint64 oracleTimestamp,
        BeaconChainProofs.StateRootProof calldata stateRootProof,
        BeaconChainProofs.WithdrawalProof[] calldata withdrawalProofs,
        bytes[] calldata validatorFieldsProofs,
        bytes32[][] calldata validatorFields,
        bytes32[][] calldata withdrawalFields
    ) external onlyNativeEthRestakeAdmin {
        -> uint256 gasBefore = gasleft();
        eigenPod.verifyAndProcessWithdrawals(
            oracleTimestamp,
            stateRootProof,
            withdrawalProofs,
            validatorFieldsProofs,
            validatorFields,
            withdrawalFields
        );
        // update the gas spent for RestakeAdmin
        -> _recordGas(gasBefore);
    }

and this is the _recordGas implementation:

function _recordGas(uint256 initialGas) internal {
        uint256 gasSpent = (initialGas - gasleft() + baseGasAmountSpent) * tx.gasprice;
        adminGasSpentInWei[msg.sender] += gasSpent;
        emit GasSpent(msg.sender, gasSpent);
    }

since the admin can only call these functions, msg.sender will have the role restake admin role guaranteed.

Now, when and how the validator rewards claimed from EigenPod?
Beacon chain effective balance is capped to 32 ether anything that is above this can be partially withdrawn to the EigenPod owner with a delayed router. The funds will be sent to DelayedRouter and once the delay passes anyone can claim behalf of the account.

When the amounts are claimed from EigenLayer delayed router, ether will sent to the pod owner which is the OperatorDelegator contract as follows:
https://github.com/Layr-Labs/eigenlayer-contracts/blob/98685285e5e504fa6180c010d2835cd506c4ecc6/src/contracts/pods/DelayedWithdrawalRouter.sol#L216-L218

Since this is a plain ether transfer, the receive() function will be triggered in operator delegator contract:

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

since the tx.origin will be the random address that calls the claim in delayed router behalf of the operator delegator contract the adminGasSpentInWei[tx.origin] will be 0. Hence, there won't be any fees taken and all the claimed amount will be sent to deposit queue.

In result, admin can never receive its gas fees since users are incentivized to call claim themselves to bypass the admin fee and receive more rewards which increases ezETH exchange rate further.

Tools Used

Recommended Mitigation Steps

Regardless of the tx.origin, take the fees. Otherwise, users are well encouraged and incentivized to call claim themselves.

Assessed type

Other

@howlbot-integration howlbot-integration bot added 3 (High Risk) Assets can be stolen/lost/compromised directly 🤖_36_group AI based duplicate group recommendation 🤖_primary AI based primary recommendation bug Something isn't working edited-by-warden sufficient quality report This report is of sufficient quality labels May 9, 2024
howlbot-integration bot added a commit that referenced this issue May 9, 2024
@jatinj615
Copy link

jatinj615 commented May 14, 2024

Expected Behaviour. As the protocol does not guarantee that gas fee are refunded but just a best effort to cover for proof submission cost.

@jatinj615 jatinj615 added the sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons label May 14, 2024
@C4-Staff
Copy link

CloudEllie marked the issue as primary issue

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

alcueca commented May 16, 2024

Judging from the code and documentation, it seems to me that the protocol expected to get refunded that amount, otherwise the code could just be removed. Medium severity because the loss is not to general users but to the admin account, and the amounts are small.

@c4-judge c4-judge added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value downgraded by judge Judge downgraded the risk level of this issue and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels May 16, 2024
@c4-judge
Copy link
Contributor

alcueca changed the severity to 2 (Med Risk)

@c4-judge
Copy link
Contributor

alcueca marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label May 16, 2024
@c4-judge
Copy link
Contributor

alcueca marked issue #148 as primary and marked this issue as a duplicate of 148

@c4-judge c4-judge added duplicate-148 and removed primary issue Highest quality submission among a set of duplicates labels May 16, 2024
@s1n1st3r01
Copy link

s1n1st3r01 commented May 25, 2024

Hey @alcueca

I believe this issue to be a QA rather than M for the following reasons:

  1. Gas refund amounts are never lost. Even if an attacker claims a delayed withdrawal on behalf an admin, the admin's refund gas amount isn't gone but rather it will remain still in the adminGasSpentInWei state variable (which is incremented by _recordGas() when the admin requests a withdrawal). So in other words, the next time the admin claims a withdrawal, he'll receive the gas funds which he should've received the previous time the attacker called claimDelayedWithdrawals(). In short, no funds / gas refunds are lost.

  2. There is no real incentive for a user to execute this attack. The gas costs of the call to claimDelayedWithdrawals() are overwhelmingly likely to be higher than the increased value in user tokens from adding the to-be-refunded admin gas cost to the TVL

So the fact that there are zero admin/user funds lost, attacker gains nothing from this and the functionality of the protocol isn't affected by this and the sponsor choosing to not fix this, makes me strongly believe this (and its duplicates) is a QA. @jatinj615 Would appreciate your input on my thoughts.

@jatinj615
Copy link

@s1n1st3r0 , as I specified in the comment this is just a best effort by the protocol to refund some gas to trusted admins performing proof verifications.
Yes, attacker does not gain anything from this. and user funds are not at risk. It just stops admin account from getting the gas refunds.
It is also correct that max possible refund will be given in any of the claim performed by admin for their gas spent.
In any case admins will still keep performing the proof verification.
Hence, acknowledged. Can be moved to QA.
up to ser @alcueca .

@c4-judge
Copy link
Contributor

alcueca changed the severity to QA (Quality Assurance)

@c4-judge c4-judge added 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 27, 2024
@c4-judge
Copy link
Contributor

alcueca marked the issue as grade-b

@C4-Staff C4-Staff reopened this Jun 3, 2024
@C4-Staff C4-Staff added the Q-47 label 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-148 edited-by-warden grade-b Q-47 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_primary AI based primary recommendation 🤖_36_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

5 participants