bhilare_ - Executor can drain owner's funds deposited in the vault for payments.

[sherlock-admin3]

bhilare_

high

Executor can drain owner's funds deposited in the vault for payments.

Summary

If a job is being executed from an open relay, the executor can drain the funds of owner that are being deposited into vault for the fee payment.

Vulnerability Detail

When job is being exected from an OpenRelay, OpenRelay::exec() function is being used, in which the fee payment is being calculated using the _gasSpent, during the execution. And then the payment amount is being transferred from vault to the feeRecipient .

Now for this attack to happen there should be condition , that there should be a job which contains a low-level call to its users. Like for example , distributeRewards function, and the rewards are being distributed using a low-level call. (which is quite common).

So a executor can participate in that protocol and get himself involved in the rewards distribution process, where the low level call will be made to itself. Now if while calling OpenRelay::exec() , the executor would execute the job and then inside of it there would be a low-level call done, which would trigger the fallback/receive at the executor's contract.

Now the whole control is at the executor side, now the executor can do arbitrary things and included highle complex logic just to consume more gas and increase the _gasSpent, so that at the end when the AutomationVault::exec function call will be completed , there will be more _gasSpent than actually would have been used for the execution of job.

And like this, by increasing _gasSpent, at one point, the owner's funds can also be drained.

A similar finding was found by TRUST SECURITY in one of their audits : Trust Security Finding

Here Trust security has also stated about the gas being converted into popular gas tokens, like CHI, personally I tried getting knowledge of it, but because of lack of time was not able to fully gain it to explain about it here, if Judge & Sponsor Team would be having idea of it, then can discuss more on it, I just shared for better prevention.

Now here , team can say that This would be owner's fault that he approved for a job contain low-level call, actually it wouldn't be completely his fault, because it might happen that he might not know about such risk. And this was not stated anywhere in details hence submitted the finding.

From xkeeper's side this risk can be mitigated, hence would be better to avoid future drama.

Impact

Here, as stated above, if the executor can convert into gas tokens and then can profit from it , then it can implement this attack at a very little cost, just as stated in Trust Security's finding, and can fully drain the owner's funds in vault. Hence impact can be high. And hence severity can be HIGH.

But if this can't happen, then for all the complex logic at the receiver's end, the executor would have to pay for all the _gasSpent, but would ofc, get the fees more than the _gasSpent cost, since other fee paramters are being used to calculate the final payment.

So here there wouldn't be much of profit, but would be equal to payment - _gasSpent, which might not be much (or might be)

Because of which for the second case there can/cannot be chance that the attacker wouldn't have much incentive to do this. Hence for this case it can be a MEDIUM risk issue.

But still this doesn't states out the fact that owner's funds can be in risk.

Hence to avoid future drama as said before, this case should be mitigated.

Code Snippet

https://github.com/sherlock-audit/2024-04-xkeeper/blob/main/xkeeper-core/solidity/contracts/relays/OpenRelay.sol https://github.com/sherlock-audit/2024-04-xkeeper/blob/b82de26633c1904ccfb3c121497845128f1c5c5f/xkeeper-core/solidity/contracts/core/AutomationVault.sol#L397-L453

Tool used

Manual Review

Recommendation

For mitigating this risk, a hard cap for _gasSpent can be used, which if gets above the cap, then the function call should revert, as this much of gasSpent was not being expected.

But ofcourse various job implementation would have various different type of logic implemented, hence xkeeper can't fix a hard cap for this.

But while adding relayJobs, there can be a extra parameter being added in the _jobsData where the owner themselve provide the hard cap for _gasSpent, since they would be having idea that how much maximum gas can be spent for executing that particular job.

And then for that job, after execution, the _gasSpent can be compared with the cappedValue, if its greater than cappedValue, then make the function revert.

Which would prevent the risk of owner's funds being drained.

Duplicate of #4

[sherlock-admin2]

Escalate

This has been issued as Duplicate of #4

Where a comment was being made

Kose commented: The restrictions related to frequency of job execution should be implemented inside job contract. So it is job owner's responsibility to prevent such things to happen and it is not related to protocol directly. Invalid.

I feel my report has been overlooked, and directly being added as duplicate of #4

This issue is not a duplicate of #4

Here , the vulnerability exists in the way fees/payment has being calculated, and not the frequency of job execution or re-entering / repeatedly calling a job.

Also want to state again one thing :

Now here , team can say that This would be owner's fault that he approved for a job contain low-level call, actually it wouldn't be completely his fault, because it might happen that he might not know about such risk. And this was not stated anywhere in details hence submitted the finding.

From xkeeper's side this risk can be mitigated, hence would be better to avoid future drama.

And regarding severity also I have shared point where a similar finding was found by Trust Security in one of their audits, but I was unsure of the point of conversion of gas into gasToken like CHI, would be possible here or not, since I didn't had idea.

Regarding which also I have shared my opinions in the IMPACT section.

Hence please kindly look into my report once again, as it is not a duplicate of #4 And the root cause is different.

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.

