feat: fixes after secondary Spearbit review (#290)

contracts/credit/CreditConfiguratorV3.sol

@@ -514,7 +514,8 @@ contract CreditConfiguratorV3 is ICreditConfiguratorV3, ControlledTrait, SanityC
             (uint128 minDebt, uint128 maxDebt) = prevCreditFacade.debtLimits();
             _setLimits({minDebt: minDebt, maxDebt: maxDebt}); // I:[CC-22]
 
-            _setLossLiquidator(prevCreditFacade.lossLiquidator()); // I:[CC-22]
+            address lossLiquidator = prevCreditFacade.lossLiquidator();
+            if (lossLiquidator != address(0)) _setLossLiquidator(lossLiquidator); // I:[CC-22]
 
             _migrateEmergencyLiquidators(prevCreditFacade); // I:[CC-22C]
 
@@ -646,11 +647,12 @@ contract CreditConfiguratorV3 is ICreditConfiguratorV3, ControlledTrait, SanityC
     }
 
     /// @notice Sets the new loss liquidator which can enforce policies on how liquidations with loss are performed
-    /// @param newLossLiquidator New loss liquidator, must be a contract or zero address
+    /// @param newLossLiquidator New loss liquidator, must be a contract
     function setLossLiquidator(address newLossLiquidator)
         external
         override
         configuratorOnly // I:[CC-2]
+        nonZeroAddress(newLossLiquidator) // I:[CC-26]
     {
         _setLossLiquidator(newLossLiquidator); // I:[CC-26]
     }

contracts/credit/CreditFacadeV3.sol

