fix: double interest on pending redistribution

liquity · May 3, 2024 · 170cc1b · 170cc1b
1 parent 7c10422
commit 170cc1b
Show file tree

Hide file tree

Showing 2 changed files with 67 additions and 12 deletions.
diff --git a/contracts/src/BorrowerOperations.sol b/contracts/src/BorrowerOperations.sol
@@ -278,7 +278,7 @@ contract BorrowerOperations is LiquityBase, Ownable, CheckContract, IBorrowerOpe
  // TODO: Use oldDebt and assert in fuzzing, remove before deployment
  uint256 oldDebt = troveManager.getTroveEntireDebt(_troveId);
  _adjustTrove(msg.sender, _troveId, 0, false, _boldAmount, false, 0, contractsCache);
- assert(troveManager.getTroveEntireDebt(_troveId) < oldDebt); 
+ assert(troveManager.getTroveEntireDebt(_troveId) < oldDebt);
  }
 
  function adjustTrove(
@@ -292,12 +292,19 @@ contract BorrowerOperations is LiquityBase, Ownable, CheckContract, IBorrowerOpe
  ContractsCacheTMAPBT memory contractsCache = ContractsCacheTMAPBT(troveManager, activePool, boldToken);
  _requireTroveIsActive(contractsCache.troveManager, _troveId);
  _adjustTrove(
- msg.sender, _troveId, _collChange, _isCollIncrease, _boldChange, _isDebtIncrease, _maxFeePercentage, contractsCache
+ msg.sender,
+ _troveId,
+ _collChange,
+ _isCollIncrease,
+ _boldChange,
+ _isDebtIncrease,
+ _maxFeePercentage,
+ contractsCache
  );
  }
 
  function adjustUnredeemableTrove(
-  uint256 _troveId,
+ uint256 _troveId,
  uint256 _maxFeePercentage,
  uint256 _collChange,
  bool _isCollIncrease,
@@ -308,12 +315,21 @@ contract BorrowerOperations is LiquityBase, Ownable, CheckContract, IBorrowerOpe
  ) external override {
  ContractsCacheTMAPBT memory contractsCache = ContractsCacheTMAPBT(troveManager, activePool, boldToken);
  _requireTroveIsUnredeemable(contractsCache.troveManager, _troveId);
-  // TODO: Gas - pass the cached TM down here, since we fetch it again inside _adjustTrove?
+ // TODO: Gas - pass the cached TM down here, since we fetch it again inside _adjustTrove?
  _adjustTrove(
- msg.sender, _troveId, _collChange, _isCollIncrease, _boldChange, _isDebtIncrease, _maxFeePercentage, contractsCache
+ msg.sender,
+ _troveId,
+ _collChange,
+ _isCollIncrease,
+ _boldChange,
+ _isDebtIncrease,
+ _maxFeePercentage,
+ contractsCache
  );
  troveManager.setTroveStatusToActive(_troveId);
- sortedTroves.insert(_troveId, contractsCache.troveManager.getTroveAnnualInterestRate(_troveId), _upperHint, _lowerHint);
+ sortedTroves.insert(
+ _troveId, contractsCache.troveManager.getTroveAnnualInterestRate(_troveId), _upperHint, _lowerHint
+ );
  }
 
  function adjustTroveInterestRate(
@@ -665,9 +681,8 @@ contract BorrowerOperations is LiquityBase, Ownable, CheckContract, IBorrowerOpe
  uint256 initialWeightedRecordedTroveDebt = _troveManager.getTroveWeightedRecordedDebt(_troveId);
  // --- Effects ---
 
- (, uint256 redistDebtGain) = _troveManager.getAndApplyRedistributionGains(_troveId);
-
  uint256 accruedTroveInterest = _troveManager.calcTroveAccruedInterest(_troveId);
+ (, uint256 redistDebtGain) = _troveManager.getAndApplyRedistributionGains(_troveId);
  uint256 recordedTroveDebt = _troveManager.getTroveDebt(_troveId);
  uint256 entireTroveDebt = recordedTroveDebt + accruedTroveInterest;
  uint256 newWeightedTroveDebt = entireTroveDebt * _annualInterestRate;
@@ -709,11 +724,13 @@ contract BorrowerOperations is LiquityBase, Ownable, CheckContract, IBorrowerOpe
  }
 
  function _requireTroveIsUnredeemable(ITroveManager _troveManager, uint256 _troveId) internal view {
- require(_troveManager.checkTroveIsUnredeemable(_troveId), "BorrowerOps: Trove does not have unredeemable status");
+ require(
+ _troveManager.checkTroveIsUnredeemable(_troveId), "BorrowerOps: Trove does not have unredeemable status"
+ );
  }
 
