Merge pull request #388 from liquity/document_batch_upfrontfee

fix: Fix lastInterestRateAdjTime when leaving/joining batches

README.md

@@ -506,9 +506,27 @@ The premature adjustment fee works as so:
 
 - When a Trove is opened, its `lastInterestRateAdjTime` property is set equal to the current time
 - When a borrower adjusts their interest rate via `adjustTroveInterestRate` the system checks that the cooldown period has passed since their last interest rate adjustment 
-
 - If the adjustment is sooner it incurs an upfront fee (equal to 7 days of average interest of the respective branch) which is added to their debt.
 
+#### Batches and upfront fee
+
+##### Joining a batch
+When a trove joins a batch, it pays upfront fee if the last trove adjustment was done more than the cool period ago. It does’t matter if trove and batch have the same interest rate, or when was the last adjustment by the batch.
+
+The last interest rate timestamp will be updated to the time of joining.
+
+Batch interest rate changes only take into account global batch timestamps, so when the new batch manager changes the interest rate less than the cooldown period after the borrower moved to the new batch, but more than the cooldown period after its last adjustment, the newly joined borrower wouldn't pay the upfront fee despite the fact that his last interest rate change happened less than the cooldown period ago.
+
+That’s why troves pay upfront fee when joining even if the interest is the same. Otherwise a trove may game it by having a batch created in advance (with no recent changens), joining it and the changing the rate of the batch.
+
+##### Leaving a batch
+When a trove leaves a batch, the user's timestamp is again reset to the current time.
+No upfront fee is charged, unless the interest rate is changed in the same transaction and the batch changed the interest rate less than the cooldown period ago.
+
+##### Switching batches
+As the function to switch batches is just a wrapper that calls the functions for leaving and joining a batch, this means that switching batches always incurs in upfront fee now (unless user doesn’t use the wrapper and waits for 1 week between leaving and joining).
+
+
 ## BOLD Redemptions
 
 Any BOLD holder (whether or not they have an active Trove) may redeem their BOLD directly with the system. Their BOLD is exchanged for a mixture of collaterals at face value: redeeming 1 BOLD token returns $1 worth of collaterals (minus a dynamic redemption fee), priced at their current market values according to their respective oracles. Redemptions have two purposes:

contracts/src/BorrowerOperations.sol

@@ -994,7 +994,7 @@ contract BorrowerOperations is LiquityBase, AddRemoveManagers, IBorrowerOperatio
         newBatchTroveChange.newWeightedRecordedDebt =
             (vars.newBatch.entireDebtWithoutRedistribution + vars.trove.entireDebt) * vars.newBatch.annualInterestRate;
 
-        // TODO: We may check the old rate to see if it’s different than the new one, but then we should check the
+        // We may check the old rate to see if it’s different than the new one, but then we should check the
         // last interest adjustment times to avoid gaming. So we decided to keep it simple and account it always
         // as a change. It’s probably not so common to join a batch with the exact same interest rate.
         // Apply upfront fee on premature adjustments
