From 34a9127abe5a83fba52b142db628eba073d6cc6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=9Fingen?= Date: Mon, 26 Aug 2024 20:18:14 +0100 Subject: [PATCH 1/4] fix: Check new interest rate when leaving batch Fixes #366. --- contracts/src/BorrowerOperations.sol | 1 + .../src/test/interestBatchManagement.t.sol | 32 +++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/contracts/src/BorrowerOperations.sol b/contracts/src/BorrowerOperations.sol index 89882eac1..be85ef211 100644 --- a/contracts/src/BorrowerOperations.sol +++ b/contracts/src/BorrowerOperations.sol @@ -1035,6 +1035,7 @@ contract BorrowerOperations is LiquityBase, AddRemoveManagers, IBorrowerOperatio ) public override { _requireIsNotShutDown(); _requireCallerIsBorrower(_troveId); + _requireValidAnnualInterestRate(_newAnnualInterestRate); LocalVariables_removeFromBatch memory vars; vars.troveManager = troveManager; diff --git a/contracts/src/test/interestBatchManagement.t.sol b/contracts/src/test/interestBatchManagement.t.sol index dbfafd4cb..e4eb1799e 100644 --- a/contracts/src/test/interestBatchManagement.t.sol +++ b/contracts/src/test/interestBatchManagement.t.sol @@ -170,6 +170,38 @@ contract InterestBatchManagementTest is DevTestSetup { assertEq(annualInterestRate, newAnnualInterestRate, "Wrong interest rate"); } + function testRemoveBatchManagerFailsIfNewRateIsTooLow() public { + uint256 troveId = openTroveAndJoinBatchManager(); + (,,,,,,,, address tmBatchManagerAddress,) = troveManager.Troves(troveId); + LatestTroveData memory trove = troveManager.getLatestTroveData(troveId); + uint256 annualInterestRate = trove.annualInterestRate; + + uint256 newAnnualInterestRate = 4e15; + assertEq(tmBatchManagerAddress, B, "Wrong batch manager in TM"); + assertNotEq(newAnnualInterestRate, annualInterestRate, "New interest rate should be different"); + + vm.startPrank(A); + vm.expectRevert(BorrowerOperations.InterestRateTooLow.selector); + borrowerOperations.removeFromBatch(troveId, newAnnualInterestRate, 0, 0, 1e24); + vm.stopPrank(); + } + + function testRemoveBatchManagerFailsIfNewRateIsTooHigh() public { + uint256 troveId = openTroveAndJoinBatchManager(); + (,,,,,,,, address tmBatchManagerAddress,) = troveManager.Troves(troveId); + LatestTroveData memory trove = troveManager.getLatestTroveData(troveId); + uint256 annualInterestRate = trove.annualInterestRate; + + uint256 newAnnualInterestRate = 101e16; + assertEq(tmBatchManagerAddress, B, "Wrong batch manager in TM"); + assertNotEq(newAnnualInterestRate, annualInterestRate, "New interest rate should be different"); + + vm.startPrank(A); + vm.expectRevert(BorrowerOperations.InterestRateTooHigh.selector); + borrowerOperations.removeFromBatch(troveId, newAnnualInterestRate, 0, 0, 1e24); + vm.stopPrank(); + } + function testOnlyBorrowerCanSetBatchManager() public { registerBatchManager(A); registerBatchManager(B); From 6be8c8904b8bfc809b557fbddd82bf06805a1339 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=9Fingen?= Date: Mon, 26 Aug 2024 20:20:00 +0100 Subject: [PATCH 2/4] fix: Check if trove is in batch when attempting to remove Fixes #367. --- contracts/src/BorrowerOperations.sol | 1 + contracts/src/test/interestBatchManagement.t.sol | 13 +++++++++++++ 2 files changed, 14 insertions(+) diff --git a/contracts/src/BorrowerOperations.sol b/contracts/src/BorrowerOperations.sol index be85ef211..f0c01e30c 100644 --- a/contracts/src/BorrowerOperations.sol +++ b/contracts/src/BorrowerOperations.sol @@ -1036,6 +1036,7 @@ contract BorrowerOperations is LiquityBase, AddRemoveManagers, IBorrowerOperatio _requireIsNotShutDown(); _requireCallerIsBorrower(_troveId); _requireValidAnnualInterestRate(_newAnnualInterestRate); + _requireIsInBatch(_troveId); LocalVariables_removeFromBatch memory vars; vars.troveManager = troveManager; diff --git a/contracts/src/test/interestBatchManagement.t.sol b/contracts/src/test/interestBatchManagement.t.sol index e4eb1799e..715ddbb97 100644 --- a/contracts/src/test/interestBatchManagement.t.sol +++ b/contracts/src/test/interestBatchManagement.t.sol @@ -202,6 +202,19 @@ contract InterestBatchManagementTest is DevTestSetup { vm.stopPrank(); } + function testRemoveBatchManagerFailsIfTroveNotInBatch() public { + uint256 troveId = openTroveNoHints100pct(A, 100e18, 5000e18, 5e16); + (,,,,,,,, address tmBatchManagerAddress,) = troveManager.Troves(troveId); + + uint256 newAnnualInterestRate = 4e16; + assertEq(tmBatchManagerAddress, address(0), "Wrong batch manager in TM"); + + vm.startPrank(A); + vm.expectRevert(BorrowerOperations.TroveNotInBatch.selector); + borrowerOperations.removeFromBatch(troveId, newAnnualInterestRate, 0, 0, 1e24); + vm.stopPrank(); + } + function testOnlyBorrowerCanSetBatchManager() public { registerBatchManager(A); registerBatchManager(B); From 64364e01ca3bd83a4785557624543bad03ff4711 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=9Fingen?= Date: Mon, 26 Aug 2024 20:24:57 +0100 Subject: [PATCH 3/4] fix: Check trove status when leaving batch ...before reinserting to SortedTroves. Fixes #368. --- contracts/src/BorrowerOperations.sol | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/contracts/src/BorrowerOperations.sol b/contracts/src/BorrowerOperations.sol index f0c01e30c..2c73f9151 100644 --- a/contracts/src/BorrowerOperations.sol +++ b/contracts/src/BorrowerOperations.sol @@ -1047,7 +1047,10 @@ contract BorrowerOperations is LiquityBase, AddRemoveManagers, IBorrowerOperatio // Remove trove from Batch in SortedTroves vars.sortedTroves.removeFromBatch(_troveId); - vars.sortedTroves.insert(_troveId, _newAnnualInterestRate, _upperHint, _lowerHint); + // Reinsert as single trove, only if it’s not zombie + if (!_checkTroveIsUnredeemable(vars.troveManager, _troveId)) { + vars.sortedTroves.insert(_troveId, _newAnnualInterestRate, _upperHint, _lowerHint); + } vars.trove = vars.troveManager.getLatestTroveData(_troveId); vars.batch = vars.troveManager.getLatestBatchData(vars.batchManager); From 05ff41c3b886d64b8910f8be6c6ccdc2302c6688 Mon Sep 17 00:00:00 2001 From: Daniel Simon Date: Tue, 27 Aug 2024 12:01:40 +0700 Subject: [PATCH 4/4] fix: trying to remove non-existent node from `SortedTroves` --- contracts/src/BorrowerOperations.sol | 15 +++++++-------- .../TestContracts/InvariantsTestHandler.t.sol | 15 ++++----------- 2 files changed, 11 insertions(+), 19 deletions(-) diff --git a/contracts/src/BorrowerOperations.sol b/contracts/src/BorrowerOperations.sol index 2c73f9151..f6cbbaa49 100644 --- a/contracts/src/BorrowerOperations.sol +++ b/contracts/src/BorrowerOperations.sol @@ -1034,23 +1034,22 @@ contract BorrowerOperations is LiquityBase, AddRemoveManagers, IBorrowerOperatio uint256 _maxUpfrontFee ) public override { _requireIsNotShutDown(); - _requireCallerIsBorrower(_troveId); - _requireValidAnnualInterestRate(_newAnnualInterestRate); - _requireIsInBatch(_troveId); LocalVariables_removeFromBatch memory vars; vars.troveManager = troveManager; vars.sortedTroves = sortedTroves; - vars.batchManager = interestBatchManagerOf[_troveId]; + _requireTroveIsActive(vars.troveManager, _troveId); + _requireCallerIsBorrower(_troveId); + _requireValidAnnualInterestRate(_newAnnualInterestRate); + + vars.batchManager = _requireIsInBatch(_troveId); delete interestBatchManagerOf[_troveId]; // Remove trove from Batch in SortedTroves vars.sortedTroves.removeFromBatch(_troveId); - // Reinsert as single trove, only if it’s not zombie - if (!_checkTroveIsUnredeemable(vars.troveManager, _troveId)) { - vars.sortedTroves.insert(_troveId, _newAnnualInterestRate, _upperHint, _lowerHint); - } + // Reinsert as single trove + vars.sortedTroves.insert(_troveId, _newAnnualInterestRate, _upperHint, _lowerHint); vars.trove = vars.troveManager.getLatestTroveData(_troveId); vars.batch = vars.troveManager.getLatestBatchData(vars.batchManager); diff --git a/contracts/src/test/TestContracts/InvariantsTestHandler.t.sol b/contracts/src/test/TestContracts/InvariantsTestHandler.t.sol index 0f7efc8cd..4e598ae09 100644 --- a/contracts/src/test/TestContracts/InvariantsTestHandler.t.sol +++ b/contracts/src/test/TestContracts/InvariantsTestHandler.t.sol @@ -238,7 +238,6 @@ contract InvariantsTestHandler is BaseHandler, BaseMultiCollateralTest { LatestTroveData t; address batchManager; uint256 batchManagementFee; - bool wasOpen; bool wasActive; bool premature; uint256 upfrontFee; @@ -1930,7 +1929,6 @@ contract InvariantsTestHandler is BaseHandler, BaseMultiCollateralTest { v.t = v.c.troveManager.getLatestTroveData(v.troveId); v.batchManager = _batchManagerOf[i][v.troveId]; v.batchManagementFee = v.c.troveManager.getLatestBatchData(v.batchManager).accruedManagementFee; - v.wasOpen = _isOpen(i, v.troveId); v.wasActive = _isActive(i, v.troveId); v.premature = _timeSinceLastBatchInterestRateAdjustment[i][v.batchManager] < INTEREST_RATE_ADJ_COOLDOWN; v.upfrontFee = v.batchManager != address(0) @@ -1984,15 +1982,6 @@ contract InvariantsTestHandler is BaseHandler, BaseMultiCollateralTest { // Effects (system) _mintYield(i, v.pendingInterest, v.upfrontFee); - } catch Error(string memory reason) { - v.errorString = reason; - - // Justify failures - if (reason.equals("ERC721: invalid token ID")) { - assertFalse(v.wasOpen, "Open Trove should have an NFT"); - } else { - revert(reason); - } } catch (bytes memory revertData) { bytes4 selector; (selector, v.errorString) = _decodeCustomError(revertData); @@ -2781,6 +2770,10 @@ contract InvariantsTestHandler is BaseHandler, BaseMultiCollateralTest { return (selector, "BorrowerOperations.TroveInBatch()"); } + if (selector == BorrowerOperations.TroveNotInBatch.selector) { + return (selector, "BorrowerOperations.TroveNotInBatch()"); + } + if (selector == BorrowerOperations.InterestNotInRange.selector) { return (selector, "BorrowerOperations.InterestNotInRange()"); }