Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nikhil840096 - The lossFee is simply added to the commonData and not reimbursed to the keeper, leading to potential losses for the keeper. #93

Open
sherlock-admin4 opened this issue Jun 20, 2024 · 15 comments
Labels
Escalation Resolved This issue's escalations have been approved/rejected 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 Jun 20, 2024

nikhil840096

High

The lossFee is simply added to the commonData and not reimbursed to the keeper, leading to potential losses for the keeper.

Summary

The lossFee is simply added to the commonData and not reimbursed to the keeper, leading to potential losses for the keeper.

Vulnerability Detail

The processExecutionFee function is designed to calculate and handle the execution fee required by the keeper and ensure that this fee is appropriately managed between the user and the keeper. The function also addresses scenarios where the actual gas cost exceeds or falls below the user's provided execution fee. Below is the implementation of the function:
https://github.com/sherlock-audit/2024-05-elfi-protocol/blob/main/elfi-perp-contracts/contracts/process/GasProcess.sol#L17-L41

  1. Execution Fee Calculation:

  2. Fee Adjustments:

  3. Transfer and Withdrawal Mechanisms:

  4. Handling Loss Fees:

Issue:

The lossFee is simply added to the common data pool and not reimbursed to the keeper, leading to potential losses for the keeper.

Impact

  • This could disincentivize keepers from participating, as they may incur losses without compensation.

Code Snippet

https://github.com/sherlock-audit/2024-05-elfi-protocol/blob/main/elfi-perp-contracts/contracts/process/GasProcess.sol#L17-L41

Tool used

Manual Review

Recommendation

Implement a function to incentivize the keepers for there loss in execution fee.

@github-actions github-actions bot added the Medium A valid Medium severity issue label Jun 27, 2024
@sherlock-admin3 sherlock-admin3 added Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed labels Jun 28, 2024
@0xELFi
Copy link

0xELFi commented Jun 30, 2024

We will fix it in future versions.

@sherlock-admin3 sherlock-admin3 added Won't Fix The sponsor confirmed this issue will not be fixed and removed Will Fix The sponsor confirmed this issue will be fixed labels Jun 30, 2024
@sherlock-admin2 sherlock-admin2 changed the title Soaring Tawny Fly - The lossFee is simply added to the commonData and not reimbursed to the keeper, leading to potential losses for the keeper. nikhil840096 - The lossFee is simply added to the commonData and not reimbursed to the keeper, leading to potential losses for the keeper. Jul 3, 2024
@sherlock-admin2 sherlock-admin2 added the Reward A payout will be made for this issue label Jul 3, 2024
@ctmotox2
Copy link

ctmotox2 commented Jul 5, 2024

Escalate
This is a future assumption of the code and can also be interpreted as a design choice. Loss fees are correctly accounted for in Diamond's storage. While there is currently no function to withdraw these fees to keepers, a new function can be introduced to facilitate this since the contract is Diamond-upgradable.

@sherlock-admin3
Copy link

Escalate
This is a future assumption of the code and can also be interpreted as a design choice. Loss fees are correctly accounted for in Diamond's storage. While there is currently no function to withdraw these fees to keepers, a new function can be introduced to facilitate this since the contract is Diamond-upgradable.

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.

@sherlock-admin4 sherlock-admin4 added the Escalated This issue contains a pending escalation label Jul 5, 2024
@Nikhil8400
Copy link

I believe my finding regarding the lossFee not being reimbursed to the keeper should be marked as valid for the following reasons:

  1. Documentation and Protocol Transparency:

    • Nowhere in the protocol's documentation, including the design choices or known issues sections, is this issue mentioned. Transparent communication about such potential losses is crucial for keepers to make informed decisions.
  2. Acknowledgment of Issue:

    • The escalation response acknowledges that the issue can be mitigated by introducing a new function to facilitate reimbursement. This acknowledgment itself indicates that the current implementation is lacking a necessary function, thereby confirming the presence of the issue.
  3. Diamond-Upgradable Argument:

    • While it's true that the contract's Diamond-upgradable nature allows for future enhancements, this does not negate the current issue. If we were to apply this logic universally, it would imply that any issue could be dismissed on the grounds that it can be fixed in the future. This undermines the purpose of identifying and addressing issues during audits.
  4. Consistency with Previous Findings:

    • A similar issue was considered valid and marked as medium in a recent audit contest link. Consistency in evaluating findings is essential for maintaining the integrity and reliability of the auditing process.

In conclusion, the absence of documentation about this issue, combined with the acknowledgment that a future function is needed to address it, strongly supports the validity of my finding. It is essential to recognize this as a medium-level issue to ensure that it is appropriately addressed in the protocol's current and future implementations.

@Hash01011122
Copy link

