fix: debt limit changes no longer break partial liquidations

Fixes spearbit-audits/review-gearbox#60
Gearbox-protocol · Sep 1, 2024 · 693de97 · 693de97
1 parent 026e7ed
commit 693de97
Show file tree

Hide file tree

Showing 5 changed files with 42 additions and 21 deletions.
diff --git a/contracts/credit/CreditFacadeV3.sol b/contracts/credit/CreditFacadeV3.sol
@@ -694,7 +694,7 @@ contract CreditFacadeV3 is ICreditFacadeV3, ACLNonReentrantTrait {
         (uint256 newDebt,,) =
             ICreditManagerV3(creditManager).manageDebt(creditAccount, amount, enabledTokensMask, action); // U:[FA-27,31]
 
-        _revertIfOutOfDebtLimits(newDebt); // U:[FA-28,32,33]
+        _revertIfOutOfDebtLimits(newDebt, action); // U:[FA-28,32,33]
     }
 
     /// @dev `ICreditFacadeV3Multicall.updateQuota` implementation
@@ -881,9 +881,11 @@ contract CreditFacadeV3 is ICreditFacadeV3, ACLNonReentrantTrait {
         totalBorrowedInBlock = uint128(newDebtInCurrentBlock); // U:[FA-43]
     }
 
