Open sherlock-admin2 opened 4 months ago
2 comment(s) were left on this issue during the judging contest.
0xmystery commented:
invalid because getPendingPoints() will not practically be used till point farming has ended. The higher precision adopted is by design
0xreadyplayer1 commented:
even in the stated case - being the worst - the value goes uptil 1e72 and not 1e77 which is needed for overflow - the issue might exist but the watson was not able to clearly show the exploit about what happens if the price goes this large.
Escalate The value 1e72 will be multiplied by the user amount, thus causing the overflow as in the provided proof of concept.
Escalate The value 1e72 will be multiplied by the user amount, thus causing the overflow as in the provided proof of concept.
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.
The extreme edge case isn't going to happen. You see, the user amount is only 1 wei when multiplying with the inflated accPointsPerShare (due to division by 1 wei of lpSupply
).
So, when user.amount
and pool.amount
become sizable amounts (updated via the next _deposit()
, lpSupply
will no longer be 1 wei but rather whatever that has been last updated on pool.amount
in _pendingPoints()
.
Do I understand correctly that the user still can deposit into the protocol by dividing their deposit into several ones? So they can successfully deposit all 10_000 tokens as the POC and later users will not have problems? @ZdravkoHr is it correct or not?
And @mystery0x as I understand, user.amount and pool.amount will be updated after the point when the revert happens, no? Cause if what you're saying is correct, the POC wouldn't work, no? Or the POC is wrong?
When lpSupply becomes an 1e18 value, the accPointsPerShare will already be a very large value because in the updatePool function the amount is being added, not overwritten.
@WangSecurity, the problem is not that the user can't deposit, but that getPendingPoints will revert when the whole process depends on it
When lpSupply becomes an 1e18 value, the accPointsPerShare will already be a very large value because in the updatePool function the amount is being added, not overwritten.
Can you run the test and show us the result reverting? I don’t think it’s going to be a problem since division by lpSupply (1e18) at this point is going to make accPointsPerShare diminished prior to having it multiplied to user.amount.
The division by 1e18
will reduce the decimals of the pointReward
, however this result will be added to the already big enough pool.accPointsPerShare
value because there is an addition operation.
Here is the result of the PoC:
@ZdravkoHr Hmm... addition would cause overflow for the max value of uint256?
We agree that the first addend user.amount * accPointsPerShare /1e36
is already curbed after the second call. How big can the second addend user.rewardSettled
initially go? Can you plug in some practical numbers for these two addends and then the summation of both?
return user.amount *
accPointsPerShare /
1e36 +
user.rewardSettled -
user.rewardDebt;
But how is the first calculation curbed if there is an addition in any subsequent call and not assignment? The value will babe large decimals initially and will continue growing, there seems to be no way to decrease it
But how is the first calculation curbed if there is an addition in any subsequent call and not assignment? The value will babe large decimals initially and will continue growing, there seems to be no way to decrease it
pointReward * 1e18 / lpSupply
is trivial as lpSupply
is no longer 1 wei but 1e18:
accPointsPerShare = pointReward *
1e18 /
lpSupply +
accPointsPerShare;
Just plug in some numbers, and I'm sure no issues are going to be found. You're not multiplying two big numbers (rather just adding) in the last arithmetic operation of function _pendingPoints
:
return user.amount *
accPointsPerShare /
1e36 +
user.rewardSettled -
user.rewardDebt;
With the default values from the tests, pointsPerBlock = 25e18 and allocation of 50%. accPointsPerShare = 12.5e36
Users deposit and this value becomes > 12.5e36
Then we go here
uint256 accPointsPerShare = pool.accPointsPerShare * 1e18;
...
return user.amount *
accPointsPerShare /
1e36 +
user.rewardSettled -
user.rewardDebt;
Let user amount be 10_000e18 = 1e22. Then we have
1e22 * 12.5e54
which is greater than type(uint256.max)
Here are logs from the PoC:
Ah, ok. @RomanHiden you might want to look into this edge issue in _pendingPoints()
.
Based on the above discussion, I agree with the escalation.
Planning to accept it and validate with medium severity. Are there any duplicates of it?
@WangSecurity, just searched for getPendingPoints
and couldn't find a duplicate (there were 16 results). Maybe if @mystery0x knows of some
Many other reports touched on the topic but none of them identified the 1 wei culprit from the first and only depositor in the initial block(s).
It's noteworthy that the suggested mitigation, "to set a floor that a user has to deposit" will also solve the issues associated with #85 and #177.
Result: Medium Unique
The protocol team fixed this issue in the following PRs/commits: sophon-org/farming-contracts@f0b82fd
Can this issue be closed?
ZdravkoHr.
medium
accPointsPerShare
can reach a very large value leading to overflowsSummary
Each pool tracks the amount of points that should be distributed for one share of its LP token in the
accPointsPerShare
variable. This variable can reach very large values causing integer overflows. This is dangerous as it puts the protocol's functionality at great risks.Vulnerability Detail
This is the code that calculates
accPointsPerShare
.pointReward
is a variable with 36 decimals precision becauseblockMultiplier
andpointsPerBlock
have both 18 decimals. The result is then divided bylpSupply
.The problem with this approach is that lpSupply can be a small value. It can happen either naturally (for example, tokens with low decimals, like USDC and USDT) or on purpose (by a malicious depositor).
The malicious depositor can deposit just 1 wei of the lp token and wait 1 block to update the
accPointsPerShare
variable. SincelpSupply
will be equal to 1,accPointsPerShare
will remain a value with 36 decimals.Let's now have a look at pendingPoints()
pool.accPointsPerShare
is multipled by 1e18 once again, resulting in 54 decimals. This is not the end, in the return statement this value will be multiplied once again byuser.amount.
Depending on the token's decimals, the value will be scaled again. For 18 decimals, the value will reach 72 decimals. This will result in unexpected overflows becausetype(uint256).max < 1e78
Proof of Concept
In this test a first depositor deposits 1 wei worth of our Mock token to inflate the
accPointsPerShare
. In the next block, a honest depositor deposits10_000
tokens. WhenpendingPoints
is called, the transaction reverts because of an overflow.Impact
The
accPointsPerShare
variable becomes too large and breaks contract functionalityCode Snippet
https://github.com/sherlock-audit/2024-05-sophon/blob/05059e53755f24ae9e3a3bb2996de15df0289a6c/farming-contracts/contracts/farm/SophonFarming.sol#L423-L432 https://github.com/sherlock-audit/2024-05-sophon/blob/05059e53755f24ae9e3a3bb2996de15df0289a6c/farming-contracts/contracts/farm/SophonFarming.sol#L357C1-L385C1
Tool used
Manual Review
Recommendation
A possible solution may be to set a floor that a user has to deposit and also scale by a smaller value.