From 12aa418e2295f9cf08128b0e4273e1175e6182d6 Mon Sep 17 00:00:00 2001 From: Dima Lekhovitsky Date: Mon, 30 Sep 2024 18:46:17 +0300 Subject: [PATCH] feat: fixes after secondary Spearbit review (#290) --- contracts/credit/CreditConfiguratorV3.sol | 6 +++-- contracts/credit/CreditFacadeV3.sol | 17 ++++++++------ .../interfaces/ICreditFacadeV3Multicall.sol | 1 + contracts/interfaces/IGaugeV3.sol | 2 -- contracts/interfaces/base/IVotingContract.sol | 1 + contracts/pool/GaugeV3.sol | 1 + contracts/pool/PoolV3.sol | 5 ++++- contracts/pool/TumblerV3.sol | 1 + .../test/helpers/IntegrationTestHelper.sol | 7 +++++- .../credit/CreditConfigurator.int.t.sol | 4 ++++ .../credit/LiquidateCreditAccount.int.t.sol | 6 ++--- .../test/integration/credit/Quotas.int.t.sol | 2 +- .../unit/credit/CreditFacadeV3.unit.t.sol | 22 ++++++------------- 13 files changed, 43 insertions(+), 32 deletions(-) diff --git a/contracts/credit/CreditConfiguratorV3.sol b/contracts/credit/CreditConfiguratorV3.sol index b0333f00..a317c72d 100644 --- a/contracts/credit/CreditConfiguratorV3.sol +++ b/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] } diff --git a/contracts/credit/CreditFacadeV3.sol b/contracts/credit/CreditFacadeV3.sol index 14bb6d08..0d27e066 100644 --- a/contracts/credit/CreditFacadeV3.sol +++ b/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(); diff --git a/contracts/interfaces/ICreditFacadeV3Multicall.sol b/contracts/interfaces/ICreditFacadeV3Multicall.sol index d0b658cf..03f33915 100644 --- a/contracts/interfaces/ICreditFacadeV3Multicall.sol +++ b/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; diff --git a/contracts/interfaces/IGaugeV3.sol b/contracts/interfaces/IGaugeV3.sol index 972c6cde..ddddfb59 100644 --- a/contracts/interfaces/IGaugeV3.sol +++ b/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); diff --git a/contracts/interfaces/base/IVotingContract.sol b/contracts/interfaces/base/IVotingContract.sol index a13b53d7..79473026 100644 --- a/contracts/interfaces/base/IVotingContract.sol +++ b/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; } diff --git a/contracts/pool/GaugeV3.sol b/contracts/pool/GaugeV3.sol index fbbf13b3..9568d1cc 100644 --- a/contracts/pool/GaugeV3.sol +++ b/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 diff --git a/contracts/pool/PoolV3.sol b/contracts/pool/PoolV3.sol index b56ccb51..3a395f5d 100644 --- a/contracts/pool/PoolV3.sol +++ b/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(); diff --git a/contracts/pool/TumblerV3.sol b/contracts/pool/TumblerV3.sol index e5d9a566..881fbf58 100644 --- a/contracts/pool/TumblerV3.sol +++ b/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 diff --git a/contracts/test/helpers/IntegrationTestHelper.sol b/contracts/test/helpers/IntegrationTestHelper.sol index 078d8aa3..07da56c5 100644 --- a/contracts/test/helpers/IntegrationTestHelper.sol +++ b/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(); diff --git a/contracts/test/integration/credit/CreditConfigurator.int.t.sol b/contracts/test/integration/credit/CreditConfigurator.int.t.sol index 340dddf0..7e6efd36 100644 --- a/contracts/test/integration/credit/CreditConfigurator.int.t.sol +++ b/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"); diff --git a/contracts/test/integration/credit/LiquidateCreditAccount.int.t.sol b/contracts/test/integration/credit/LiquidateCreditAccount.int.t.sol index 542e3a2f..1ad20535 100644 --- a/contracts/test/integration/credit/LiquidateCreditAccount.int.t.sol +++ b/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); diff --git a/contracts/test/integration/credit/Quotas.int.t.sol b/contracts/test/integration/credit/Quotas.int.t.sol index b4d53bb5..2fe5ed10 100644 --- a/contracts/test/integration/credit/Quotas.int.t.sol +++ b/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) { diff --git a/contracts/test/unit/credit/CreditFacadeV3.unit.t.sol b/contracts/test/unit/credit/CreditFacadeV3.unit.t.sol index 4f7745ed..190bd703 100644 --- a/contracts/test/unit/credit/CreditFacadeV3.unit.t.sol +++ b/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