-    /// @dev Ensures that account's debt principal is within allowed range or is zero
-    function _revertIfOutOfDebtLimits(uint256 debt) internal view {
-        if (debt == 0) return;
+    /// @dev Ensures that account's debt principal takes allowed values:
+    ///      - for borrowing, new debt must be within allowed limits
+    ///      - for repayment, new debt must be above allowed minimum or zero
+    function _revertIfOutOfDebtLimits(uint256 debt, ManageDebtAction action) internal view {
+        if (debt == 0 && action == ManageDebtAction.DECREASE_DEBT) return;
         uint256 minDebt;
         uint256 maxDebt;
 
@@ -895,7 +897,7 @@ contract CreditFacadeV3 is ICreditFacadeV3, ACLNonReentrantTrait {
             minDebt := and(data, 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF)
         }
 
-        if (debt < minDebt || debt > maxDebt) {
+        if (debt < minDebt || debt > maxDebt && action == ManageDebtAction.INCREASE_DEBT) {
             revert BorrowAmountOutOfLimitsException(); // U:[FA-44]
         }
     }

diff --git a/contracts/interfaces/ICreditFacadeV3Multicall.sol b/contracts/interfaces/ICreditFacadeV3Multicall.sol
@@ -102,7 +102,7 @@ interface ICreditFacadeV3Multicall {
     /// @param amount Underlying amount to borrow
     /// @dev Increasing debt is prohibited when closing an account
     /// @dev Increasing debt is prohibited if it was previously updated in the same block
-    /// @dev The resulting debt amount must be within allowed range
+    /// @dev The resulting debt amount must be within allowed limits
     /// @dev Increasing debt is prohibited if there are forbidden tokens enabled as collateral on the account
     /// @dev After debt increase, total amount borrowed by the credit manager in the current block must not exceed
     ///      the limit defined in the facade
@@ -112,7 +112,8 @@ interface ICreditFacadeV3Multicall {
     /// @param amount Underlying amount to repay, value above account's total debt indicates full repayment
     /// @dev Decreasing debt is prohibited when opening an account
     /// @dev Decreasing debt is prohibited if it was previously updated in the same block
-    /// @dev The resulting debt amount must be within allowed range or zero
+    /// @dev The resulting debt amount must be above allowed minimum or zero (maximum is not checked here
+    ///      to allow small repayments and partial liquidations in case configurator lowers it)
     /// @dev Full repayment brings account into a special mode that skips collateral checks and thus requires
     ///      an account to have no potential debt sources, e.g., all quotas must be disabled
     function decreaseDebt(uint256 amount) external;

diff --git a/contracts/test/helpers/IntegrationTestHelper.sol b/contracts/test/helpers/IntegrationTestHelper.sol
@@ -495,16 +495,23 @@ contract IntegrationTestHelper is TestHelper, BalanceHelper, ConfigManager {
 
         return creditFacade.openCreditAccount(
             onBehalfOf,
-            MultiCallBuilder.build(
-                MultiCall({
-                    target: address(creditFacade),
-                    callData: abi.encodeCall(ICreditFacadeV3Multicall.increaseDebt, (debt))
-                }),
-                MultiCall({
-                    target: address(creditFacade),
-                    callData: abi.encodeCall(ICreditFacadeV3Multicall.addCollateral, (underlying, amount))
-                })
-            ),
+            debt == 0
+                ? MultiCallBuilder.build(
+                    MultiCall({
+                        target: address(creditFacade),
+                        callData: abi.encodeCall(ICreditFacadeV3Multicall.addCollateral, (underlying, amount))
+                    })
+                )
+                : MultiCallBuilder.build(
+                    MultiCall({
+                        target: address(creditFacade),
+                        callData: abi.encodeCall(ICreditFacadeV3Multicall.increaseDebt, (debt))
+                    }),
+                    MultiCall({
+                        target: address(creditFacade),
+                        callData: abi.encodeCall(ICreditFacadeV3Multicall.addCollateral, (underlying, amount))
+                    })
+                ),
             referralCode
         );
     }

diff --git a/contracts/test/unit/credit/CreditFacadeV3.unit.t.sol b/contracts/test/unit/credit/CreditFacadeV3.unit.t.sol
@@ -306,6 +306,7 @@ contract CreditFacadeV3UnitTest is TestHelper, BalanceHelper, ICreditFacadeV3Eve
     /// @dev U:[FA-7]: payable functions wraps eth to msg.sender
     function test_U_FA_07_payable_functions_wraps_eth_to_msg_sender() public notExpirableCase {
         vm.deal(USER, 3 ether);
+        creditManagerMock.setManageDebt(1 ether);
 
         vm.prank(CONFIGURATOR);
         creditFacade.setDebtLimits(1 ether, 9 ether, 9);
@@ -1580,10 +1581,20 @@ contract CreditFacadeV3UnitTest is TestHelper, BalanceHelper, ICreditFacadeV3Eve
         creditFacade.setDebtLimits(minDebt, maxDebt, 1);
 
         vm.expectRevert(BorrowAmountOutOfLimitsException.selector);
-        creditFacade.revertIfOutOfDebtLimits(minDebt - 1);
+        creditFacade.revertIfOutOfDebtLimits(0, ManageDebtAction.INCREASE_DEBT);
+
+        creditFacade.revertIfOutOfDebtLimits(0, ManageDebtAction.DECREASE_DEBT);
+
+        vm.expectRevert(BorrowAmountOutOfLimitsException.selector);
+        creditFacade.revertIfOutOfDebtLimits(minDebt - 1, ManageDebtAction.INCREASE_DEBT);
 
         vm.expectRevert(BorrowAmountOutOfLimitsException.selector);
-        creditFacade.revertIfOutOfDebtLimits(maxDebt + 1);
+        creditFacade.revertIfOutOfDebtLimits(minDebt - 1, ManageDebtAction.DECREASE_DEBT);
+
+        vm.expectRevert(BorrowAmountOutOfLimitsException.selector);
+        creditFacade.revertIfOutOfDebtLimits(maxDebt + 1, ManageDebtAction.INCREASE_DEBT);
+
+        creditFacade.revertIfOutOfDebtLimits(maxDebt + 1, ManageDebtAction.DECREASE_DEBT);
     }
 
     /// @dev U:[FA-45]: multicall handles forbidden tokens properly

diff --git a/contracts/test/unit/credit/CreditFacadeV3Harness.sol b/contracts/test/unit/credit/CreditFacadeV3Harness.sol
@@ -56,8 +56,8 @@ contract CreditFacadeV3Harness is CreditFacadeV3 {
         return totalBorrowedInBlock;
     }
 
-    function revertIfOutOfDebtLimits(uint256 debt) external view {
-        _revertIfOutOfDebtLimits(debt);
+    function revertIfOutOfDebtLimits(uint256 debt, ManageDebtAction action) external view {
+        _revertIfOutOfDebtLimits(debt, action);
     }
 
     function isExpired() external view returns (bool) {