sherlock-audit / 2024-04-xkeeper-judging

3 stars 4 forks source link

IllIllI - L1 data fees are not reimbursed #57

Open sherlock-admin4 opened 5 months ago

sherlock-admin4 commented 5 months ago

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.

sherlock-admin2 commented 5 months ago

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

Kose commented:

medium

realfugazzi commented 5 months ago

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

adam-idarrha commented 5 months ago

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

realfugazzi commented 5 months ago

@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 commented 5 months ago

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

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

BoRonG0d commented 5 months ago

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:

https://github.com/sherlock-audit/2023-07-perennial-judging/issues/91