@@ -348,8 +348,7 @@ contract CreditFacadeV3 is ICreditFacadeV3, Pausable, ACLTrait, ReentrancyGuardT
         if (reportedLoss != 0) {
             maxDebtPerBlockMultiplier = 0; // U:[FA-17]
 
-            address lossLiquidator_ = lossLiquidator;
-            if (lossLiquidator_ != address(0) && msg.sender != lossLiquidator_) {
+            if (msg.sender != lossLiquidator) {
                 revert CallerNotLossLiquidatorException(); // U:[FA-17]
             }
         }
@@ -376,7 +375,7 @@ contract CreditFacadeV3 is ICreditFacadeV3, Pausable, ACLTrait, ReentrancyGuardT
     /// @dev Reverts if `token` is underlying or if `token` is a phantom token and its `depositedToken` is underlying
     /// @dev If `token` is a phantom token, it's withdrawn first, and its `depositedToken` is then sent to the liquidator.
     ///      Both `seizedAmount` and `minSeizedAmount` refer to `depositedToken` in this case.
-    /// @dev Like in full liquidations, liquidator can seize non-enabled tokens from the credit account, altough here
+    /// @dev Like in full liquidations, liquidator can seize non-enabled tokens from the credit account, although here
     ///      they are actually used to repay debt. Unclaimed rewards are safe since adapter calls are not allowed.
     function partiallyLiquidateCreditAccount(
         address creditAccount,
@@ -802,8 +801,10 @@ contract CreditFacadeV3 is ICreditFacadeV3, Pausable, ACLTrait, ReentrancyGuardT
     {
         if (adapter == address(0) || target == address(0)) revert TargetContractNotAllowedException(); // U:[FA-38]
 
-        if (flags & EXTERNAL_CONTRACT_WAS_CALLED_FLAG == 0) _setActiveCreditAccount(creditAccount); // U:[FA-38]
-        flags |= EXTERNAL_CONTRACT_WAS_CALLED_FLAG;
+        if (flags & EXTERNAL_CONTRACT_WAS_CALLED_FLAG == 0) {
+            _setActiveCreditAccount(creditAccount); // U:[FA-38]
+            flags |= EXTERNAL_CONTRACT_WAS_CALLED_FLAG; // U:[FA-38]
+        }
 
         bool useSafePrices = abi.decode(adapter.functionCall(callData), (bool)); // U:[FA-38]
         if (useSafePrices) flags |= REVERT_ON_FORBIDDEN_TOKENS_FLAG | USE_SAFE_PRICES_FLAG; // U:[FA-38,45]
@@ -857,13 +858,13 @@ contract CreditFacadeV3 is ICreditFacadeV3, Pausable, ACLTrait, ReentrancyGuardT
     /// @notice Sets the new loss liquidator
     /// @param newLossLiquidator New loss liquidator
     /// @dev Reverts if caller is not credit configurator
-    /// @dev Reverts if `newLossLiquidator` is not a contract, unless it's zero address
+    /// @dev Reverts if `newLossLiquidator` is not a contract
     function setLossLiquidator(address newLossLiquidator)
         external
         override
         creditConfiguratorOnly // U:[FA-6]
     {
-        if (newLossLiquidator != address(0) && newLossLiquidator.code.length == 0) {
+        if (newLossLiquidator.code.length == 0) {
             revert AddressIsNotContractException(newLossLiquidator); // U:[FA-51]
         }
         lossLiquidator = newLossLiquidator; // U:[FA-51]
@@ -902,6 +903,8 @@ contract CreditFacadeV3 is ICreditFacadeV3, Pausable, ACLTrait, ReentrancyGuardT
     }
 
     /// @notice Pauses contract, can only be called by an account with pausable admin role
+    /// @dev Pause blocks all user entrypoints to the contract.
+    ///      Liquidations remain open only to emergency and loss liquidators.
     /// @dev Reverts if contract is already paused
     function pause() external override pausableAdminsOnly {
         _pause();

contracts/interfaces/ICreditFacadeV3Multicall.sol

@@ -136,6 +136,7 @@ interface ICreditFacadeV3Multicall {
     /// @dev Withdrawals are prohibited in multicalls if there are forbidden tokens enabled as collateral on the account
     /// @dev Withdrawals activate safe pricing (min of main and reserve feeds) in collateral check
     /// @dev If `token` is a phantom token, it's withdrawn first, and its `depositedToken` is then sent to the recipient.
+    ///      No slippage prevention mechanism is provided as withdrawals are assumed to happen at non-manipulatable rate.
     ///      Although an adapter call is made in process, permission for external calls is not required.
     function withdrawCollateral(address token, uint256 amount, address to) external;
 

contracts/interfaces/IGaugeV3.sol

@@ -41,8 +41,6 @@ interface IGaugeV3Events {
 
 /// @title Gauge V3 interface
 interface IGaugeV3 is IVotingContract, IRateKeeper, IControlledTrait, IGaugeV3Events {
-    function voter() external view returns (address);
-
     function updateEpoch() external;
 
     function epochLastUpdate() external view returns (uint16);

contracts/interfaces/base/IVotingContract.sol

@@ -9,6 +9,7 @@ import {IVersion} from "./IVersion.sol";
 /// @notice Generic interface for a contract that can be voted for in `GearStakingV3` contract
 /// @dev `vote` and `unvote` must implement votes accounting since it's not performed on the staking contract side
 interface IVotingContract is IVersion {
+    function voter() external view returns (address);
     function vote(address user, uint96 votes, bytes calldata extraData) external;
     function unvote(address user, uint96 votes, bytes calldata extraData) external;
 }

contracts/pool/GaugeV3.sol

@@ -35,6 +35,7 @@ contract GaugeV3 is IGaugeV3, ControlledTrait, SanityCheckTrait {
     uint256 public constant override version = 3_10;
 
     /// @notice Contract type
+    /// @dev "RK" stands for "rate keeper"
     bytes32 public constant override contractType = "RK_GAUGE";
 
     /// @notice Address of the pool this gauge is connected to

contracts/pool/PoolV3.sol

@@ -56,10 +56,10 @@ contract PoolV3 is
     ERC4626,
     ERC20Permit,
     Pausable,
+    ReentrancyGuardTrait,
     SanityCheckTrait,
     ControlledTrait,
     ContractsRegisterTrait,
-    ReentrancyGuardTrait,
     IPoolV3
 {
     using Math for uint256;
@@ -758,6 +758,9 @@ contract PoolV3 is
     }
 
     /// @notice Pauses contract, can only be called by an account with pausable admin role
+    /// @dev Pause only blocks deposits, withdrawals and transfers.
+    ///      Borrowing and repayment can be paused on the credit side but are not blocked here
+    ///      to allow emergency liquidations to proceed.
     /// @dev Reverts if contract is already paused
     function pause() external override pausableAdminsOnly {
         _pause();

contracts/pool/TumblerV3.sol

@@ -27,6 +27,7 @@ contract TumblerV3 is ITumblerV3, ControlledTrait, SanityCheckTrait {
     uint256 public constant override version = 3_10;
 
     /// @notice Contract type
+    /// @dev "RK" stands for "rate keeper"
     bytes32 public constant override contractType = "RK_TUMBLER";
 
     /// @notice Pool whose quota rates are set by this contract

contracts/test/helpers/IntegrationTestHelper.sol

@@ -414,6 +414,10 @@ contract IntegrationTestHelper is TestHelper, BalanceHelper, ConfigManager {
                 cmParams.feeLiquidationExpired,
                 cmParams.liquidationPremiumExpired
             );
+
+            vm.etch(LIQUIDATOR, "DUMMY_CODE");
+            creditConfigurator.setLossLiquidator(LIQUIDATOR);
+
             vm.stopPrank();
             vm.roll(block.number + 1);
 
@@ -568,13 +572,14 @@ contract IntegrationTestHelper is TestHelper, BalanceHelper, ConfigManager {
         );
     }
 
-    function _makeAccountsLiquitable() internal {
+    function _makeAccountsLiquidatable() internal {
         vm.startPrank(CONFIGURATOR);
         uint256 idx = creditManager.collateralTokensCount() - 1;
         while (idx != 0) {
             address token = creditManager.getTokenByMask(1 << (idx--));
             creditConfigurator.setLiquidationThreshold(token, 0);
         }
+        // FIXME: setting liquidation premium to 90% is not the cleanest way to make account liquidatable
         creditConfigurator.setFees(200, 9000, 100, 9500);
         vm.stopPrank();
 

contracts/test/integration/credit/CreditConfigurator.int.t.sol

@@ -1032,6 +1032,10 @@ contract CreditConfiguratorIntegrationTest is IntegrationTestHelper, ICreditConf
 
     /// @dev I:[CC-26]: setLossLiquidator works correctly
     function test_I_CC_26_setLossLiquidator_works_correctly() public creditTest {
+        vm.expectRevert(ZeroAddressException.selector);
+        vm.prank(CONFIGURATOR);
+        creditConfigurator.setLossLiquidator(address(0));
+
         address liquidator = makeAddr("LOSS_LIQUIDATOR");
         vm.etch(liquidator, "DUMMY_CODE");
 

contracts/test/integration/credit/LiquidateCreditAccount.int.t.sol

@@ -62,7 +62,7 @@ contract LiquidateCreditAccountIntegrationTest is IntegrationTestHelper, ICredit
             MultiCall({target: address(adapterMock), callData: abi.encodeCall(AdapterMock.dumbCall, ())})
         );
 
-        _makeAccountsLiquitable();
+        _makeAccountsLiquidatable();
 
         // EXPECTED STACK TRACE & EVENTS
 
@@ -111,7 +111,7 @@ contract LiquidateCreditAccountIntegrationTest is IntegrationTestHelper, ICredit
             MultiCall({target: address(adapterMock), callData: abi.encodeCall(AdapterMock.dumbCall, ())})
         );
 
-        _makeAccountsLiquitable();
+        _makeAccountsLiquidatable();
 
         vm.prank(LIQUIDATOR);
         creditFacade.liquidateCreditAccount(creditAccount, FRIEND, calls);
@@ -137,7 +137,7 @@ contract LiquidateCreditAccountIntegrationTest is IntegrationTestHelper, ICredit
 
         (address creditAccount,) = _openTestCreditAccount();
 
-        _makeAccountsLiquitable();
+        _makeAccountsLiquidatable();
         vm.expectRevert(abi.encodeWithSelector(NoPermissionException.selector, INCREASE_DEBT_PERMISSION));
 
         vm.prank(LIQUIDATOR);

contracts/test/integration/credit/Quotas.int.t.sol

@@ -432,7 +432,7 @@ contract QuotasIntegrationTest is IntegrationTestHelper, ICreditManagerV3Events
 
         address[2] memory quotedTokens = [tokenTestSuite.addressOf(Tokens.USDT), tokenTestSuite.addressOf(Tokens.LINK)];
 
-        vm.prank(USER);
+        vm.prank(LIQUIDATOR);
         creditFacade.liquidateCreditAccount(creditAccount, FRIEND, new MultiCall[](0));
 
         for (uint256 i = 0; i < quotedTokens.length; ++i) {

contracts/test/unit/credit/CreditFacadeV3.unit.t.sol

@@ -940,25 +940,21 @@ contract CreditFacadeV3UnitTest is TestHelper, BalanceHelper, ICreditFacadeV3Eve
 
         creditManagerMock.setLiquidateCreditAccountReturns(0, 100);
 
-        // loss forbids borrowing, anyone can call
+        // only the loss liquidator can call
+        vm.expectRevert(CallerNotLossLiquidatorException.selector);
         vm.prank(FRIEND);
-        uint256 loss =
-            creditFacade.liquidateCreditAccount({creditAccount: creditAccount, to: FRIEND, calls: new MultiCall[](0)});
-        assertEq(loss, 100, "Incorrect loss");
-        assertEq(creditFacade.maxDebtPerBlockMultiplier(), 0, "Borrowing not forbidden");
+        creditFacade.liquidateCreditAccount({creditAccount: creditAccount, to: FRIEND, calls: new MultiCall[](0)});
 
         vm.etch(LIQUIDATOR, "CODE");
         vm.prank(CONFIGURATOR);
         creditFacade.setLossLiquidator(LIQUIDATOR);
 
-        // only the loss liquidator can call
-        vm.expectRevert(CallerNotLossLiquidatorException.selector);
-        vm.prank(FRIEND);
-        creditFacade.liquidateCreditAccount({creditAccount: creditAccount, to: FRIEND, calls: new MultiCall[](0)});
-
+        // loss forbids borrowing
         vm.prank(LIQUIDATOR);
-        creditFacade.liquidateCreditAccount({creditAccount: creditAccount, to: FRIEND, calls: new MultiCall[](0)});
+        uint256 loss =
+            creditFacade.liquidateCreditAccount({creditAccount: creditAccount, to: FRIEND, calls: new MultiCall[](0)});
         assertEq(loss, 100, "Incorrect loss");
+        assertEq(creditFacade.maxDebtPerBlockMultiplier(), 0, "Borrowing not forbidden");
     }
 
     //
@@ -2236,10 +2232,6 @@ contract CreditFacadeV3UnitTest is TestHelper, BalanceHelper, ICreditFacadeV3Eve
         creditFacade.setLossLiquidator(liquidator);
 
         assertEq(creditFacade.lossLiquidator(), liquidator, "Loss liquidator not set");
-
-        vm.prank(CONFIGURATOR);
-        creditFacade.setLossLiquidator(address(0));
-        assertEq(creditFacade.lossLiquidator(), address(0), "Loss liquidator not unset");
     }
 
     /// @dev U:[FA-52]: setTokenAllowance works properly