Skip to content
This repository has been archived by the owner on Oct 13, 2024. It is now read-only.

IllIllI - L1 data fees are not reimbursed #57

Open
sherlock-admin4 opened this issue Apr 13, 2024 · 7 comments
Open

IllIllI - L1 data fees are not reimbursed #57

sherlock-admin4 opened this issue Apr 13, 2024 · 7 comments
Labels
Has Duplicates A valid issue with 1+ other issues describing the same vulnerability Medium A valid Medium severity issue Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Won't Fix The sponsor confirmed this issue will not be fixed

Comments

@sherlock-admin4
Copy link

sherlock-admin4 commented Apr 13, 2024

IllIllI

medium

L1 data fees are not reimbursed

Summary

L1 data fees are not reimbursed, and they are often orders of magnitude more expensive than the L2 gas fees being reimbursed. Not reimbursing these fees will lead to jobs not being executed on L2s.

Vulnerability Detail

While the contest README says that the protocol is interested in all EVM-compatible chains, its not clear to what extent they need compatibility. For instance, many chains have workarounds for the fact that they need to publish data from the L2 to the L1 via calldata, and therefore charge for those operations directly. Such caveats indicate that the compatibility is not 100% and therefore we can't really assume that any random chain will be supported. However, the README explicitly mentions Optimism as a target chain, so its idiosyncracies are in-scope.

The Gelato protocol properly handles L1 data fees, but neither Keep3r nor OpenRelay do. It could be argued that for Keep3r, it's possible to modify a job's fee rate dynamically and therefore it's not that big a risk, but for OpenRelay, the reimbursement formula is hard-coded, which means there's no workaround.

Looking at a transaction from this vault, the transaction cost was 0.000305237594921218 ETH ($1.17) but only 0.00000008929694256 ETH (<$0.01) was reimbursed. The L1 data fee was 0.000304676890061656 Eth, whereas the gas fee was 0.000000111805735391.

Impact

As is shown in this analysis for another contest, the two fees do not have a fixed ratio, and the L1 data fee is frequently much larger than the L2 gas fee, for extended periods of time. This essentially means that the protocol is broken on L2s for any use case which requires timely executions. The contest README states that the sponsor is interested in issues where future integrations would be negatively impacted, and one such case would be where an exchange is trying to use xkeeper to handle customer operations, as is outlined at the end of the linked-to comment above. Operations will appear to hang for multiple hours at a time, causing loss of funds for customers trying to close their orders.

Code Snippet

Only the L2 gas is reimbursed, not any of the L1 data fees:

// File: solidity/contracts/relays/OpenRelay.sol : OpenRelay.exec()   #1

28        // Execute the automation vault counting the gas spent
29        uint256 _initialGas = gasleft();
30        _automationVault.exec(msg.sender, _execData, new IAutomationVault.FeeData[](0));
31        uint256 _gasSpent = _initialGas - gasleft();
32    
33        // Calculate the payment for the relayer
34        uint256 _payment = (_gasSpent + GAS_BONUS) * block.basefee * GAS_MULTIPLIER / BASE;
35    
36        // Send the payment to the relayer
37        IAutomationVault.FeeData[] memory _feeData = new IAutomationVault.FeeData[](1);
38        _feeData[0] = IAutomationVault.FeeData(_feeRecipient, _NATIVE_TOKEN, _payment);
39:       _automationVault.exec(msg.sender, new IAutomationVault.ExecData[](0), _feeData);

https://github.com/sherlock-audit/2024-04-xkeeper/blob/main/xkeeper-core/solidity/contracts/relays/OpenRelay.sol#L28-L39

Tool used

Manual Review

Recommendation

Every L2 has its own formula for calculating the L1 data fee, so different versions of the code will have to be written for each L2. This is the description for Optimism. Note that the Ecotone upgrade has occurred, so be sure to implement based on those or these instructions.

@github-actions github-actions bot added the Excluded Excluded by the judge without consulting the protocol or the senior label Apr 16, 2024
@sherlock-admin2
Copy link
Contributor

1 comment(s) were left on this issue during the judging contest.

Kose commented:

medium

@Hash01011122 Hash01011122 added Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label and removed Excluded Excluded by the judge without consulting the protocol or the senior labels Apr 18, 2024
@Hash01011122 Hash01011122 added Medium A valid Medium severity issue Has Duplicates A valid issue with 1+ other issues describing the same vulnerability and removed Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label labels Apr 18, 2024
@Hash01011122 Hash01011122 reopened this Apr 18, 2024
@sherlock-admin3 sherlock-admin3 changed the title Restless Tiger Ape - L1 data fees are not reimbursed IllIllI - L1 data fees are not reimbursed Apr 24, 2024
@sherlock-admin3 sherlock-admin3 added the Reward A payout will be made for this issue label Apr 24, 2024
@realfugazzi
Copy link

Why is this an issue? This is not the responsibility of the OpenRelay contract.

@adam-idarrha
Copy link

@realfugazzi because that contract is in scope , and written by xkeeper .

@realfugazzi
Copy link

@IllIllI000 how is this not a feature request, according to your logic?

You are stating that L1 fees are not reimbursed, but there is no place where this is posted as protocol requirement. Job executioners will simulate the op and see if reimbursement is fit for them. Looks like a feature request then, correct?

@ashitakah
Copy link

I agree, it is true that the payments in L2 would be too low to incentivize the work and we are working on improving the system, but it is not a vulnerability and continues to pay although in a residual way.

@IllIllI000
Copy link
Collaborator

@ashitakah isn't "too low to incentivize the work" a future integration issue for any project looking to use xkeeper on an L2?

@BoRonG0d
Copy link

Contest readme explicitly states that the contract will be deployed on OP mainnet, and ALL information watson received during the competition did not indicate that OpenRelay would be used as future integration. So this is a valid issue.

And, this was judged as high in the past:

sherlock-audit/2023-07-perennial-judging#91

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Has Duplicates A valid issue with 1+ other issues describing the same vulnerability Medium A valid Medium severity issue Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Won't Fix The sponsor confirmed this issue will not be fixed
Projects
None yet
Development

No branches or pull requests

9 participants