search

code-423n4 / 2024-06-renzo-mitigation-findings

0 stars 0 forks source link

OperatorDelegator will leak ezETH yield due to integrating an inconsistent gas refunds mechanism #28

Closed c4-bot-9 closed 3 months ago

c4-bot-9 commented 3 months ago

Lines of code

https://github.com/Renzo-Protocol/Contracts/blob/ez-withdraw/contracts/Delegation/OperatorDelegator.sol#L323 https://github.com/Renzo-Protocol/Contracts/blob/ez-withdraw/contracts/Delegation/OperatorDelegator.sol#L399 https://github.com/Renzo-Protocol/Contracts/blob/ez-withdraw/contracts/Delegation/OperatorDelegator.sol#L502

Vulnerability details

Proof of Concept

NB: During the mitigation review, I spent time going across the findings repo to see if there is any valid findings the sponsors might have missed due to wrong duplications or misunderstanding from the validators. Now see: https://github.com/code-423n4/2024-04-renzo-findings/issues/84, the sponsor didn't go through this issue due to it's initial wrong duplication tag duplicate-81, considering it wasn't the primary. Now the scoping rules for this mitigation review, explicitly states that disputed issues are OOS, however this bug case never got to the sponsor for them to review so it wasn't disputed by them which I assume makes this finding in-scope.

Now going through the issue, it indeed is valid and would lead to heavy leakage of ezETH yield.

In the OperatorDelegator contract : https://github.com/Renzo-Protocol/Contracts/blob/ez-withdraw/contracts/Delegation/OperatorDelegator.sol, the baseGasAmountSpent value is applied to all admin gas refunds recorded for methods like: queueWithdrawals(), completeQueuedWithdrawal(), verifyWithdrawalCredentials(), and verifyAndProcessWithdrawals(). However, the initial cost of transactions mostly depends on calldata size and is not the same for these methods/functions.

Checking Etherscan, we can see that baseGasAmountSpent is set to 1,350,000 gas: https://etherscan.io/address/0xbAf5f3A05BD7Af6f3a0BBA207803bf77e2657c8F#readProxyContract#F2, which in our case would match the base cost of verifyAndProcessWithdrawals ( for example see the gas profiling for the linked example transaction from the previous finding). This example transaction has 101,952 bytes of calldata and pays 1,330,000 gas in initial costs, which is short of just 20,000 to match the baseGasAmountSpent. So if this periodic transaction is to properly be refunded, it must continue using the same value. t

However, issue with this implementation is that within the queueWithdrawals and completeQueuedWithdrawal methods which will be called periodically to process withdrawals, they will have a much much lower initial gas cost. But protocol still have their gas expense being recorded using the same code with the large baseGasAmountSpent value, causing the admin gas refunds for these methods to be overestimated. The gasSpent will be inflated by almost 1.3M gas for each transaction.

See where protocol records the expense for these calls:

And protocol still sets the same high gas cost for the not-so gas hefty call when verifying the the withdrawal credentials via verifyWithdrawalCredentials(), asides queueWithdrawals() & completeQueuedWithdrawal().

Impact

This bug case leads to refunds being over-refunded from the received rewards, which results in an unfair loss of yield for ezETH holders, since the refunds are deducted from the incoming rewards: https://github.com/Renzo-Protocol/Contracts/blob/ez-withdraw/contracts/Delegation/OperatorDelegator.sol#L625-L629

            // considered as protocol reward
            uint256 gasRefunded = 0;
            uint256 remainingAmount = address(this).balance;
            if (adminGasSpentInWei[tx.origin] > 0) {
                gasRefunded = _refundGas();
                // update the remaining amount
                remainingAmount -= gasRefunded;

The previous issue, provided an exquisite explanation on the amount of yield that could be lost, quote on quote: " Calculating the real base cost for queueWithdrawals:

Similarly, completeQueuedWithdrawal also requires a different baseGasAmountSpent, which is also much lower than currently set. Its calldata length is roughly 676 bytes, which is also two orders of magnitude shorter than the cost for verifyAndProcessWithdrawals (at around 100,000 bytes).

This is also true for verifyWithdrawalCredentials, but it is unlikely to be called periodically, so it is not as important. "

As hinted, even the callDataLength of the verifyWithdrawalCredentials() function is way way lower than that of the verifyAndProcessWithdrawals() that has ~ 100,000 bytes as shown in the example tx considering the processing of these withdrawals attached, the verifyWithdrawalCredentials() then requires a lower value of baseGasAmountSpent which then means that in our case, every instance of querying verifyWithdrawalCredentials() heavily over inflates the refunded gas and directly reflects on lesser yield for ezETH holders.

All the above showcase how this would result in a loss of yield for the ezETH holders.

Tool used

As hinted in the previous finding, consider removing the use of baseGasAmountSpent in the calculation for verifyWithdrawalCredentials(), but keep it for the verifyAndProcessWithdrawals that has expensive intrinsic costs. So introduce a nominal gas cost and 

+    uint256 internal constant NOMINAL_GAS_BASE_COST = 50_000;

-    function _recordGas(uint256 initialGas) internal {
-       uint256 gasSpent = (initialGas - gasleft() + baseGasAmountSpent) * tx.gasprice;
+    function _recordGas(uint256 initialGas, uint256 baseGas) internal {
+        uint256 gasSpent = (initialGas - gasleft() + baseGas) * tx.gasprice;

..

function verifyWithdrawalCredentials(
        ..
-       _recordGas(gasBefore);
+       _recordGas(gasBefore, NOMINAL_GAS_BASE_COST);

..

    function verifyAndProcessWithdrawals(
        ..
-       _recordGas(gasBefore);
+       _recordGas(gasBefore, baseGasAmountSpent);

..

    function queueWithdrawals(
        ..
-       _recordGas(gasBefore);
+       _recordGas(gasBefore, NOMINAL_GAS_BASE_COST);

..

    function completeQueuedWithdrawal(
        ..
-       _recordGas(gasBefore);
+       _recordGas(gasBefore, NOMINAL_GAS_BASE_COST);

Assessed type

Context

alcueca commented 3 months ago

Unfortunately, findings that were overlooked during the parent review are out of scope, and wardens are encouraged to forward them to the sponsor externally, for example through a bug bounty.

From the Guidelines for Mitigation Reviews, What Not To Include section:

Vulnerabilities submitted during the preceding audit should not be submitted as new HM issues during the mitigation review. They will be considered out of scope and ineligible for awards. If a warden feels an issue from the preceding audit was overlooked or undervalued, it is recommended to look into submitting it to the sponsor via other channels (e.g., a bug bounty program).

c4-judge commented 3 months ago

alcueca marked the issue as unsatisfactory: Out of scope