search

sherlock-audit / 2024-05-elfi-protocol-judging

10 stars 7 forks source link

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 3 months ago

sherlock-admin4 commented 3 months ago

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

Manual Review

Recommendation

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

0xELFi commented 3 months ago

We will fix it in future versions.

ctmotox2 commented 2 months ago

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

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.

Nikhil8400 commented 2 months ago

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

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

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

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

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

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

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

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

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

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

Result: Medium Unique

sherlock-admin4 commented 2 months ago

Escalations have been resolved successfully!

Escalation status: