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.

Sign up for GitHub

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

Fix arithmetic underflow in the StabilityPool #438

Merged
merged 24 commits into from
Oct 15, 2024
Merged

Fix arithmetic underflow in the StabilityPool #438

Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
cc314ca
test: Add unclaimable yield test
bingen Sep 24, 2024
b54822b
fix: Handle error offset correctly
bingen Sep 25, 2024
29c9d80
fix: Use (P - 1) instead of P for total yield rewards
bingen Sep 26, 2024
7c09b6a
fix: Prevent yields underflow
bingen Sep 26, 2024
ee32ae8
test: Increase sensitivity in SP test
bingen Sep 26, 2024
d6c6bf6
chore: Merge _computeYieldPerUnitStaked function into parent
bingen Sep 27, 2024
6802c59
chore: forge fmt
bingen Sep 27, 2024
e54ba8d
chore: Move yield gain cap to inner function and small refactor
bingen Sep 30, 2024
2f5a627
test: Add tests for coll gains underflow
bingen Sep 30, 2024
bd46494
fix: Apply the same fixes of yield gains to coll gains
bingen Sep 30, 2024
fdd6c6c
test: Increase SP test sensitivity
bingen Sep 30, 2024
1c05912
test: Add test case for not enough yield gains
bingen Oct 2, 2024
aeffca6
fix: Make getDepositorYieldGainWithPending consistent
bingen Oct 2, 2024
120792d
fix: wrong truncation ceiling in `getDepositorYieldGainWithPending()`
danielattilasimon Oct 2, 2024
991a80b
test: Remove some logs from Anchored SP tests
bingen Oct 2, 2024
9ce60db
fix: Apply P-1 patch to compounded deposits
bingen Oct 3, 2024
7f862c6
test: Remove changed, too long, tests
bingen Oct 3, 2024
d147474
test: Adjust SP tests sensitivity after P - 1 change
bingen Oct 3, 2024
6783051
fixup! fix: Apply P-1 patch to compounded deposits
bingen Oct 3, 2024
1134158
Merge branch 'main' into sp-underflow
bingen Oct 3, 2024
389aeb6
test: Pin unclaimable deposits test
bingen Oct 3, 2024
0a297bd
test: Pin SP fuzz test and adjust sensitivity
bingen Oct 3, 2024
e6b56c0
test: Remove inequalities to all funds claimable checks in SP invariants
bingen Oct 4, 2024
4e4b95f
Merge branch 'main' into sp-underflow
bingen Oct 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 66 additions & 67 deletions contracts/src/StabilityPool.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ contract StabilityPool is LiquityBase, IStabilityPool, IStabilityPoolEvents {
// Error trackers for the error correction in the offset calculation
uint256 public lastCollError_Offset;
uint256 public lastBoldLossError_Offset;
uint256 public lastBoldLossError_TotalDeposits;

// Error tracker fror the error correction in the BOLD reward calculation
uint256 public lastYieldError;
Expand Down Expand Up @@ -408,7 +409,7 @@ contract StabilityPool is LiquityBase, IStabilityPool, IStabilityPoolEvents {
uint256 yieldPerUnitStaked = yieldNumerator / totalBoldDepositsCached;
lastYieldError = yieldNumerator - yieldPerUnitStaked * totalBoldDepositsCached;

uint256 marginalYieldGain = yieldPerUnitStaked * P;
uint256 marginalYieldGain = yieldPerUnitStaked * (P - 1);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't fully get the use of P - 1 rather than P. As far as I can see:

  • The P error correction increases P, but
  • P error correction itself has rounding error (so, smaller than it should be)

Why do we then use P -1? Doesn't that make it even smaller than it should be?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rationale is that we end up using x * P_n / P_m, with n > m (and being P_n the one in the line above.
The fraction P_n / P_m represents the ratio of total deposits between those two snapshots (m and n).
We want the ratio between P’s to always be <= than the ratio between deposits. So that rewards shared are less or equal than should be, but not greater.
As both P_n and P_m are inaccurate, it may happen than the error (truncating) in P_m is bigger than in P_n. That would make the fraction to increase.
Let’s put an example, assuming DECIMAL_PRECISION = 100. Let’s imagine that the total deposits are 4999 and 2001 for m and n snapshots, and that theoretical values of P would be 49.99 (truncated to 49) and 20.01 (truncated to 20). Then we have P_n / P_m = 20 / 49 > 2001 / 4999 = deposit_n / deposit_m. That’s what we wan to avoid, so we use P_n = 19 instead of 20. Of course in this example this makes the precision of P_n / P_m very bad, but this is an edge case and mostly due to the low precision (only 2 decimals). With 18 decimals, subtracting 1 wouldn’t be such big deal.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, yes I see. So this correction should always make the error in P_n bigger than the error in P_m. Cool, it makes sense.

epochToScaleToB[currentEpoch][currentScale] = epochToScaleToB[currentEpoch][currentScale] + marginalYieldGain;

emit B_Updated(epochToScaleToB[currentEpoch][currentScale], currentEpoch, currentScale);
Expand All @@ -426,10 +427,7 @@ contract StabilityPool is LiquityBase, IStabilityPool, IStabilityPoolEvents {
uint256 totalBold = totalBoldDeposits; // cached to save an SLOAD
if (totalBold == 0 || _debtToOffset == 0) return;

(uint256 collGainPerUnitStaked, uint256 boldLossPerUnitStaked) =
_computeCollRewardsPerUnitStaked(_collToAdd, _debtToOffset, totalBold);

_updateCollRewardSumAndProduct(collGainPerUnitStaked, boldLossPerUnitStaked); // updates S and P
_updateCollRewardSumAndProduct(_collToAdd, _debtToOffset, totalBold); // updates S and P

_moveOffsetCollAndDebt(_collToAdd, _debtToOffset);
}
Expand All @@ -438,7 +436,7 @@ contract StabilityPool is LiquityBase, IStabilityPool, IStabilityPoolEvents {

function _computeCollRewardsPerUnitStaked(uint256 _collToAdd, uint256 _debtToOffset, uint256 _totalBoldDeposits)
internal
returns (uint256 collGainPerUnitStaked, uint256 boldLossPerUnitStaked)
returns (uint256 collGainPerUnitStaked, uint256 boldLossPerUnitStaked, uint256 newLastBoldLossErrorOffset)
{
/*
* Compute the Bold and Coll rewards. Uses a "feedback" error correction, to keep
Expand All @@ -457,33 +455,39 @@ contract StabilityPool is LiquityBase, IStabilityPool, IStabilityPoolEvents {
if (_debtToOffset == _totalBoldDeposits) {
boldLossPerUnitStaked = DECIMAL_PRECISION; // When the Pool depletes to 0, so does each deposit
lastBoldLossError_Offset = 0;
lastBoldLossError_TotalDeposits = _totalBoldDeposits;
} else {
uint256 boldLossNumerator = _debtToOffset * DECIMAL_PRECISION - lastBoldLossError_Offset;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering was this right in the first place (or maybe I need to refresh). Prior to this PR we were subtracting the error from the bold loss (L461). But shouldn't we add it, as we do for the coll?

Let's consider what we were trying to fix, if there was no error correction at all:

  • boldLossPerUnitStaked (b) is too small, due to floor division
  • newProductFactor (f) is too big since f = 1e18 - b
  • Therefore P is not reduced enough at the liquidation
  • Therefore the deposit calc,P/P_0 * initialDeposit is too large
  • So collateral gains and BOLD gains will also be too high, as they use P values in numerator at each reward
  • This could lead to underflows withdrawing deposits, since

So with no error correction it's like that.

But by subtracting the last BOLD error, don't we make it worse: b would already be too small from the floor division, and we then make it smaller.

In contrast, with the coll (line 452), we add the last coll error, which makes sense: coll rewards are too small from floor div, so we're storing the error and adding it.

I wonder if simply changing the sign here in L461 would fix underflow (or if its not so simple)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think that’s what we were trying to fix with the error. That we do it with the+ 1 in boldLossPerUnitStaked assignment below. We are doing a (pseudo-) ceil division.
Then with the lastBoldLossError_Offset what we are trying is that the difference between the real theoretical value and the value resulting from the ceil division doesn’t grow too big over time.
So:

  • With the + 1 we try to avoid underflows
  • With lastBoldLossError_Offset we try that the accumulated error from the previous measure doesn’t accumulate.

The problem is that the second step can revert the effect of the first one to some extent, and make the underflows appear again (if I’m not mistaken because the treatment of that error was wrong). That’s what we are trying to fix in this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In contrast, with the coll (line 452), we add the last coll error, which makes sense: coll rewards are too small from floor div, so we're storing the error and adding it.

Yes, coll rewards have the opposite direction because it’s a floor division (as opposed to the ceil division of bold deposits). I.e., we don’t do the +1 trick.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, coll rewards have the opposite direction because it’s a floor division (as opposed to the ceil division of bold deposits). I.e., we don’t do the +1 trick.

Ahhh right. Thanks, yes that makes it clear!

uint256 boldLossNumerator = _debtToOffset * DECIMAL_PRECISION;
/*
* Add 1 to make error in quotient positive. We want "slightly too much" Bold loss,
* which ensures the error in any given compoundedBoldDeposit favors the Stability Pool.
*/
boldLossPerUnitStaked = boldLossNumerator / _totalBoldDeposits + 1;
lastBoldLossError_Offset = boldLossPerUnitStaked * _totalBoldDeposits - boldLossNumerator;
newLastBoldLossErrorOffset = boldLossPerUnitStaked * _totalBoldDeposits - boldLossNumerator;
}

collGainPerUnitStaked = collNumerator / _totalBoldDeposits;
lastCollError_Offset = collNumerator - collGainPerUnitStaked * _totalBoldDeposits;

return (collGainPerUnitStaked, boldLossPerUnitStaked);
return (collGainPerUnitStaked, boldLossPerUnitStaked, newLastBoldLossErrorOffset);
}

// Update the Stability Pool reward sum S and product P
function _updateCollRewardSumAndProduct(uint256 _collGainPerUnitStaked, uint256 _boldLossPerUnitStaked) internal {
function _updateCollRewardSumAndProduct(uint256 _collToAdd, uint256 _debtToOffset, uint256 _totalBoldDeposits)
internal
{
(uint256 collGainPerUnitStaked, uint256 boldLossPerUnitStaked, uint256 newLastBoldLossErrorOffset) =
_computeCollRewardsPerUnitStaked(_collToAdd, _debtToOffset, _totalBoldDeposits);

uint256 currentP = P;
uint256 newP;

assert(_boldLossPerUnitStaked <= DECIMAL_PRECISION);
assert(boldLossPerUnitStaked <= DECIMAL_PRECISION);
/*
* The newProductFactor is the factor by which to change all deposits, due to the depletion of Stability Pool Bold in the liquidation.
* We make the product factor 0 if there was a pool-emptying. Otherwise, it is (1 - boldLossPerUnitStaked)
*/
uint256 newProductFactor = uint256(DECIMAL_PRECISION) - _boldLossPerUnitStaked;
uint256 newProductFactor = uint256(DECIMAL_PRECISION) - boldLossPerUnitStaked;

uint128 currentScaleCached = currentScale;
uint128 currentEpochCached = currentEpoch;
Expand All @@ -496,7 +500,7 @@ contract StabilityPool is LiquityBase, IStabilityPool, IStabilityPoolEvents {
*
* Since S corresponds to Coll gain, and P to deposit loss, we update S first.
*/
uint256 marginalCollGain = _collGainPerUnitStaked * currentP;
uint256 marginalCollGain = collGainPerUnitStaked * (currentP - 1);
uint256 newS = currentS + marginalCollGain;
epochToScaleToS[currentEpochCached][currentScaleCached] = newS;
emit S_Updated(newS, currentEpochCached, currentScaleCached);
Expand Down Expand Up @@ -524,8 +528,14 @@ contract StabilityPool is LiquityBase, IStabilityPool, IStabilityPoolEvents {
emit ScaleUpdated(currentScale);
// If there's no scale change and no pool-emptying, just do a standard multiplication
} else {
newP = currentP * newProductFactor / DECIMAL_PRECISION;
uint256 errorFactor;
if (lastBoldLossError_Offset > 0) {
errorFactor = lastBoldLossError_Offset * newProductFactor / lastBoldLossError_TotalDeposits;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to understand the rationale for the denominator here.

lastBoldLossError_TotalDeposits is only assigned a value in L458 (for a pool-emptying liq) and in L538 after it's been used in the division in L533.

So that means 1) if the current liq is pool-emptying, we use current _totalBoldDeposits in the division. But if 2) it's not pool emptying, we use the stored value lastBoldLossError_TotalDeposits which is stored at the previous liq.

Is that intended? If so, why?

In case 1), we use the most recent totalBoldDeposits, which could have changed since the last liq as deposits were added/removed. In 2, we use a potentially outdated stored value.

I'd have assumed we should always use the current value, since we're scaling a past error (calculated using past total deposits) by current total deposits.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that intended? If so, why?

Yes, that’s intended. In case 1 we are kind of resetting everything, P to 1 too. So we need to reset the error tracking too.

Anyway, there’s a TODO in this PR to review the case for scale updates, there may be something missing (although as far I know, it hasn’t surfaced during fuzzing yet). So, it’s intended, but I’m not 100% sure it’s correct. I want to get deep into that case once we make sure the basic logic (for the generic case where P is not reset) is sound.

}
newP = (currentP * newProductFactor + errorFactor) / DECIMAL_PRECISION;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems consistent with the previous implementation, i.e. making P larger (than without error correction). But see my comment on L461 questioning whether that was correct in the first place.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Answered above.

}
lastBoldLossError_Offset = newLastBoldLossErrorOffset;
lastBoldLossError_TotalDeposits = _totalBoldDeposits;

assert(newP > 0);
P = newP;
Expand Down Expand Up @@ -577,8 +587,22 @@ contract StabilityPool is LiquityBase, IStabilityPool, IStabilityPoolEvents {

Snapshots memory snapshots = depositSnapshots[_depositor];

uint256 collGain = _getCollGainFromSnapshots(initialDeposit, snapshots);
return collGain;
/*
* Grab the sum 'S' from the epoch at which the stake was made. The Coll gain may span up to one scale change.
* If it does, the second portion of the Coll gain is scaled by 1e9.
* If the gain spans no scale change, the second portion will be 0.
*/
uint128 epochSnapshot = snapshots.epoch;
uint128 scaleSnapshot = snapshots.scale;
uint256 S_Snapshot = snapshots.S;
uint256 P_Snapshot = snapshots.P;

uint256 firstPortion = epochToScaleToS[epochSnapshot][scaleSnapshot] - S_Snapshot;
uint256 secondPortion = epochToScaleToS[epochSnapshot][scaleSnapshot + 1] / SCALE_FACTOR;

uint256 collGain = initialDeposit * (firstPortion + secondPortion) / P_Snapshot / DECIMAL_PRECISION;

return LiquityMath._min(collGain, collBalance);
}

function getDepositorYieldGain(address _depositor) public view override returns (uint256) {
Expand All @@ -588,8 +612,22 @@ contract StabilityPool is LiquityBase, IStabilityPool, IStabilityPoolEvents {

Snapshots memory snapshots = depositSnapshots[_depositor];

uint256 yieldGain = _getYieldGainFromSnapshots(initialDeposit, snapshots);
return yieldGain;
/*
* Grab the sum 'B' from the epoch at which the stake was made. The Bold gain may span up to one scale change.
* If it does, the second portion of the Bold gain is scaled by 1e9.
* If the gain spans no scale change, the second portion will be 0.
*/
uint128 epochSnapshot = snapshots.epoch;
uint128 scaleSnapshot = snapshots.scale;
uint256 B_Snapshot = snapshots.B;
uint256 P_Snapshot = snapshots.P;

uint256 firstPortion = epochToScaleToB[epochSnapshot][scaleSnapshot] - B_Snapshot;
uint256 secondPortion = epochToScaleToB[epochSnapshot][scaleSnapshot + 1] / SCALE_FACTOR;

uint256 yieldGain = initialDeposit * (firstPortion + secondPortion) / P_Snapshot / DECIMAL_PRECISION;

return LiquityMath._min(yieldGain, yieldGainsOwed);
}

function getDepositorYieldGainWithPending(address _depositor) external view override returns (uint256) {
Expand All @@ -600,13 +638,14 @@ contract StabilityPool is LiquityBase, IStabilityPool, IStabilityPoolEvents {
Snapshots memory snapshots = depositSnapshots[_depositor];

uint256 pendingSPYield = activePool.calcPendingSPYield() + yieldGainsPending;
uint256 newYieldGainsOwed = yieldGainsOwed + (totalBoldDeposits >= DECIMAL_PRECISION ? pendingSPYield : 0);
uint256 firstPortionPending;
uint256 secondPortionPending;

if (pendingSPYield > 0 && snapshots.epoch == currentEpoch && totalBoldDeposits >= DECIMAL_PRECISION) {
uint256 yieldNumerator = pendingSPYield * DECIMAL_PRECISION + lastYieldError;
uint256 yieldPerUnitStaked = yieldNumerator / totalBoldDeposits;
uint256 marginalYieldGain = yieldPerUnitStaked * P;
uint256 marginalYieldGain = yieldPerUnitStaked * (P - 1);

if (currentScale == snapshots.scale) firstPortionPending = marginalYieldGain;
else if (currentScale == snapshots.scale + 1) secondPortionPending = marginalYieldGain;
Expand All @@ -616,53 +655,9 @@ contract StabilityPool is LiquityBase, IStabilityPool, IStabilityPoolEvents {
uint256 secondPortion =
(epochToScaleToB[snapshots.epoch][snapshots.scale + 1] + secondPortionPending) / SCALE_FACTOR;

return initialDeposit * (firstPortion + secondPortion) / snapshots.P / DECIMAL_PRECISION;
}
uint256 yieldGain = initialDeposit * (firstPortion + secondPortion) / snapshots.P / DECIMAL_PRECISION;

function _getCollGainFromSnapshots(uint256 initialDeposit, Snapshots memory snapshots)
internal
view
returns (uint256)
{
/*
* Grab the sum 'S' from the epoch at which the stake was made. The Coll gain may span up to one scale change.
* If it does, the second portion of the Coll gain is scaled by 1e9.
* If the gain spans no scale change, the second portion will be 0.
*/
uint128 epochSnapshot = snapshots.epoch;
uint128 scaleSnapshot = snapshots.scale;
uint256 S_Snapshot = snapshots.S;
uint256 P_Snapshot = snapshots.P;

uint256 firstPortion = epochToScaleToS[epochSnapshot][scaleSnapshot] - S_Snapshot;
uint256 secondPortion = epochToScaleToS[epochSnapshot][scaleSnapshot + 1] / SCALE_FACTOR;

uint256 collGain = initialDeposit * (firstPortion + secondPortion) / P_Snapshot / DECIMAL_PRECISION;

return collGain;
}

function _getYieldGainFromSnapshots(uint256 initialDeposit, Snapshots memory snapshots)
internal
view
returns (uint256)
{
/*
* Grab the sum 'B' from the epoch at which the stake was made. The Bold gain may span up to one scale change.
* If it does, the second portion of the Bold gain is scaled by 1e9.
* If the gain spans no scale change, the second portion will be 0.
*/
uint128 epochSnapshot = snapshots.epoch;
uint128 scaleSnapshot = snapshots.scale;
uint256 B_Snapshot = snapshots.B;
uint256 P_Snapshot = snapshots.P;

uint256 firstPortion = epochToScaleToB[epochSnapshot][scaleSnapshot] - B_Snapshot;
uint256 secondPortion = epochToScaleToB[epochSnapshot][scaleSnapshot + 1] / SCALE_FACTOR;

uint256 yieldGain = initialDeposit * (firstPortion + secondPortion) / P_Snapshot / DECIMAL_PRECISION;

return yieldGain;
return LiquityMath._min(yieldGain, newYieldGainsOwed);
}

// --- Compounded deposit ---
Expand Down Expand Up @@ -697,14 +692,18 @@ contract StabilityPool is LiquityBase, IStabilityPool, IStabilityPoolEvents {
uint256 compoundedStake;
uint128 scaleDiff = currentScale - scaleSnapshot;

// To make sure rouning errors favour the system, we use P - 1 if P decreased
uint256 cachedP = P;
uint256 currentPToUse = cachedP != snapshot_P ? cachedP - 1 : cachedP;

/* Compute the compounded stake. If a scale change in P was made during the stake's lifetime,
* account for it. If more than one scale change was made, then the stake has decreased by a factor of
* at least 1e-9 -- so return 0.
*/
if (scaleDiff == 0) {
compoundedStake = initialStake * P / snapshot_P;
compoundedStake = initialStake * currentPToUse / snapshot_P;
} else if (scaleDiff == 1) {
compoundedStake = initialStake * P / snapshot_P / SCALE_FACTOR;
compoundedStake = initialStake * currentPToUse / snapshot_P / SCALE_FACTOR;
} else {
// if scaleDiff >= 2
compoundedStake = 0;
Expand Down
Loading
Loading