search

sherlock-audit / 2024-05-sophon-judging

7 stars 6 forks source link

ZdravkoHr. - Users are not protected against deposit slippage #42

Closed sherlock-admin2 closed 5 months ago

sherlock-admin2 commented 5 months ago

ZdravkoHr.

medium

Users are not protected against deposit slippage

Summary

The are a lot of reasons why a transaction may stay for a long time in the mempool - low gas provided, network congestion, etc. Users make decision in which pool to deposit based on the allocPoints the given pool has. Since these points can change and there is no slippage parameter, users may lose their tokens for rewards that don't satisfy them.

Vulnerability Detail

Imagine the following scenario:

[!NOTE] The same is true for pointsPerBlock and boostMultiplier

Impact

Medium since users lose their tokens forever, but still receive some rewards.

[!Warning] If the team plans to deactivate reward pools by setting their % to 0, the impact becomes high

Code Snippet

https://github.com/sherlock-audit/2024-05-sophon/blob/05059e53755f24ae9e3a3bb2996de15df0289a6c/farming-contracts/contracts/farm/SophonFarming.sol#L195-L216

Tool used

Manual Review

Recommendation

Add a slippage parameter to the deposit function and revert the transaction allocPoints, pointsPerBlock or boostMultiplier are not a predefined expected value.

sherlock-admin4 commented 5 months ago

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

0xmystery commented:

invalid because users can always optionally call deposit() by pre-converting DAI/sDAI, ETH/stETH, ETH/eTH at their control

ZdravkoHr commented 5 months ago

Escalate

I don't underdtand the provided reason. The issue talks about slippage protection

sherlock-admin3 commented 5 months ago

Escalate

I don't underdtand the provided reason. The issue talks about slippage protection

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.

mystery0x commented 5 months ago

Per Sherlock Judging, "An admin action can break certain assumptions about the functioning of the code".

In your case, reducing the pool's allocation point affects not only Alice but all other existing users associated with the pool to seemingly reduce point farming. This is not considered a valid issue.

WangSecurity commented 5 months ago

Agree with the lead judge, I believe this rule about TRUSTED admins should apply here:

An admin action can break certain assumptions about the functioning of the code. Example: Pausing a collateral causes some users to be unfairly liquidated or any other action causing loss of funds. This is not considered a valid issue.

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

ZdravkoHr commented 5 months ago

This issue is not about owners screwing users on purpose. In the example from the rules it's expected that some users will be liquidated if the collateral is paused.

This issue shows how users may deposit their funds at unfavorable terms because there is no slippage protection. It can happen because of transaction ordering. It's a low probability with high impact.

WangSecurity commented 5 months ago

I didn't say that it's about screwing users on purpose, excuse me if it seemed so. The only described scenario in this report is:

  1. The user submits txn expecting the protocol to function in one way.
  2. The admin submits txn will break this user's assumption about the functioning of the code (decreasing reward allocation).
  3. The user suffers a loss.

I believe it falls down this rule. Of course, no one says it's screwing users on purpose, it can happen unintentionally, but the problem is that the TRUSTED admin's actions leading to the user losing funds is invalid.

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

WangSecurity commented 5 months ago

Result: Invalid Unique

sherlock-admin4 commented 5 months ago

Escalations have been resolved successfully!

Escalation status: