IllIllI - OpenRelay relay does not properly compensate jobs with small gas usages

[sherlock-admin2]

IllIllI

medium

OpenRelay relay does not properly compensate jobs with small gas usages

Summary

The OpenRelay relay does not account for the base cost of transactions, nor for the gas cost of the separate call to reimburse the keeper

Vulnerability Detail

The gas cost reimbursed is solely the cost of the external call to execute the job, which can be only a small part of the transaction's cost. For example, for this transactions for this vault, the transaction fee was 0.000160418704443228 ETH, but only 0.000087537325119585 ETH was reimbursed. Looking at the Gas Profiler section of the Phalcon report for this transaction, the first exec() call (the one that gets reimbursed) accounts for only 18975 / 65142 = ~30% of the full transaction's cost. The rest of the cost comes from the 16 gas/byte of the transaction calldata, the Gtransaction (21000 gas) cost, as well as the execution cost of the external call to issue the refund.

Impact

Because there's a multiplier on the raw amount of gas spent, the larger the execution's gas cost, the more of the deficit is paid back. However, this eats into the intended 20% bonus, which means small jobs won't get executed, and large jobs won't get as big a refund as was intended.

Note that a more and more frequent use of keepers is with Pyth price updates, which significantly increase the amount of calldata. 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 operations that involve Pyth updates, but do little else. Such jobs will never be executed, because they will be unprofitable for keepers to execute.

Code Snippet

Only one of the external calls is reimbursed, and there is no accounting for the cost of calldata:

// 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

Include the calldata cost in the calculation, and also increase the GAS_BONUS to account for Gtransaction and the refund call's gas cost

[sherlock-admin4]

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

Kose commented:

It seems like GAS_BONUS is enough for costs other than main exec. The reason for the loss in given Phalcon report is because there is a priority fee in the transaction which is intentionally not used in calculation as mentioned by README. Invalid.

[IllIllI000]

Even ignoring the priority fee, it's still possible for the calldata cost to exceed the GAS_BONUS. Extending the above example, the amount reimbursed is proportional to (50_000+18_975)*1.2 = 82_770, and once the calldata reaches 72 words (e.g. twice the number of Pyth updates this transaction does), the non-reimbursed cost will be 65_142-18_975+(16*72*32)=83_031, which is more than the refund. I recognize that this is a lot of calldata, for a relatively small job, but this is a possible future integration issue.

[IllIllI000]

Escalate

see above

[sherlock-admin2]

Escalate

see above

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[Hash01011122]

@IllIllI000 Gas BONUS in the range 20% to 30% will be considered as low to medium impact for protocol, moreover the likelyhood of this happening is pretty low. I am still considering this as low severity issue. I hope I am making sense here. @IllIllI000 Do you want to add anything?

[IllIllI000]

If the reward is smaller than the cost, the job will not be executed, which means someone interested in integrating/using the xkeeper protocol with this sort of job will be unable to. Likelyhood does not appear in Sherlock's rules anywhere

[cvetanovv]

To me, this seems like a typical Low severity issue. The chances of this happening are small and furthermore, the value of GAS_BONUS in almost all cases is quite sufficient. The impact is also Low.

Likelihood/Impact are not mentioned as specific rules, but don't we still have to judge in some way whether issues are Invalid/Low/Medium/High?

[IllIllI000]

Again, likelyhood/chances of happening does not appear anywhere in the rules, and this is an issue for non-functional future integrations, not a loss of funds issue. The 'future integration issue' readme directive is relatively new, and the sponsor just wrote Yes., so that means to me that I'm supposed to submit everything possible, no matter how small, since we only have the README to go by, not sponsor comments after the fact. This org issue seems to indicate that the goal is to move towards rulings where anything the sponsor lists as important in the readme, should automatically be Medium, but I honestly don't know the current limits.

[cvetanovv]

About the 'future integration issue', I didn't say I didn't think they were invalid. They will be considered valid for this contest because the protocol has written in the Readme that they want to know about them, and the Readme overrides Sherlock docs.

I'm trying to figure out how much the amount of calldata can actually increase. If we take into account a high value of calldata then the issue becomes Medium, but for the moment it's Low.

@Hash01011122 What do you think?

[IllIllI000]

The upper limit of calldata is the block gas size, which is much larger than the amount required to hit this issue. Why would that not be the limit used for this issue?

[realfugazzi]

Aside from the unlikelihood of the scenario, there is no security issue here. Executors are expected to validate the profitability of their actions, as mentioned elsewhere nobody is forcing keepers.

[Hash01011122]

@cvetanovv, I stand by my previous assessment that this is a low severity issue. The likelihood of this scenario occurring is negligible, and the impact, involving only a 20%-30% gas inflation, is considered low.

[cvetanovv]

I stand by my previous comments and the Lead Judge opinion that this is a Low severity issue. The impact and likelihood are definitely Low. The comment that we should count the upper limit of calldata to be block gas size is very unrealistic.

Planning to reject the escalation and leave the issue as is.

[IllIllI000]

@Evert0x can you respond to the points in this comment, since the core of the issue is not being addressed (future integration issue, with no constraints)?: https://github.com/sherlock-audit/2024-04-xkeeper-judging/issues/58#issuecomment-2081573175

[cvetanovv]

@Evert0x can you respond to the points in this comment, since the core of the issue is not being addressed (future integration issue, with no constraints)?: #58 (comment)

@IllIllI000 Point out to me where it is written: future integration issue, with no constraints.

[IllIllI000]