- function _requireTroveIsNotOpen(ITroveManager _troveManager, uint256 _troveId) internal view { 
- require(!_troveManager.checkTroveIsOpen(_troveId),"BorrowerOps: Trove is open");
+ function _requireTroveIsNotOpen(ITroveManager _troveManager, uint256 _troveId) internal view {
+ require(!_troveManager.checkTroveIsOpen(_troveId), "BorrowerOps: Trove is open");
  }
 
  function _requireNonZeroCollChange(uint256 _collChange) internal pure {

diff --git a/contracts/src/test/interestRateAggregate.t.sol b/contracts/src/test/interestRateAggregate.t.sol
@@ -2570,7 +2570,45 @@ contract InterestRateAggregate is DevTestSetup {
  // Check recorded debt sum has changed correctly
  assertEq(activePool.aggWeightedDebtSum(), expectedAggWeightedRecordedDebt);
  }
-
+
+ // A bug was caught while reading the implementation of `_updateActivePoolTrackersNoDebtChange()`, wherein
+ // a borrower incurred double interest on redistribution gains when adjusting their interest rate.
+ // This test case covers that scenario.
+ //
+ // We should properly address the TODO below ("tests with pending debt redist. gain >0"), but in the meantime,
+ // keep this testcase.
+ function testNoDoubleInterestOnPendingRedistribution() public {
+ TroveIDs memory troveIDs;
+
+ uint256 coll = 100 ether;
+ uint256 borrow = 10_000 ether - 200 ether;
+ uint256 interestRate = 1 ether;
+ troveIDs.A = openTroveNoHints100pctMaxFee(A, coll, borrow, interestRate);
+ troveIDs.B = openTroveNoHints100pctMaxFee(B, coll, borrow, interestRate);
+ troveIDs.C = openTroveNoHints100pctMaxFee(C, coll, borrow, interestRate);
+ troveIDs.D = openTroveNoHints100pctMaxFee(D, coll, borrow, interestRate);
+
+ emit log_named_decimal_uint("Trove D debt (initial) ", troveManager.getTroveEntireDebt(troveIDs.D), 18);
+ vm.warp(block.timestamp + 365 days);
+ emit log_named_decimal_uint("Trove D debt (post-1y) ", troveManager.getTroveEntireDebt(troveIDs.D), 18);
+
+ priceFeed.setPrice(110 ether);
+
+ uint256[] memory liquidatedTroves = new uint256[](3);
+ liquidatedTroves[0] = troveIDs.A;
+ liquidatedTroves[1] = troveIDs.B;
+ liquidatedTroves[2] = troveIDs.C;
+ troveManager.batchLiquidateTroves(liquidatedTroves);
+
+ uint256 debtBefore = troveManager.getTroveEntireDebt(troveIDs.D);
+ emit log_named_decimal_uint("Trove D debt (post-liq) ", debtBefore, 18);
+ changeInterestRateNoHints(D, troveIDs.D, 0.1 ether);
+ uint256 debtAfter = troveManager.getTroveEntireDebt(troveIDs.D);
+ emit log_named_decimal_uint("Trove D debt (post-adj) ", debtAfter, 18);
+
+ assertEq(debtBefore, debtAfter, "Adjusting interest rate shouldn't change Trove's debt");
+ }
+
  // TODO: mixed collateral & debt adjustment opps
  // TODO: tests with pending debt redist. gain >0
  // TODO: tests that show total debt change under user ops