From b871eabaf65322e54507c7c8e3aae1db3aa9aa66 Mon Sep 17 00:00:00 2001 From: Debugger022 Date: Wed, 13 Sep 2023 13:47:10 +0530 Subject: [PATCH 1/5] feat: ven-1930 force liquidation --- contracts/Comptroller.sol | 23 ++++++++++- contracts/ComptrollerStorage.sol | 5 ++- .../Comptroller/liquidateAccountTest.ts | 38 ++++++++++++++++++- 3 files changed, 63 insertions(+), 3 deletions(-) diff --git a/contracts/Comptroller.sol b/contracts/Comptroller.sol index 9d2de5830..75d88f7db 100644 --- a/contracts/Comptroller.sol +++ b/contracts/Comptroller.sol @@ -91,6 +91,9 @@ contract Comptroller is /// @notice Emitted when a market is supported event MarketSupported(VToken vToken); + /// @notice Emitted when forced liquidation is enabled or disabled for a market + event IsForcedLiquidationEnabledUpdated(address indexed vToken, bool enable); + /// @notice Thrown when collateral factor exceeds the upper bound error InvalidCollateralFactor(); @@ -475,7 +478,7 @@ contract Comptroller is uint256 borrowBalance = VToken(vTokenBorrowed).borrowBalanceStored(borrower); /* Allow accounts to be liquidated if the market is deprecated or it is a forced liquidation */ - if (skipLiquidityCheck || isDeprecated(VToken(vTokenBorrowed))) { + if (skipLiquidityCheck || isDeprecated(VToken(vTokenBorrowed)) || isForcedLiquidationEnabled[vTokenBorrowed]) { if (repayAmount > borrowBalance) { revert TooMuchRepay(); } @@ -1004,6 +1007,24 @@ contract Comptroller is _setMaxLoopsLimit(limit); } + /** + * @notice Enables forced liquidations for a market. If forced liquidation is enabled, + * borrows in the market may be liquidated regardless of the account liquidity + * @param vTokenBorrowed Borrowed vToken + * @param enable Whether to enable forced liquidations + */ + function setForcedLiquidation(address vTokenBorrowed, bool enable) external { + _checkAccessAllowed("setForcedLiquidation(address,bool)"); + ensureNonzeroAddress(vTokenBorrowed); + + if (!markets[vTokenBorrowed].isListed) { + revert MarketNotListed(vTokenBorrowed); + } + + isForcedLiquidationEnabled[address(vTokenBorrowed)] = enable; + emit IsForcedLiquidationEnabledUpdated(vTokenBorrowed, enable); + } + /** * @notice Determine the current account liquidity with respect to liquidation threshold requirements * @dev The interface of this function is intentionally kept compatible with Compound and Venus Core diff --git a/contracts/ComptrollerStorage.sol b/contracts/ComptrollerStorage.sol index b710ca4a4..af90d8d23 100644 --- a/contracts/ComptrollerStorage.sol +++ b/contracts/ComptrollerStorage.sol @@ -118,10 +118,13 @@ contract ComptrollerStorage { // No collateralFactorMantissa may exceed this value uint256 internal constant MAX_COLLATERAL_FACTOR_MANTISSA = 0.9e18; // 0.9 + /// @notice Flag indicating whether forced liquidation enabled for a market + mapping(address => bool) public isForcedLiquidationEnabled; + /** * @dev This empty reserved space is put in place to allow future versions to add new * variables without shifting down storage in the inheritance chain. * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps */ - uint256[50] private __gap; + uint256[49] private __gap; } diff --git a/tests/hardhat/Comptroller/liquidateAccountTest.ts b/tests/hardhat/Comptroller/liquidateAccountTest.ts index 5c32ff465..cdaf06a81 100644 --- a/tests/hardhat/Comptroller/liquidateAccountTest.ts +++ b/tests/hardhat/Comptroller/liquidateAccountTest.ts @@ -53,6 +53,7 @@ describe("liquidateAccount", () => { let OMG: FakeContract; let ZRX: FakeContract; let BAT: FakeContract; + let accessControl: FakeContract; const maxLoopsLimit = 150; type LiquidateAccountFixture = { @@ -113,7 +114,7 @@ describe("liquidateAccount", () => { [, liquidator, user] = await ethers.getSigners(); const contracts = await loadFixture(liquidateAccountFixture); configure(contracts); - ({ comptroller, OMG, ZRX, BAT } = contracts); + ({ comptroller, OMG, ZRX, BAT, accessControl } = contracts); }); describe("collateral to borrows ratio requirements", async () => { @@ -342,4 +343,39 @@ describe("liquidateAccount", () => { ); }); }); + + describe("setForcedLiquidation", async () => { + it("fails if asset is not listed", async () => { + const someVToken = await smock.fake("VToken"); + await expect(comptroller.setForcedLiquidation(someVToken.address, true)).to.be.revertedWithCustomError( + comptroller, + "MarketNotListed", + ); + }); + + it("fails if ACM does not allow the call", async () => { + accessControl.isAllowedToCall.returns(false); + await expect(comptroller.setForcedLiquidation(OMG.address, true)).to.be.revertedWithCustomError( + comptroller, + "Unauthorized", + ); + accessControl.isAllowedToCall.returns(true); + }); + + it("sets forced liquidation", async () => { + await comptroller.setForcedLiquidation(OMG.address, true); + expect(await comptroller.isForcedLiquidationEnabled(OMG.address)).to.be.true; + + await comptroller.setForcedLiquidation(OMG.address, false); + expect(await comptroller.isForcedLiquidationEnabled(OMG.address)).to.be.false; + }); + + it("emits IsForcedLiquidationEnabledUpdated event", async () => { + const tx1 = await comptroller.setForcedLiquidation(OMG.address, true); + await expect(tx1).to.emit(comptroller, "IsForcedLiquidationEnabledUpdated").withArgs(OMG.address, true); + + const tx2 = await comptroller.setForcedLiquidation(OMG.address, false); + await expect(tx2).to.emit(comptroller, "IsForcedLiquidationEnabledUpdated").withArgs(OMG.address, false); + }); + }); }); From e062119483c18ddea4e0a3ab399179867a89188a Mon Sep 17 00:00:00 2001 From: Debugger022 Date: Wed, 13 Sep 2023 16:50:59 +0530 Subject: [PATCH 2/5] tests: pre liquidate hook --- .../Comptroller/liquidateAccountTest.ts | 90 +++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/tests/hardhat/Comptroller/liquidateAccountTest.ts b/tests/hardhat/Comptroller/liquidateAccountTest.ts index cdaf06a81..e5b006e14 100644 --- a/tests/hardhat/Comptroller/liquidateAccountTest.ts +++ b/tests/hardhat/Comptroller/liquidateAccountTest.ts @@ -378,4 +378,94 @@ describe("liquidateAccount", () => { await expect(tx2).to.emit(comptroller, "IsForcedLiquidationEnabledUpdated").withArgs(OMG.address, false); }); }); + + describe("preLiquidateHook", async () => { + let accounts: SignerWithAddress[]; + + before(async () => { + accounts = await ethers.getSigners(); + }); + const generalTests = () => { + it("reverts if borrowed market is not listed", async () => { + const someVToken = await smock.fake("VToken"); + await expect( + comptroller.preLiquidateHook( + someVToken.address, + OMG.address, + accounts[0].address, + parseUnits("1", 18), + false, + ), + ).to.be.revertedWithCustomError(comptroller, "MarketNotListed"); + }); + + it("reverts if collateral market is not listed", async () => { + const someVToken = await smock.fake("VToken"); + await expect( + comptroller.preLiquidateHook( + OMG.address, + someVToken.address, + accounts[0].address, + parseUnits("1", 18), + false, + ), + ).to.be.revertedWithCustomError(comptroller, "MarketNotListed"); + }); + }; + + describe("isForcedLiquidationEnabled == true", async () => { + beforeEach(async () => { + await comptroller.setForcedLiquidation(OMG.address, true); + }); + + generalTests(); + + it("allows liquidations without shortfall", async () => { + OMG.borrowBalanceStored.returns(parseUnits("100", 18)); + await comptroller.callStatic.preLiquidateHook( + OMG.address, + OMG.address, + accounts[0].address, + parseUnits("1", 18), + true, + ); + }); + + it("allows to repay 100% of the borrow", async () => { + OMG.borrowBalanceStored.returns(parseUnits("1", 18)); + await comptroller.callStatic.preLiquidateHook( + OMG.address, + OMG.address, + accounts[0].address, + parseUnits("1", 18), + false, + ); + }); + + it("fails with TOO_MUCH_REPAY if trying to repay > borrowed amount", async () => { + OMG.borrowBalanceStored.returns(parseUnits("0.99", 18)); + const tx = comptroller.callStatic.preLiquidateHook( + OMG.address, + OMG.address, + accounts[0].address, + parseUnits("1", 18), + false, + ); + await expect(tx).to.be.revertedWithCustomError(comptroller, "TooMuchRepay"); + }); + + it("checks the shortfall if isForcedLiquidationEnabled is set back to false", async () => { + await comptroller.setForcedLiquidation(OMG.address, false); + OMG.borrowBalanceStored.returns(parseUnits("100", 18)); + const tx = comptroller.callStatic.preLiquidateHook( + OMG.address, + OMG.address, + accounts[0].address, + parseUnits("1", 18), + false, + ); + await expect(tx).to.be.revertedWithCustomError(comptroller, "MinimalCollateralViolated"); + }); + }); + }); }); From cbd9b18a99c4e1f92bf9404e88fceb8ebc36d55f Mon Sep 17 00:00:00 2001 From: Debugger022 Date: Fri, 15 Sep 2023 16:58:20 +0530 Subject: [PATCH 3/5] fix: pr comments --- contracts/Comptroller.sol | 15 +- contracts/ComptrollerStorage.sol | 6 +- .../Comptroller/liquidateAccountTest.ts | 132 ++++++++---------- 3 files changed, 60 insertions(+), 93 deletions(-) diff --git a/contracts/Comptroller.sol b/contracts/Comptroller.sol index 75d88f7db..33a4fe45c 100644 --- a/contracts/Comptroller.sol +++ b/contracts/Comptroller.sol @@ -478,7 +478,7 @@ contract Comptroller is uint256 borrowBalance = VToken(vTokenBorrowed).borrowBalanceStored(borrower); /* Allow accounts to be liquidated if the market is deprecated or it is a forced liquidation */ - if (skipLiquidityCheck || isDeprecated(VToken(vTokenBorrowed)) || isForcedLiquidationEnabled[vTokenBorrowed]) { + if (skipLiquidityCheck || isForcedLiquidationEnabled[vTokenBorrowed]) { if (repayAmount > borrowBalance) { revert TooMuchRepay(); } @@ -1244,19 +1244,6 @@ contract Comptroller is return _actionPaused[market][action]; } - /** - * @notice Check if a vToken market has been deprecated - * @dev All borrows in a deprecated vToken market can be immediately liquidated - * @param vToken The market to check if deprecated - * @return deprecated True if the given vToken market has been deprecated - */ - function isDeprecated(VToken vToken) public view returns (bool) { - return - markets[address(vToken)].collateralFactorMantissa == 0 && - actionPaused(address(vToken), Action.BORROW) && - vToken.reserveFactorMantissa() == MANTISSA_ONE; - } - /** * @notice Add the market to the borrower's "assets in" for liquidity calculations * @param vToken The market to enter diff --git a/contracts/ComptrollerStorage.sol b/contracts/ComptrollerStorage.sol index af90d8d23..6732a6349 100644 --- a/contracts/ComptrollerStorage.sol +++ b/contracts/ComptrollerStorage.sol @@ -107,6 +107,9 @@ contract ComptrollerStorage { // Used to check if rewards distributor is added mapping(address => bool) internal rewardsDistributorExists; + /// @notice Flag indicating whether forced liquidation enabled for a market + mapping(address => bool) public isForcedLiquidationEnabled; + uint256 internal constant NO_ERROR = 0; // closeFactorMantissa must be strictly greater than this value @@ -118,9 +121,6 @@ contract ComptrollerStorage { // No collateralFactorMantissa may exceed this value uint256 internal constant MAX_COLLATERAL_FACTOR_MANTISSA = 0.9e18; // 0.9 - /// @notice Flag indicating whether forced liquidation enabled for a market - mapping(address => bool) public isForcedLiquidationEnabled; - /** * @dev This empty reserved space is put in place to allow future versions to add new * variables without shifting down storage in the inheritance chain. diff --git a/tests/hardhat/Comptroller/liquidateAccountTest.ts b/tests/hardhat/Comptroller/liquidateAccountTest.ts index e5b006e14..75fbd4f06 100644 --- a/tests/hardhat/Comptroller/liquidateAccountTest.ts +++ b/tests/hardhat/Comptroller/liquidateAccountTest.ts @@ -382,90 +382,70 @@ describe("liquidateAccount", () => { describe("preLiquidateHook", async () => { let accounts: SignerWithAddress[]; - before(async () => { + beforeEach(async () => { accounts = await ethers.getSigners(); + await comptroller.setForcedLiquidation(OMG.address, true); }); - const generalTests = () => { - it("reverts if borrowed market is not listed", async () => { - const someVToken = await smock.fake("VToken"); - await expect( - comptroller.preLiquidateHook( - someVToken.address, - OMG.address, - accounts[0].address, - parseUnits("1", 18), - false, - ), - ).to.be.revertedWithCustomError(comptroller, "MarketNotListed"); - }); - it("reverts if collateral market is not listed", async () => { - const someVToken = await smock.fake("VToken"); - await expect( - comptroller.preLiquidateHook( - OMG.address, - someVToken.address, - accounts[0].address, - parseUnits("1", 18), - false, - ), - ).to.be.revertedWithCustomError(comptroller, "MarketNotListed"); - }); - }; + it("reverts if borrowed market is not listed", async () => { + const someVToken = await smock.fake("VToken"); + await expect( + comptroller.preLiquidateHook(someVToken.address, OMG.address, accounts[0].address, parseUnits("1", 18), false), + ).to.be.revertedWithCustomError(comptroller, "MarketNotListed"); + }); - describe("isForcedLiquidationEnabled == true", async () => { - beforeEach(async () => { - await comptroller.setForcedLiquidation(OMG.address, true); - }); + it("reverts if collateral market is not listed", async () => { + const someVToken = await smock.fake("VToken"); + await expect( + comptroller.preLiquidateHook(OMG.address, someVToken.address, accounts[0].address, parseUnits("1", 18), false), + ).to.be.revertedWithCustomError(comptroller, "MarketNotListed"); + }); - generalTests(); - - it("allows liquidations without shortfall", async () => { - OMG.borrowBalanceStored.returns(parseUnits("100", 18)); - await comptroller.callStatic.preLiquidateHook( - OMG.address, - OMG.address, - accounts[0].address, - parseUnits("1", 18), - true, - ); - }); + it("allows liquidations without shortfall", async () => { + OMG.borrowBalanceStored.returns(parseUnits("100", 18)); + await comptroller.callStatic.preLiquidateHook( + OMG.address, + OMG.address, + accounts[0].address, + parseUnits("1", 18), + true, + ); + }); - it("allows to repay 100% of the borrow", async () => { - OMG.borrowBalanceStored.returns(parseUnits("1", 18)); - await comptroller.callStatic.preLiquidateHook( - OMG.address, - OMG.address, - accounts[0].address, - parseUnits("1", 18), - false, - ); - }); + it("allows to repay 100% of the borrow", async () => { + OMG.borrowBalanceStored.returns(parseUnits("1", 18)); + await comptroller.callStatic.preLiquidateHook( + OMG.address, + OMG.address, + accounts[0].address, + parseUnits("1", 18), + false, + ); + }); - it("fails with TOO_MUCH_REPAY if trying to repay > borrowed amount", async () => { - OMG.borrowBalanceStored.returns(parseUnits("0.99", 18)); - const tx = comptroller.callStatic.preLiquidateHook( - OMG.address, - OMG.address, - accounts[0].address, - parseUnits("1", 18), - false, - ); - await expect(tx).to.be.revertedWithCustomError(comptroller, "TooMuchRepay"); - }); + it("fails with TOO_MUCH_REPAY if trying to repay > borrowed amount", async () => { + OMG.borrowBalanceStored.returns(parseUnits("0.99", 18)); + const tx = comptroller.callStatic.preLiquidateHook( + OMG.address, + OMG.address, + accounts[0].address, + parseUnits("1", 18), + false, + ); + await expect(tx).to.be.revertedWithCustomError(comptroller, "TooMuchRepay"); + }); - it("checks the shortfall if isForcedLiquidationEnabled is set back to false", async () => { - await comptroller.setForcedLiquidation(OMG.address, false); - OMG.borrowBalanceStored.returns(parseUnits("100", 18)); - const tx = comptroller.callStatic.preLiquidateHook( - OMG.address, - OMG.address, - accounts[0].address, - parseUnits("1", 18), - false, - ); - await expect(tx).to.be.revertedWithCustomError(comptroller, "MinimalCollateralViolated"); - }); + it("checks the shortfall if isForcedLiquidationEnabled is set back to false", async () => { + await comptroller.setForcedLiquidation(OMG.address, false); + OMG.borrowBalanceStored.returns(parseUnits("100", 18)); + const tx = comptroller.callStatic.preLiquidateHook( + OMG.address, + OMG.address, + accounts[0].address, + parseUnits("1", 18), + false, + ); + await expect(tx).to.be.revertedWithCustomError(comptroller, "MinimalCollateralViolated"); }); }); }); From f4e8d2b5517ad6b104cffcdbe03c9eb2fd94ddbc Mon Sep 17 00:00:00 2001 From: Debugger022 Date: Thu, 12 Oct 2023 14:07:56 +0530 Subject: [PATCH 4/5] fix: CVP-04 --- contracts/Comptroller.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/Comptroller.sol b/contracts/Comptroller.sol index 33a4fe45c..1ae39027f 100644 --- a/contracts/Comptroller.sol +++ b/contracts/Comptroller.sol @@ -1021,7 +1021,7 @@ contract Comptroller is revert MarketNotListed(vTokenBorrowed); } - isForcedLiquidationEnabled[address(vTokenBorrowed)] = enable; + isForcedLiquidationEnabled[vTokenBorrowed] = enable; emit IsForcedLiquidationEnabledUpdated(vTokenBorrowed, enable); } From ebc9a9b043064e6fe4af2ac48fdc24e24eddba58 Mon Sep 17 00:00:00 2001 From: Debugger022 Date: Thu, 12 Oct 2023 14:09:47 +0530 Subject: [PATCH 5/5] fix: CVP-03 --- contracts/Comptroller.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/Comptroller.sol b/contracts/Comptroller.sol index 1ae39027f..525d29650 100644 --- a/contracts/Comptroller.sol +++ b/contracts/Comptroller.sol @@ -477,7 +477,7 @@ contract Comptroller is uint256 borrowBalance = VToken(vTokenBorrowed).borrowBalanceStored(borrower); - /* Allow accounts to be liquidated if the market is deprecated or it is a forced liquidation */ + /* Allow accounts to be liquidated if it is a forced liquidation */ if (skipLiquidityCheck || isForcedLiquidationEnabled[vTokenBorrowed]) { if (repayAmount > borrowBalance) { revert TooMuchRepay();