@@ -1071,7 +1071,7 @@ contract BorrowerOperations is LiquityBase, AddRemoveManagers, IBorrowerOperatio
         // Apply upfront fee on premature adjustments
         if (
             vars.batch.annualInterestRate != _newAnnualInterestRate
-                && block.timestamp < vars.batch.lastInterestRateAdjTime + INTEREST_RATE_ADJ_COOLDOWN
+            && block.timestamp < vars.trove.lastInterestRateAdjTime + INTEREST_RATE_ADJ_COOLDOWN
         ) {
             vars.trove.entireDebt =
                 _applyUpfrontFee(vars.trove.entireColl, vars.trove.entireDebt, batchChange, _maxUpfrontFee);

contracts/src/HintHelpers.sol

@@ -216,15 +216,36 @@ contract HintHelpers is IHintHelpers {
             return 0;
         }
 
+        return _predictJoinBatchInterestRateUpfrontFee(activePool, trove, batch);
+    }
+
+    function forcePredictJoinBatchInterestRateUpfrontFee(uint256 _collIndex, uint256 _troveId, address _batchAddress)
+        external
+        view
+        returns (uint256)
+    {
+        ITroveManager troveManager = collateralRegistry.getTroveManager(_collIndex);
+        IActivePool activePool = troveManager.activePool();
+        LatestTroveData memory trove = troveManager.getLatestTroveData(_troveId);
+        LatestBatchData memory batch = troveManager.getLatestBatchData(_batchAddress);
+
+        return _predictJoinBatchInterestRateUpfrontFee(activePool, trove, batch);
+    }
+
+    function _predictJoinBatchInterestRateUpfrontFee(
+        IActivePool _activePool,
+        LatestTroveData memory _trove,
+        LatestBatchData memory _batch
+    ) internal view returns (uint256) {
         TroveChange memory newBatchTroveChange;
-        newBatchTroveChange.appliedRedistBoldDebtGain = trove.redistBoldDebtGain;
-        newBatchTroveChange.batchAccruedManagementFee = batch.accruedManagementFee;
-        newBatchTroveChange.oldWeightedRecordedDebt = batch.weightedRecordedDebt + trove.weightedRecordedDebt;
+        newBatchTroveChange.appliedRedistBoldDebtGain = _trove.redistBoldDebtGain;
+        newBatchTroveChange.batchAccruedManagementFee = _batch.accruedManagementFee;
+        newBatchTroveChange.oldWeightedRecordedDebt = _batch.weightedRecordedDebt + _trove.weightedRecordedDebt;
         newBatchTroveChange.newWeightedRecordedDebt =
-            (batch.entireDebtWithoutRedistribution + trove.entireDebt) * batch.annualInterestRate;
+            (_batch.entireDebtWithoutRedistribution + _trove.entireDebt) * _batch.annualInterestRate;
 
-        uint256 avgInterestRate = activePool.getNewApproxAvgInterestRateFromTroveChange(newBatchTroveChange);
-        return _calcUpfrontFee(trove.entireDebt, avgInterestRate);
+        uint256 avgInterestRate = _activePool.getNewApproxAvgInterestRateFromTroveChange(newBatchTroveChange);
+        return _calcUpfrontFee(_trove.entireDebt, avgInterestRate);
     }
 
     function predictRemoveFromBatchUpfrontFee(uint256 _collIndex, uint256 _troveId, uint256 _newInterestRate)