[RealLTDingZhen]

Escalate

delegate to @Pranay-Bhilare

[sherlock-admin2]

Escalate

delegate to @Pranay-Bhilare

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.

[Pranay-Bhilare]

@Hash01011122 This has been issued as Duplicate of #4

Where a comment was being made

Kose commented: The restrictions related to frequency of job execution should be implemented inside job contract. So it is job owner's responsibility to prevent such things to happen and it is not related to protocol directly. Invalid.

I feel my report has been overlooked, and directly being added as duplicate of #4

This issue is not a duplicate of #4

Here , the vulnerability exists in the way fees/payment has being calculated, and not the frequency of job execution or re-entering / repeatedly calling a job.

Also want to state again one thing :

Now here , team can say that This would be owner's fault that he approved for a job contain low-level call, actually it wouldn't be completely his fault, because it might happen that he might not know about such risk. And this was not stated anywhere in details hence submitted the finding.

From xkeeper's side this risk can be mitigated, hence would be better to avoid future drama.

And regarding severity also I have shared point where a similar finding was found by Trust Security in one of their audits, but I was unsure of the point of conversion of gas into gasToken like CHI, would be possible here or not, since I didn't had idea.

Regarding which also I have shared my opinions in the IMPACT section.

Hence please kindly look into my report once again, as it is not a duplicate of #4 And the root cause is different.

To be honest, I am unable to make sense on what Watson here is trying to point out. @ashitakah it would be great if you can respond to this one. @cvetanovv if you want my opinion on this issue I will consider it as invalid because the root cause pointed out in this report:

If a job is being executed from an open relay, the executor can drain the funds of owner that are being deposited into vault for the fee payment.

The attack path mentioned doesn't show any valid attack vector. Watson just attached some finding of Trust security by matching pattern without narrowing down any attack surface.

It would be better to consult to sponsors regarding this. Tagging @ashitakah

[ashitakah]

I think that is the same person and issue https://github.com/sherlock-audit/2024-04-xkeeper-judging/issues/85 Im checking with the team if this is valid. As far I know even if the attack you describe were valid. In the end it would be calling different vaults that in turn would have to execute different jobs and it would be the same as passing an array of ExecData[] to execute different jobs and automation vaults. Even so we are investigating if this is possible.

[Pranay-Bhilare]

@Hash01011122 @ashitakah Sir the root cause here and in issue #85 are different, there I stated related to re-entrancy vulnerability in exec function.

if you want my opinion on this issue I will consider it as invalid because the root cause pointed out in this report: If a job is being executed from an open relay, the executor can drain the funds of owner that are being deposited into vault for the fee payment.

This was the impact which I was trying to say, NOT the root cause. Here the root cause what I was trying say was, if a low-level call is present in a job & if low-level call is being done while executing the job, and then recieve/fallback function is being triggered.

The complete control now would be in the executor's side, they can implement any arbitrary gas expensive complex logic in their fallback/recieve function.

Just so that they can increase the gasSpent after the exec call is being done, so that they can get more payment .

The attack path mentioned doesn't show any valid attack vector. Watson just attached some finding of Trust security by matching pattern without narrowing down any attack surface.

I clearly shared how from adding complex gas expensive logic in fallback/recieve function on executor side, the executor can increase the gasSpent parameter, which was supposed to be only equal to the gas costs being job executed. It was not a pattern matching and directly submitting an issue sir.

I attached the Trust security finding , to give context about the point they discussed about conversion into gasToken which could profit the attacker. But I was honestly not able to get much idea hence just attached it , so that if the Judge or sponsors might be having idea regarding this then they can discuss regarding this.

Attaching Trust security finding was done solely to give the context about the point of conversion into gasTokens so that you guys can get the idea and further discuss. It was just an added point in the vulnerability, not the main root cause of vulnerability.

And hence regarding it also I did added in IMPACT section , that if attacker is being able to convert it into gasTokens then would profit much more .

If not then ofcourse for implementing the gas expensive complex logic on the attacker side, the attacker would also have to pay for it. But still attacker can earn more than the gasSpent since the payment is being done by multiplying more different parameters with the gasSpent.

// Calculate the payment for the relayer
    uint256 _payment = (_gasSpent + GAS_BONUS) * block.basefee * GAS_MULTIPLIER / BASE;

Which shows that anyone can still have incentive to do this.

AND AT THE END CAN DRAIN THE FUNDS OF OWNERS DEPOSITED IN VAULT.

Whether attacker has incentive or not, it doesn't states out the fact that owners funds would be in risk

AND THIS IS NOT SIMILAR TO #85 THERE CAUSE IS RE-ENTRANCY, HERE CAUSE IS DIFFERENT

[Pranay-Bhilare]

@ashitakah

Putting our own blocker could affect the performance of other jobs.

I didn't suggested regarding putting your own blocker.

Instead I suggested just to add a parameter while adding the jobs , which the owner of job will set to the maximum value of gas cost/usage they might be expecting while executing the job.

