From 5d87d2f68cb8e23393e901bf15c66c56f9ba8ea4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=9Fingen?= Date: Mon, 12 Apr 2021 18:13:57 +0200 Subject: [PATCH 1/4] contracts: Add comments to TroveManager --- packages/contracts/contracts/TroveManager.sol | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/contracts/contracts/TroveManager.sol b/packages/contracts/contracts/TroveManager.sol index 147e433533..37a58c2810 100644 --- a/packages/contracts/contracts/TroveManager.sol +++ b/packages/contracts/contracts/TroveManager.sol @@ -788,10 +788,16 @@ contract TroveManager is LiquityBase, Ownable, CheckContract, ITroveManager { } function _sendGasCompensation(IActivePool _activePool, address _liquidator, uint _LUSD, uint _ETH) internal { + // Before calling this function, we always check that something was liquidated, otherwise revert. + // LUSD gas compensation could then only be zero if we set to zero that constant, but it’s ok to have this here as a sanity check if (_LUSD > 0) { lusdToken.returnFromPool(gasPoolAddress, _liquidator, _LUSD); } + // ETH gas compensation could only be zero if all liquidated troves in the sequence had collateral lower than 200 Wei + // (see _getCollGasCompensation function in LiquityBase) + // With the current values of min debt this seems quite unlikely, unless ETH price was in the order of magnitude of $10^19 or more, + // but it’s ok to have this here as a sanity check if (_ETH > 0) { _activePool.sendETH(_liquidator, _ETH); } From bf8bc1f8ddfbd11b0c0c9254c8a9fc141172c172 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=9Fingen?= Date: Mon, 12 Apr 2021 18:26:16 +0200 Subject: [PATCH 2/4] contracts: Remove unused _requireCallerIsBorrower ...from BorrowerOperations. --- packages/contracts/contracts/BorrowerOperations.sol | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/contracts/contracts/BorrowerOperations.sol b/packages/contracts/contracts/BorrowerOperations.sol index 589392ec46..834aa33f7f 100644 --- a/packages/contracts/contracts/BorrowerOperations.sol +++ b/packages/contracts/contracts/BorrowerOperations.sol @@ -466,10 +466,6 @@ contract BorrowerOperations is LiquityBase, Ownable, CheckContract, IBorrowerOpe require(msg.value == 0 || _collWithdrawal == 0, "BorrowerOperations: Cannot withdraw and add coll"); } - function _requireCallerIsBorrower(address _borrower) internal view { - require(msg.sender == _borrower, "BorrowerOps: Caller must be the borrower for a withdrawal"); - } - function _requireNonZeroAdjustment(uint _collWithdrawal, uint _LUSDChange) internal view { require(msg.value != 0 || _collWithdrawal != 0 || _LUSDChange != 0, "BorrowerOps: There must be either a collateral change or a debt change"); } From 691f19fee143c36fac93e4816599bc57bbe07042 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=9Fingen?= Date: Mon, 12 Apr 2021 22:49:05 +0200 Subject: [PATCH 3/4] contracts: Remove unnecessary _requireValidLUSDRepayment Minor refactor to _adjustTrove. --- .../contracts/BorrowerOperations.sol | 27 ++++++++----------- .../contracts/test/BorrowerOperationsTest.js | 11 ++++++++ 2 files changed, 22 insertions(+), 16 deletions(-) diff --git a/packages/contracts/contracts/BorrowerOperations.sol b/packages/contracts/contracts/BorrowerOperations.sol index 834aa33f7f..eb73ab9b3f 100644 --- a/packages/contracts/contracts/BorrowerOperations.sol +++ b/packages/contracts/contracts/BorrowerOperations.sol @@ -170,7 +170,7 @@ contract BorrowerOperations is LiquityBase, Ownable, CheckContract, IBorrowerOpe vars.LUSDFee = _triggerBorrowingFee(contractsCache.troveManager, contractsCache.lusdToken, _LUSDAmount, _maxFeePercentage); vars.netDebt = vars.netDebt.add(vars.LUSDFee); } - _requireAtLeastMinNetDebt(vars.netDebt); + _requireAtLeastMinNetDebt(vars.netDebt, 0); // ICR is based on the composite debt, i.e. the requested LUSD amount + LUSD borrowing fee + LUSD gas comp. vars.compositeDebt = _getCompositeDebt(vars.netDebt); @@ -279,21 +279,20 @@ contract BorrowerOperations is LiquityBase, Ownable, CheckContract, IBorrowerOpe vars.debt = contractsCache.troveManager.getTroveDebt(_borrower); vars.coll = contractsCache.troveManager.getTroveColl(_borrower); - + + // When the adjustment is a debt repayment, check it's a valid amount and that the caller has enough LUSD + if (!_isDebtIncrease && _LUSDChange > 0) { + _requireAtLeastMinNetDebt(_getNetDebt(vars.debt), vars.netDebtChange); + _requireSufficientLUSDBalance(contractsCache.lusdToken, _borrower, vars.netDebtChange); + } + // Get the trove's old ICR before the adjustment, and what its new ICR will be after the adjustment vars.oldICR = LiquityMath._computeCR(vars.coll, vars.debt, vars.price); vars.newICR = _getNewICRFromTroveChange(vars.coll, vars.debt, vars.collChange, vars.isCollIncrease, vars.netDebtChange, _isDebtIncrease, vars.price); - assert(_collWithdrawal <= vars.coll); + assert(_collWithdrawal <= vars.coll); // Check the adjustment satisfies all conditions for the current system mode _requireValidAdjustmentInCurrentMode(isRecoveryMode, _collWithdrawal, _isDebtIncrease, vars); - - // When the adjustment is a debt repayment, check it's a valid amount and that the caller has enough LUSD - if (!_isDebtIncrease && _LUSDChange > 0) { - _requireAtLeastMinNetDebt(_getNetDebt(vars.debt).sub(vars.netDebtChange)); - _requireValidLUSDRepayment(vars.debt, vars.netDebtChange); - _requireSufficientLUSDBalance(contractsCache.lusdToken, _borrower, vars.netDebtChange); - } (vars.newColl, vars.newDebt) = _updateTroveFromAdjustment(contractsCache.troveManager, _borrower, vars.collChange, vars.isCollIncrease, vars.netDebtChange, _isDebtIncrease); vars.stake = contractsCache.troveManager.updateStakeAndTotalStakes(_borrower); @@ -544,12 +543,8 @@ contract BorrowerOperations is LiquityBase, Ownable, CheckContract, IBorrowerOpe require(_newTCR >= CCR, "BorrowerOps: An operation that would result in TCR < CCR is not permitted"); } - function _requireAtLeastMinNetDebt(uint _netDebt) internal pure { - require (_netDebt >= MIN_NET_DEBT, "BorrowerOps: Trove's net debt must be greater than minimum"); - } - - function _requireValidLUSDRepayment(uint _currentDebt, uint _debtRepayment) internal pure { - require(_debtRepayment <= _currentDebt.sub(LUSD_GAS_COMPENSATION), "BorrowerOps: Amount repaid must not be larger than the Trove's debt"); + function _requireAtLeastMinNetDebt(uint _currentNetDebt, uint _debtRepayment) internal pure { + require (_currentNetDebt >= MIN_NET_DEBT.add(_debtRepayment), "BorrowerOps: Trove's net debt must be greater than minimum"); } function _requireCallerIsStabilityPool() internal view { diff --git a/packages/contracts/test/BorrowerOperationsTest.js b/packages/contracts/test/BorrowerOperationsTest.js index b98ee259a2..3017bcbc89 100644 --- a/packages/contracts/test/BorrowerOperationsTest.js +++ b/packages/contracts/test/BorrowerOperationsTest.js @@ -1365,6 +1365,17 @@ contract('BorrowerOperations', async accounts => { await assertRevert(repayTxAPromise, "BorrowerOps: Trove's net debt must be greater than minimum") }) + it("adjustTrove(): Reverts if repaid amount is greater than current debt", async () => { + const { totalDebt } = await openTrove({ extraLUSDAmount: toBN(dec(10000, 18)), ICR: toBN(dec(10, 18)), extraParams: { from: alice } }) + const repayAmount = totalDebt.add(toBN(1)) + await openTrove({ extraLUSDAmount: repayAmount, ICR: toBN(dec(150, 16)), extraParams: { from: bob } }) + + await lusdToken.transfer(alice, repayAmount, { from: bob }) + + await assertRevert(borrowerOperations.adjustTrove(th._100pct, 0, repayAmount, false, alice, alice, { from: alice }), + "BorrowerOps: Trove's net debt must be greater than minimum") + }) + it("repayLUSD(): reverts when calling address does not have active trove", async () => { await openTrove({ extraLUSDAmount: toBN(dec(10000, 18)), ICR: toBN(dec(2, 18)), extraParams: { from: alice } }) await openTrove({ extraLUSDAmount: toBN(dec(10000, 18)), ICR: toBN(dec(2, 18)), extraParams: { from: bob } }) From f4a21a427b48ffbcf2877b6d2fe99f8afd5f5352 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=9Fingen?= Date: Mon, 12 Apr 2021 22:56:08 +0200 Subject: [PATCH 4/4] =?UTF-8?q?contracts:=20Don=E2=80=99t=20check=20max=20?= =?UTF-8?q?fee=20percentage=20in=20Recovery=20Mode?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ...for borrowing operations. --- packages/contracts/contracts/BorrowerOperations.sol | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/packages/contracts/contracts/BorrowerOperations.sol b/packages/contracts/contracts/BorrowerOperations.sol index eb73ab9b3f..d6db169c56 100644 --- a/packages/contracts/contracts/BorrowerOperations.sol +++ b/packages/contracts/contracts/BorrowerOperations.sol @@ -556,13 +556,11 @@ contract BorrowerOperations is LiquityBase, Ownable, CheckContract, IBorrowerOpe } function _requireValidMaxFeePercentage(uint _maxFeePercentage, bool _isRecoveryMode) internal pure { - if (_isRecoveryMode) { - require(_maxFeePercentage <= DECIMAL_PRECISION, - "Max fee percentage must less than or equal to 100%"); - } else { - require(_maxFeePercentage >= BORROWING_FEE_FLOOR && _maxFeePercentage <= DECIMAL_PRECISION, - "Max fee percentage must be between 0.5% and 100%"); - } + // In recovery mode we are not charging borrowing fee, so we can ignore this param + if (_isRecoveryMode) { return; } + + require(_maxFeePercentage >= BORROWING_FEE_FLOOR && _maxFeePercentage <= DECIMAL_PRECISION, + "Max fee percentage must be between 0.5% and 100%"); } // --- ICR and TCR getters ---