@Nikhil8400

  1. Documentation and Protocol Transparency: If it wasn't mentioned in protocol's documentation doesn't mean its a valid issue. Validity will be based on breakage of core functionality or loss of funds
  2. Acknowledgment of Issue: Escalation points out that function can be added without any impact caused to protocol or any parties involved. Moreover, your issue doesn't even point out While there is currently no function to withdraw these fees to keepers
  3. Diamond-Upgradable Argument: Agreed with the reasoning of this one, but it doesn't qualify for medium severity.
  4. Consistency with Previous Findings: The finding you mentioned, has different root cause then yours. Root cause of that finding is: Discrepancy Gas fee ratio of L1 and L2 chain which breaks the core functionality of contract which cannot be reversed if contracts are deployed. Whereas your issue points out lossfee because of no withdraw function which was pointed out by @ctmotox2, which is reversible without causing any impact.

This should be considered as low severity issue.

I hope I answered your concerns @Nikhil8400.

@nevillehuang
Copy link
Collaborator

nevillehuang commented Jul 6, 2024

By this logic mentioned here, any potential issues can be upgraded via the diamond proxy pattern to resolve issue. So I believe this is still a valid medium severity issue.

@ctmotox2
Copy link

ctmotox2 commented Jul 6, 2024

That logic here is related to the recovery of the issue, meaning that issue is reversible without causing any impact as mentioned by @Hash01011122 .

The main point here is that, loss fees are still correctly accounted for in Diamond's storage.

Hence, I believe this issue does not qualify for medium severity.

@WangSecurity
Copy link

Firstly, we have to remember that historical decisions are not sources of truth.

Secondly, I believe the design decision rule doesn't apply here. Not due to the reason this issue is not mentioned as a design decision, but because it leads to a loss of funds.

Thirdly, the argument that the upgradeability could resolve this issue decreases the severity, but I disagree it makes the issue low. I agree with the Lead Judge that medium severity is indeed appropriate here.

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

@mstpr
Copy link
Collaborator

mstpr commented Jul 8, 2024

Firstly, we have to remember that historical decisions are not sources of truth.

Secondly, I believe the design decision rule doesn't apply here. Not due to the reason this issue is not mentioned as a design decision, but because it leads to a loss of funds.

Thirdly, the argument that the upgradeability could resolve this issue decreases the severity, but I disagree it makes the issue low. I agree with the Lead Judge that medium severity is indeed appropriate here.

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

How do we possibly know that maybe a contract OOS has a function to withdraw the funds?

@Nikhil8400
Copy link

But ser elfi team has admitted this issue in above comments and stated that they are going to fix this in future version link

@WangSecurity
Copy link

Firstly, we have to remember that historical decisions are not sources of truth.
Secondly, I believe the design decision rule doesn't apply here. Not due to the reason this issue is not mentioned as a design decision, but because it leads to a loss of funds.
Thirdly, the argument that the upgradeability could resolve this issue decreases the severity, but I disagree it makes the issue low. I agree with the Lead Judge that medium severity is indeed appropriate here.
Planning to reject the escalation and leave the issue as it is.

How do we possibly know that maybe a contract OOS has a function to withdraw the funds?

If there's concrete evidence there's such a function, please provide it. Otherwise, the decision remains the same, planning to reject the escalation and leave the issue as it is.

@mstpr
Copy link
Collaborator

mstpr commented Jul 10, 2024

Firstly, we have to remember that historical decisions are not sources of truth.
Secondly, I believe the design decision rule doesn't apply here. Not due to the reason this issue is not mentioned as a design decision, but because it leads to a loss of funds.
Thirdly, the argument that the upgradeability could resolve this issue decreases the severity, but I disagree it makes the issue low. I agree with the Lead Judge that medium severity is indeed appropriate here.
Planning to reject the escalation and leave the issue as it is.

How do we possibly know that maybe a contract OOS has a function to withdraw the funds?

If there's concrete evidence there's such a function, please provide it. Otherwise, the decision remains the same, planning to reject the escalation and leave the issue as it is.

We assumed a lot of stuff in this contest.

For example settling the unsettled fees are also not in the code, they are probably in a OOS code. Would that mean if I would've submit unsettled fees can't be settled because there is no functionality would be a valid issue?

"If there's concrete evidence there's such a function, please provide it" I would rather not do that because why would I be checking OOS code...

@WangSecurity
Copy link

Fair point, but in this case we also have a confirming this issue is correct. Of course, I don't say the sponsor confirming the bug or adding labels affects the validity or severity of the issue, but I believe this comment indeed confirms there is no function to withdraw funds.

Hence, the decision remains the same, planning to reject the escalation and leave the issue as it is.

@WangSecurity
Copy link

Result:
Medium
Unique

@sherlock-admin2 sherlock-admin2 removed the Escalated This issue contains a pending escalation label Jul 11, 2024
@sherlock-admin3 sherlock-admin3 added the Escalation Resolved This issue's escalations have been approved/rejected label Jul 11, 2024
@sherlock-admin4
Copy link
Author

Escalations have been resolved successfully!

Escalation status:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Escalation Resolved This issue's escalations have been approved/rejected 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

10 participants