Which while executing the job, if after the job's execution the gasSpent becomes greater than the cappedValue, then should revert, because it might be a situation where something malicious was being done, since the owner was expecting max gas usage equal to the cappedValue. It would be completely owner's responsibility to set this cappedValue .

This would prevent future drama if any similar situation arises, hence suggested this mitigation step, as this won't cause any loss to owner or protocol.

For jobs that have a low level call, they usually limit the calls or gas to be used.

They usually don't limit for gas being used, since it can cause different issues. Hence I shared & reported this issue, because it was nowhere stated in contest details also regarding this.

And from xkeeper's side this risk can be mitigated, hence would be better to avoid future drama.

[cvetanovv]

The owner is TRUSTED, and it would be his fault if he approves such work that contains a low-level call. You can also see this comment of mine which also applies here https://github.com/sherlock-audit/2024-04-xkeeper-judging/issues/85#issuecomment-2093038008

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

[Pranay-Bhilare]

@cvetanovv Sir I just want to say something before complete rejection of escalations.

Yes owner is TRUSTED, but it doesn't implies that approve such work that low-level call would be completely his fault. Because it was never given in contest details that owner should not add jobs with low level calls. Even sponsors didn't shared related to this , they did shared one thing thou :

however in this case, you need to be carefull with the jobs that you approve in order to avoid that someone can drain all your balance with low level calls.

They just said that the job owner should be careful .

Also in the comment by sponsor in #85

For jobs that have a low level call, they usually limit the calls or gas to be used. Since usually the owner of the vault is in charge of developing his own jobs to be automated, he has to have his own limitations in his code to not make it vulnerable.

They didn't stated the job shouldn't have a low level call, instead they said if a job has low level call they should just have their own limitation regarding gas/calls.

So I feel just because of owner is TRUSTED, wouldn't imply that it would be completely his fault, by also considering sponsors comments.

Yes it would be a sensitive job to be added since it has low level call , and it can be his fault, but from an owner's side if he's mitigating every vulnerabilities from his side, it wouldn't then be completely his fault.

Especially for my other issue #85 , if an owner is mitigating every possibility of vulnerability, and is risk free from any re-entrancy attack, just because the exec function in xKeeper's contract is vulnerable to re-entrancy, would cause loss to him. For this case it wouldn't be his fault since, he did took every secure measures from his side, but still got loss since exec was vulnerable to be re-entered.

Hence kindly reconsider this case.

[cvetanovv]

@Pranay-Bhilare I completely understand you. But when a protocol specifies in the Readme that the Owner is TRUSTED, we always assume that he will act in the right way, and will not make mistakes. These are the rules.

[Pranay-Bhilare]

@cvetanovv Sir I understand regarding your point and the rules.

But specifically for my other issue #85

How would this be an owner's mistake if he's mitigating every possibility of not getting attacked in their job, if their job is safe & secure from such possibilities like re-entrancy, why would it be his fault ?

Just because exec function is re-enterable , he would lose his funds, wouldn't this be a bit fault from xKeeper side and not Owner's?

Just because it has low-level call, it would be a sensitive call , if seen from xKeeper side, because they know their can be re-entrancy while executing jobs, but knowing this fact if Owner is mitigating vulerabilities from their side, then why would be it his fault/mistake sir ?

[cvetanovv]

This issue is very similar to #85. You can see this comment from the sponsor. Besides being a design decision, it is the responsibility of the Owner, who is TRUSTED, to set his own limits.

Remains my early decision to reject escalation.

[Pranay-Bhilare]

@cvetanovv

You can also see from the sponsor's comment that they intentionally didn't add a non-reentrant modifier. Absolutely the same applies to your other issue https://github.com/sherlock-audit/2024-04-xkeeper-judging/issues/131.

Sir you said this in my other issue.

HOW ADDING NON-REENTRANT MODIFIER ABSOLUTELY APPLIES HERE? I didn't even stated anything related to re-entrancy in this issue.

This issue is very similar to https://github.com/sherlock-audit/2024-04-xkeeper-judging/issues/85

I really don't know why you & sponsors are saying this is similar as #85. The only similar thing is the presence of low-level call in a job. In #85 root cause is Re-entrancy. Here I didn't even stated anything related to re-entrancy and still you said DESIGN CHOICE of not adding non re-entrant modifier applies here.

And adding a low-level call can't be an OWNER's mistake , if an owner is putting limitations from their side in their job, JUST LIKE YOU HAVE BEEN REFERRING TO.

Even if owner puts limitation to not make their job vulnerable. Still their funds deposited in vaults for payment can be stolen. I am saying this same thing from past few comments here and #85 .

And since I am being said that owner is TRUSTED, and is expected to not mistakes, and it's his responsibility to add limitations..... then if owner is adding limitations then it would NOT be owner's mistake.

[cvetanovv]

Take a look at this comment of mine.

Remains my early decision to reject the escalation.

[Evert0x]

Result: Invalid Duplicate of #4

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• RealLTDingZhen: rejected