fugazzi - Bots using the OpenRelay are highly susceptible to griefing attacks

[sherlock-admin2]

fugazzi

medium

Bots using the OpenRelay are highly susceptible to griefing attacks

Summary

Keepers that operate using the OpenRelay can be tricked into executing a malicious job that could eventually fail while executed on-chain, causing them to waste gas costs without getting any payment.

Vulnerability Detail

The OpenRelay has a simple design in which the relay calculates the amount of gas spent while running the jobs, and then rewards the caller by transferring back the spent gas plus some bonus as payment.

// Execute the automation vault counting the gas spent
uint256 _initialGas = gasleft();
_automationVault.exec(msg.sender, _execData, new IAutomationVault.FeeData[](0));
uint256 _gasSpent = _initialGas - gasleft();

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

// Send the payment to the relayer
IAutomationVault.FeeData[] memory _feeData = new IAutomationVault.FeeData[](1);
_feeData[0] = IAutomationVault.FeeData(_feeRecipient, _NATIVE_TOKEN, _payment);
_automationVault.exec(msg.sender, new IAutomationVault.ExecData[](0), _feeData);

We observe from the previous code that, if the call to _automationVault.exec() fails, then the whole operation will fail. In this case, the caller will carry the gas costs of the failed transaction, without any payment.

Naturally, it is expected that callers will simulate the operation off-chain to avoid calling a job that they know will fail beforehand. However, this can be easily bypassed by a malicious job. The job could have different runtime behavior depending on whether it is called during the simulation or the effective on-chain transaction. This can be implemented either by contextual variables (such as timestamp, gas, block number, origin, etc) or by querying some storage variable (not necessarily in the job contract).