@@ -240,7 +261,7 @@ contract HintHelpers is IHintHelpers {
 
         if (
             _newInterestRate == batch.annualInterestRate
-                || block.timestamp >= batch.lastInterestRateAdjTime + INTEREST_RATE_ADJ_COOLDOWN
+                || block.timestamp >= trove.lastInterestRateAdjTime + INTEREST_RATE_ADJ_COOLDOWN
         ) {
             return 0;
         }

contracts/src/TroveManager.sol

@@ -1223,6 +1223,7 @@ contract TroveManager is LiquityBase, ITroveManager, ITroveEvents {
         Troves[_troveId].status = Status.active;
         Troves[_troveId].arrayIndex = uint64(TroveIds.length);
         Troves[_troveId].interestBatchManager = _batchAddress;
+        Troves[_troveId].lastInterestRateAdjTime = uint64(block.timestamp);
 
         _updateTroveRewardSnapshots(_troveId);
 

contracts/src/test/TestContracts/BaseTest.sol

@@ -128,6 +128,14 @@ contract BaseTest is TestAccounts, Logging {
         return hintHelpers.predictJoinBatchInterestRateUpfrontFee(0, _troveId, _batchAddress);
     }
 
+    function forcePredictJoinBatchInterestRateUpfrontFee(uint256 _troveId, address _batchAddress)
+        internal
+        view
+        returns (uint256)
+    {
+        return hintHelpers.forcePredictJoinBatchInterestRateUpfrontFee(0, _troveId, _batchAddress);
+    }
+
     // Quick and dirty binary search instead of Newton's, because it's easier
     function findAmountToBorrowWithOpenTrove(uint256 targetDebt, uint256 interestRate)
         internal

contracts/src/test/TestContracts/InvariantsTestHandler.t.sol

@@ -468,7 +468,7 @@ contract InvariantsTestHandler is BaseHandler, BaseMultiCollateralTest {
                 address batchManager = _batchManagerOf[j][troveId];
                 Trove storage trove = _troves[j][troveId];
 
-                if (batchManager == address(0)) _timeSinceLastTroveInterestRateAdjustment[j][troveId] += timeDelta;
+                _timeSinceLastTroveInterestRateAdjustment[j][troveId] += timeDelta;
                 if (isShutdown[j]) continue; // shutdown branches stop accruing interest & batch management fees
 
                 uint256 interest = trove.accrueInterest(timeDelta);
@@ -872,8 +872,11 @@ contract InvariantsTestHandler is BaseHandler, BaseMultiCollateralTest {
         v.trove = _troves[i][v.troveId];
         v.wasActive = _isActive(i, v.troveId);
         v.premature = _timeSinceLastTroveInterestRateAdjustment[i][v.troveId] < INTEREST_RATE_ADJ_COOLDOWN;
-        v.upfrontFee = hintHelpers.predictAdjustInterestRateUpfrontFee(i, v.troveId, newInterestRate);
-        if (v.upfrontFee > 0) assertTrue(v.premature, "Only premature adjustment should incur upfront fee");
+
+        if (v.batchManager == address(0)) {
+            v.upfrontFee = hintHelpers.predictAdjustInterestRateUpfrontFee(i, v.troveId, newInterestRate);
+            if (v.upfrontFee > 0) assertTrue(v.premature, "Only premature adjustment should incur upfront fee");
+        }
 
         info("upper hint: ", _hintToString(i, v.upperHint));
         info("lower hint: ", _hintToString(i, v.lowerHint));
@@ -1875,8 +1878,11 @@ contract InvariantsTestHandler is BaseHandler, BaseMultiCollateralTest {
         v.wasOpen = _isOpen(i, v.troveId);
         v.wasActive = _isActive(i, v.troveId);
         v.premature = _timeSinceLastTroveInterestRateAdjustment[i][v.troveId] < INTEREST_RATE_ADJ_COOLDOWN;
-        v.upfrontFee = hintHelpers.predictJoinBatchInterestRateUpfrontFee(i, v.troveId, v.newBatchManager);
-        if (v.upfrontFee > 0) assertTrue(v.premature, "Only premature adjustment should incur upfront fee");
+
+        if (_batchManagerOf[i][v.troveId] == address(0)) {
+            v.upfrontFee = hintHelpers.predictJoinBatchInterestRateUpfrontFee(i, v.troveId, v.newBatchManager);
+            if (v.upfrontFee > 0) assertTrue(v.premature, "Only premature adjustment should incur upfront fee");
+        }
 
         info("batch manager: ", vm.getLabel(v.newBatchManager));
         info("upper hint: ", _hintToString(i, v.upperHint));
@@ -1991,11 +1997,15 @@ contract InvariantsTestHandler is BaseHandler, BaseMultiCollateralTest {
         v.batchManager = _batchManagerOf[i][v.troveId];
         v.batchManagementFee = v.c.troveManager.getLatestBatchData(v.batchManager).accruedManagementFee;
         v.wasActive = _isActive(i, v.troveId);
-        v.premature = _timeSinceLastBatchInterestRateAdjustment[i][v.batchManager] < INTEREST_RATE_ADJ_COOLDOWN;
-        v.upfrontFee = v.batchManager != address(0)
-            ? hintHelpers.predictRemoveFromBatchUpfrontFee(i, v.troveId, newInterestRate)
-            : 0;
-        if (v.upfrontFee > 0) assertTrue(v.premature, "Only premature adjustment should incur upfront fee");
+        v.premature = Math.min(
+            _timeSinceLastTroveInterestRateAdjustment[i][v.troveId],
+            _timeSinceLastBatchInterestRateAdjustment[i][v.batchManager]
+        ) < INTEREST_RATE_ADJ_COOLDOWN;
+
+        if (v.batchManager != address(0)) {
+            v.upfrontFee = hintHelpers.predictRemoveFromBatchUpfrontFee(i, v.troveId, newInterestRate);
+            if (v.upfrontFee > 0) assertTrue(v.premature, "Only premature adjustment should incur upfront fee");
+        }
 
         Trove memory trove = _troves[i][v.troveId];
         Batch storage batch = _batches[i][v.batchManager];
@@ -2035,6 +2045,7 @@ contract InvariantsTestHandler is BaseHandler, BaseMultiCollateralTest {
             trove.interestRate = newInterestRate;
             trove.batchManagementRate = 0;
             _troves[i][v.troveId] = trove;
+            _timeSinceLastTroveInterestRateAdjustment[i][v.troveId] = 0;
             delete _batchManagerOf[i][v.troveId];
 
             // Effects (batch)