Merge pull request #151 from liquity/bug-double-interest

fix: double interest on pending redistribution
liquity · May 6, 2024 · 96cc851 · 96cc851
2 parents 0b75754 + 0111dcf
commit 96cc851
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