ZdravkoHr. - `accPointsPerShare` can reach a very large value leading to overflows
Opened this issue · 19 comments
ZdravkoHr.
medium
accPointsPerShare can reach a very large value leading to overflows
Summary
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 because blockMultiplier and pointsPerBlock have both 18 decimals. The result is then divided by lpSupply.
uint256 blockMultiplier = _getBlockMultiplier(pool.lastRewardBlock, getBlockNumber());
uint256 pointReward =
blockMultiplier *
_pointsPerBlock *
_allocPoint /
totalAllocPoint;
pool.accPointsPerShare = pointReward /
lpSupply +
pool.accPointsPerShare;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. Since lpSupply will be equal to 1, accPointsPerShare will remain a value with 36 decimals.
Let's now have a look at pendingPoints()
function _pendingPoints(uint256 _pid, address _user) internal view returns (uint256) {
PoolInfo storage pool = poolInfo[_pid];
UserInfo storage user = userInfo[_pid][_user];
uint256 accPointsPerShare = pool.accPointsPerShare * 1e18;
uint256 lpSupply = pool.amount;
if (getBlockNumber() > pool.lastRewardBlock && lpSupply != 0) {
uint256 blockMultiplier = _getBlockMultiplier(pool.lastRewardBlock, getBlockNumber());
uint256 pointReward =
blockMultiplier *
pointsPerBlock *
pool.allocPoint /
totalAllocPoint;
accPointsPerShare = pointReward *
1e18 /
lpSupply +
accPointsPerShare;
}
return user.amount *
accPointsPerShare /
1e36 +
user.rewardSettled -
user.rewardDebt;
}
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 by user.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 because type(uint256).max < 1e78
return user.amount *
accPointsPerShare 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 deposits 10_000 tokens. When pendingPoints is called, the transaction reverts because of an overflow.
function testOverflow() public {
vm.startPrank(deployer);
MockERC20 usdc = new MockERC20("Mock USDC Token", "MockUSDC", 18);
usdc.mint(address(deployer), 10000e18);
dai.mint(address(deployer), 1000e18);
uint256 usdcId = sophonFarming.add(60000, address(usdc), "", false);
dai.approve(address(sophonFarming), 1000e18);
usdc.approve(address(sophonFarming), 10000e18);
sophonFarming.depositDai(1000e18, 0);
sophonFarming.deposit(usdcId, 1, 0);
vm.roll(block.number + 1);
sophonFarming.massUpdatePools();
address[] memory users = new address[](1);
users[0] = deployer;
uint256[][] memory pendingPoints = sophonFarming.getPendingPoints(
users
);
for (uint i = 0; i < pendingPoints.length; i++) {
uint256 poolsLength = pendingPoints[i].length;
for (uint j = 0; j < poolsLength; j++) {
uint256 currentPoints = pendingPoints[i][j];
console.log(currentPoints);
}
}
sophonFarming.deposit(usdcId, 10000e18 - 1, 0);
vm.roll(block.number + 1);
sophonFarming.massUpdatePools();
pendingPoints = sophonFarming.getPendingPoints(users);
for (uint i = 0; i < pendingPoints.length; i++) {
uint256 poolsLength = pendingPoints[i].length;
for (uint j = 0; j < poolsLength; j++) {
uint256 currentPoints = pendingPoints[i][j];
console.log(currentPoints);
}
}
}Impact
The accPointsPerShare variable becomes too large and breaks contract functionality
Code 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.
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
Result:
Medium
Unique
The protocol team fixed this issue in the following PRs/commits: sophon-org/farming-contracts@f0b82fd
Can this issue be closed?