As opposed to other adapters such as the Keep3rRelay that have an underlying protocol that can eventually slash jobs (see https://docs.keep3r.network/core/jobs#disputing-and-slashing-a-job), the OpenRelay adapter doesn't provide such guarantees to the caller.

Impact

Bots running with the OpenRelay can be griefed by making them spend gas in unsuccessful jobs without consequences for the job owner.

Code Snippet

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

Tool used

Manual Review

Recommendation

The relay would require a redesign to consider the possibility of job failure without the keeper being responsible. In such cases, the keeper deserves compensation for the wasted effort, or the job owner should face consequences that discourage bad behavior.

[sherlock-admin2]

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

Kose commented:

It is bot's responsibility to choose which job to execute. Not related to protocol. Invalid.

[realfugazzi]

Escalate

This is a valid medium security concern. The invalidation reason reads:

It is bot's responsibility to choose which job to execute. Not related to protocol. Invalid.

This is exactly the point of the issue. The runner can do their due diligence but even so be tricked into wasting gas. We can take the ERC4337 as a good example of how this can be nefarious for executors. The same issue broadly appears in the ERC4337 schema and requires extreme caution and treatment. There, the standard proposes heavy limitations (refer to section "Simulation Rationale" in the standard) plus a scoring system (refer to section "Reputation Rationale" in the standard)

This is mentioned in the report of the issue, and is why the keep3r network maintains a dispute system:

As opposed to other adapters such as the Keep3rRelay that have an underlying protocol that can eventually slash jobs (see https://docs.keep3r.network/core/jobs#disputing-and-slashing-a-job), the OpenRelay adapter doesn't provide such guarantees to the caller.

I encourage judges to read the report again.

[sherlock-admin2]

Escalate

This is a valid medium security concern. The invalidation reason reads:

It is bot's responsibility to choose which job to execute. Not related to protocol. Invalid.

This is exactly the point of the issue. The runner can do their due diligence but even so be tricked into wasting gas. We can take the ERC4337 as a good example of how this can be nefarious for executors. The same issue broadly appears in the ERC4337 schema and requires extreme caution and treatment. There, the standard proposes heavy limitations (refer to section "Simulation Rationale" in the standard) plus a scoring system (refer to section "Reputation Rationale" in the standard)

This is mentioned in the report of the issue, and is why the keep3r network maintains a dispute system:

As opposed to other adapters such as the Keep3rRelay that have an underlying protocol that can eventually slash jobs (see https://docs.keep3r.network/core/jobs#disputing-and-slashing-a-job), the OpenRelay adapter doesn't provide such guarantees to the caller.

I encourage judges to read the report again.

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.

[RealLTDingZhen]

Keepers are not forced to do a job. Please refer to this issue.

https://github.com/sherlock-audit/2024-02-rubicon-finance-judging/issues/53

[Hash01011122]

Hey I have consulted sponsors regarding this issue. Their take was as follows:

The job developers are responsible for preventing multiple calls, not us. There are usually restrictions that have to be put in place during development.

This was the reason I invalidated it, Do you want to add anything here @realfugazzi ??

[realfugazzi]

Hey I have consulted sponsors regarding this issue. Their take was as follows:

The job developers are responsible for preventing multiple calls, not us. There are usually restrictions that have to be put in place during development.

This was the reason I invalidated it, Do you want to add anything here @realfugazzi ??

That reason seems completely unrelated with the issue, this is a keeper being griefed. Please refer to the report and the escalation.

[cvetanovv]

I don't think the impact and the loss of gas are enough for this issue to be Medium.

Also, @RealLTDingZhen has left a good reference to a similar previous issue and why is invalid.

I'm planning to reject the escalation and leave the issue as it is.

[realfugazzi]

Keepers are not forced to do a job. Please refer to this issue.

sherlock-audit/2024-02-rubicon-finance-judging#53

Nobody is forced to initiate any transaction in any blockchain.

The protocol entails a cooperation between two parties and the contracts should provide security guarantees for both sides. In the same way we can discuss issues that affect vaults and their funds, we need to ensure keepers are safe too.

Saying that a keeper is not forced to do a job is the same as saying that a vault owner is not forced to deploy a vault, or that any user from any protocol is not forced to initiate any use case.

I believe the issue is valid, and it is quite clear by seeing how the keep3r network provides security for executors by slashing jobs. It depicts a flawed design in the OpenRelay contract, which is in scope.

[IllIllI000]

The admin is TRUSTED and therefore setting up a vault with a malicious job is invalid.

[realfugazzi]

The admin is TRUSTED and therefore setting up a vault with a malicious job is invalid.

The attitude from this person is hilarious. In any case I will reply accordingly.

Of course the owner of a vault won't do anything that causes harm to themselves. Admins of vaults are simply another entity in the protocol that can behave good or bad, these are NOT admins of the protocol or any contained entity that could be thought to behave well.

[Hash01011122]

I request that @realfugazzi adhere to professionalism when commenting on escalated issues. There have been multiple instances observed where your interactions with @IllIllI000 seem contentious. Additionally, I concur with @cvetanovv that the issue should be invalidated.

[realfugazzi]

I request that @realfugazzi adhere to professionalism when commenting on escalated issues. There have been multiple instances observed where your interactions with @IllIllI000 seem contentious. Additionally, I concur with @cvetanovv that the issue should be invalidated.

Could you please point how are these unprofessional? I'm defending issues with arguments, while the LSW is dishonestly posting completely unrelated reasons to nerf other's issues.

[cvetanovv]

The admin is TRUSTED and therefore setting up a vault with a malicious job is invalid.

The attitude from this person is hilarious. In any case I will reply accordingly.

Of course the owner of a vault won't do anything that causes harm to themselves. Admins of vaults are simply another entity in the protocol that can behave good or bad, these are NOT admins of the protocol or any contained entity that could be thought to behave well.

When it is written that the Оwner of the vault is a Тrusted he will only behave good and will not do any malicious things. It doesn't matter if it is about him or someone else. These are the rules. From there on all other things become invalid.

[realfugazzi]

The admin is TRUSTED and therefore setting up a vault with a malicious job is invalid.

The attitude from this person is hilarious. In any case I will reply accordingly. Of course the owner of a vault won't do anything that causes harm to themselves. Admins of vaults are simply another entity in the protocol that can behave good or bad, these are NOT admins of the protocol or any contained entity that could be thought to behave well.

When it is written that the Оwner of the vault is a Тrusted he will only behave good and will not do any malicious things. It doesn't matter if it is about him or someone else. These are the rules. From there on all other things become invalid.

You know this is not how the protocol operates. Vault owners are just another actor, the protocol states this by saying "that role is TRUSTED for the vault itself". This means that vault owners do not harm themselves or assets held by the vault itself. The "the whole protocol" includes keepers, so the role is restricted for the matters of this issue.

See https://audits.sherlock.xyz/contests/248:

Each automation vault has an owner, that role is TRUSTED for the vault itself, but RESTRICTED for the whole protocol.

[Hash01011122]

I standby what I assessed earlier, this will be considered as invalid issue.

[realfugazzi]

I standby what I assessed earlier, this will be considered as invalid issue.

That's fine, but please state why.

[Hash01011122]

Hey @realfugazzi, I meant @cvetanovv comments not mine for this issue 😅.

[realfugazzi]

The admin is TRUSTED and therefore setting up a vault with a malicious job is invalid.

The attitude from this person is hilarious. In any case I will reply accordingly. Of course the owner of a vault won't do anything that causes harm to themselves. Admins of vaults are simply another entity in the protocol that can behave good or bad, these are NOT admins of the protocol or any contained entity that could be thought to behave well.

When it is written that the Оwner of the vault is a Тrusted he will only behave good and will not do any malicious things. It doesn't matter if it is about him or someone else. These are the rules. From there on all other things become invalid.

You know this is not how the protocol operates. Vault owners are just another actor, the protocol states this by saying "that role is TRUSTED for the vault itself". This means that vault owners do not harm themselves or assets held by the vault itself. The "the whole protocol" includes keepers, so the role is restricted for the matters of this issue.

See https://audits.sherlock.xyz/contests/248:

Each automation vault has an owner, that role is TRUSTED for the vault itself, but RESTRICTED for the whole protocol.

@cvetanovv just making sure you have seen this, thanks

[ashitakah]

It is out of our scope if a bot tries to execute a malicious job, it should be the bot's responsibility to check the logic of the jobs it runs.

[cvetanovv]

@realfugazzi You can see this comment above from the sponsor and this comment also, https://github.com/sherlock-audit/2024-04-xkeeper-judging/issues/172#issuecomment-2090449220, which confirms that the Owner is TRUSTED.

I stand by my earlier decision and plan to reject the escalation and leave the issue as it is.

[realfugazzi]

@realfugazzi You can see this comment above from the sponsor and this comment also, #172 (comment), which confirms that the Owner is TRUSTED.

I stand by my earlier decision and plan to reject the escalation and leave the issue as it is.

I understand the comments, but the key point of the issue is that bots are tricked into executing a non-malicious looking job. Asking for responsibility on their side is equivalent to asking a user to not deposit funds in a vulnerable contract.

[realfugazzi]

I don't understand the logic either. In #57 it is ok to say that callers are dumb enough to not check their costs are being reimbursed (literally the very first thing you would check while building a keeper bot), but in the present issue callers are assumed to be the most smart builders that could detect any type of malicious job.

[kosedogus]

I believe you misunderstood #57 . That issue states that vaults at L2 regardless of their owner won't be used by keepers. So it is not saying the keepers won't check if their costs are being reimbursed and they will lose funds, it says they will check their costs and they will see that it is not reimbursed and they won't execute the jobs. See the summary in #57:

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.

[realfugazzi]

I believe you misunderstood #57 . That issue states that vaults at L2 regardless of their owner won't be used by keepers. So it is not saying the keepers won't check if their costs are being reimbursed and they will lose funds, it says they will check their costs and they will see that it is not reimbursed and they won't execute the jobs. See the summary in #57:

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.

Correct, then it is not a security issue. There is no loss, no lock, and no potentiality for neither. At most jobs won't be executed using that particular relay, which leaves the issue at the low/info side.

[kosedogus]

If jobs won't be executed using a particular relay, that means that relay is useless (core functionality is broken). Which makes it a valid medium. You can refer to Sherlock Documentation:

Breaks core contract functionality, rendering the contract useless or leading to loss of funds.

[cvetanovv]

"Gas griefing", according to Sherlock's rules, is a Low Severity issue. Furthermore, it is the bot's responsibility to choose what jobs to take, not the protocol.

My early decision to reject escalation remains.

[Evert0x]

Result: Invalid Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• realfugazzi: rejected