@cvetanovv Yes. has no constraints. Further, the issue does not require a full block of calldata - it only requires twice the amount in a real on-chain transaction.

[cvetanovv]

Despite this, I still think is Low severity. The readme says that the protocol wants to know about future integration problems, without specifying what exactly this is about. We still need to figure out what the severity is this issue, and not automatically become Medium. I agree that this is a future valid issue and judge it as Low. But to be most fair, I'll also ask what the protocol thinks.

[IllIllI000]

As mentioned, sponsor comments after the fact do not figure into the validity of issues submitted during the contest. Regardless of what they say, I'd still like to hear from Evert on this escalation, given what was done here, the intentions described in the org issue, and the fact that the readme overrides the judging rules according to the current docs.

[ashitakah]

Regarding the team's perspective, we have been using this same math on keep3r accounts for years and it has never been a problem. While it is true that we assume that on L2 we could develop too small payouts and we are working to implement a v2, in all other cases in normal situations the bonus plus the payout would be more than enough. It is not done through oracles since it would increase the cost of gas too much. While I can come to agree as a low issue given that the math is not as precise as possible.

[Evert0x]

I believe it's fair to take into account the sponsors opinion as a last resort as the README answer was incomplete

Should potential issues, like broken assumptions about function behavior, be reported if they could pose risks in future integrations, even if they might not be an issue in the context of the scope? If yes, can you elaborate on properties/invariants that should hold?

The protocol team didn't elaborate so it's not possible to know what kind of future integrations should be reported.

This is a mistake on Sherlock end for not verifying the README contents properly, for that reason we can not count this negatively to the payout/escalation factors.

[IllIllI000]

@Evert0x Can you address how likelyhood factors into severity? My understanding is that it does not factor in at all, except to downgrade a High to a Med. Can you comment on what factors make this submission a Low according to Sherlock rules (I understand that the sponsor considers it a Low)? The sponsor is in fact fixing the issue as they mention, for the L2 scenario.

[Hash01011122]

I will respect whatever decision the head of judge will make for this issue.

[cvetanovv]

@Evert0x Can you address how likelyhood factors into severity? My understanding is that it does not factor in at all, except to downgrade a High to a Med. Can you comment on what factors make this submission a Low according to Sherlock rules (I understand that the sponsor considers it a Low)? The sponsor is in fact fixing the issue as they mention, for the L2 scenario.

There is no exact rule for determining likelihood. It has always been considered an unwritten rule when determining severity. Just because the sponsor fixed it doesn't mean it has to be Medium.

Remains my decision to reject the escalation.

[IllIllI000]

Unwritten rules are not rules, and recently the unwritten rule of not accepting known issues was reversed by Evert, so I really would like to hear from @Evert0x on why this is low. The readme said they're interested in integration issues, so I accepted the response that we check with the sponsor of whether it's an integration issue, and they confirmed that it was an integration issue (they're fixing it because the core contract functionality is broken for that use case), but thought that it was Low. In Sherlock contests, sponsors do not decide severity, so we need to follow the Sherlock rules, and since I submitted an escalation, I would like a response, since things seem to be being judged differently based on the contest, and nowhere in the rules does likelihood change a Medium to a Low. Also, this scenario seems a lot more likely than someone using the equivalent of a large fraction of bitcoin's hash rate to come up with half a hash collision, which has been a Med many times, so even using likelihood, there is some inconsistency here.

[cvetanovv]

You only look at Sherlock's rules letter by letter when it's to your advantage. The documentation says, "Historical decisions are not considered sources of truth." So, whatever examples you give of previous decisions they do not apply. The sponsor must also specify what future integration issues will be considered valid. Since it is not specified, then it is fair to take his opinion into consideration.

[IllIllI000]

Your response didn't really address any of the points I've made. As I said, I'm open to taking account what the sponsor has said, but it should also be based on the Sherlock rules, not a knee-jerk reaction from the judge, based on and unwritten rule that hasn't been regularly or recently applied (that I'm aware of). My point in linking the decision in the other issue was that unwritten rules do not really apply, hence why I'm asking for Evert to take a look at the arguments on both sides, so he can make the decision for Sherlock over all, not just this one case.

[IllIllI000]

The fairly straight forward question is, is likelihood a valid reason for downgrading a Med (core contract functionality is broken for that use case - a Sherlock rule, written in the rules) to a Low, when likelihood does not appear in the rules?

[Hash01011122]

@IllIllI000, Likelihood is indeed a tremendous factor to assign severity to any issue. If an attack occurence is negligible than why should it be of med/high severity?

Also please refrain to comment same statement again and again which leads to same cyclic loop of conversation.

[IllIllI000]

A High can be downgraded to Med solely because there are limiting factors, and this is mentioned specifically in the rules. The scenario matches a Med, is not infinitesimally unlikely, and in the submission and updates afterwards, I outlined why it's not all that unlikely, and why it's even more likely to happen going forward. The response was just that it's Low severity, without offering any analysis on likelihood, or how other Sherlock rules apply, which are normally what a judge would provide. I've tried multiple times to get this information about the likelihood rule, both in discord and here, but there hasn't been any argument provided, just the assertion that it's low. I don't see how that's a fair response to an escalation, so I would like to hear what @Evert0x has to say.

[Evert0x]

I don't believe this is an issue. It's a design recommendation. Something they plan on including for the next version.

It's true that specific type of jobs do not offer an optimal reimbursement. But this is a trade-off between complexity of the code and the implementation to match real life use cases.

As stated before, it's not a future integration issue as the README language wasn't clear.

[Evert0x]

Result: Invalid Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• IllIllI000: rejected