From 170cc1b21987cb8eab6b720eb3fe1158c2b573db Mon Sep 17 00:00:00 2001 From: Daniel Simon Date: Fri, 3 May 2024 16:44:55 +0700 Subject: [PATCH] fix: double interest on pending redistribution --- contracts/src/BorrowerOperations.sol | 39 +++++++++++++----- .../src/test/interestRateAggregate.t.sol | 40 ++++++++++++++++++- 2 files changed, 67 insertions(+), 12 deletions(-) diff --git a/contracts/src/BorrowerOperations.sol b/contracts/src/BorrowerOperations.sol index 54c9afdd..b7d414ce 100644 --- a/contracts/src/BorrowerOperations.sol +++ b/contracts/src/BorrowerOperations.sol @@ -278,7 +278,7 @@ contract BorrowerOperations is LiquityBase, Ownable, CheckContract, IBorrowerOpe // TODO: Use oldDebt and assert in fuzzing, remove before deployment uint256 oldDebt = troveManager.getTroveEntireDebt(_troveId); _adjustTrove(msg.sender, _troveId, 0, false, _boldAmount, false, 0, contractsCache); - assert(troveManager.getTroveEntireDebt(_troveId) < oldDebt); + assert(troveManager.getTroveEntireDebt(_troveId) < oldDebt); } function adjustTrove( @@ -292,12 +292,19 @@ contract BorrowerOperations is LiquityBase, Ownable, CheckContract, IBorrowerOpe ContractsCacheTMAPBT memory contractsCache = ContractsCacheTMAPBT(troveManager, activePool, boldToken); _requireTroveIsActive(contractsCache.troveManager, _troveId); _adjustTrove( - msg.sender, _troveId, _collChange, _isCollIncrease, _boldChange, _isDebtIncrease, _maxFeePercentage, contractsCache + msg.sender, + _troveId, + _collChange, + _isCollIncrease, + _boldChange, + _isDebtIncrease, + _maxFeePercentage, + contractsCache ); } function adjustUnredeemableTrove( - uint256 _troveId, + uint256 _troveId, uint256 _maxFeePercentage, uint256 _collChange, bool _isCollIncrease, @@ -308,12 +315,21 @@ contract BorrowerOperations is LiquityBase, Ownable, CheckContract, IBorrowerOpe ) external override { ContractsCacheTMAPBT memory contractsCache = ContractsCacheTMAPBT(troveManager, activePool, boldToken); _requireTroveIsUnredeemable(contractsCache.troveManager, _troveId); - // TODO: Gas - pass the cached TM down here, since we fetch it again inside _adjustTrove? + // TODO: Gas - pass the cached TM down here, since we fetch it again inside _adjustTrove? _adjustTrove( - msg.sender, _troveId, _collChange, _isCollIncrease, _boldChange, _isDebtIncrease, _maxFeePercentage, contractsCache + msg.sender, + _troveId, + _collChange, + _isCollIncrease, + _boldChange, + _isDebtIncrease, + _maxFeePercentage, + contractsCache ); troveManager.setTroveStatusToActive(_troveId); - sortedTroves.insert(_troveId, contractsCache.troveManager.getTroveAnnualInterestRate(_troveId), _upperHint, _lowerHint); + sortedTroves.insert( + _troveId, contractsCache.troveManager.getTroveAnnualInterestRate(_troveId), _upperHint, _lowerHint + ); } function adjustTroveInterestRate( @@ -665,9 +681,8 @@ contract BorrowerOperations is LiquityBase, Ownable, CheckContract, IBorrowerOpe uint256 initialWeightedRecordedTroveDebt = _troveManager.getTroveWeightedRecordedDebt(_troveId); // --- Effects --- - (, uint256 redistDebtGain) = _troveManager.getAndApplyRedistributionGains(_troveId); - uint256 accruedTroveInterest = _troveManager.calcTroveAccruedInterest(_troveId); + (, uint256 redistDebtGain) = _troveManager.getAndApplyRedistributionGains(_troveId); uint256 recordedTroveDebt = _troveManager.getTroveDebt(_troveId); uint256 entireTroveDebt = recordedTroveDebt + accruedTroveInterest; uint256 newWeightedTroveDebt = entireTroveDebt * _annualInterestRate; @@ -709,11 +724,13 @@ contract BorrowerOperations is LiquityBase, Ownable, CheckContract, IBorrowerOpe } function _requireTroveIsUnredeemable(ITroveManager _troveManager, uint256 _troveId) internal view { - require(_troveManager.checkTroveIsUnredeemable(_troveId), "BorrowerOps: Trove does not have unredeemable status"); + require( + _troveManager.checkTroveIsUnredeemable(_troveId), "BorrowerOps: Trove does not have unredeemable status" + ); } - function _requireTroveIsNotOpen(ITroveManager _troveManager, uint256 _troveId) internal view { - require(!_troveManager.checkTroveIsOpen(_troveId),"BorrowerOps: Trove is open"); + function _requireTroveIsNotOpen(ITroveManager _troveManager, uint256 _troveId) internal view { + require(!_troveManager.checkTroveIsOpen(_troveId), "BorrowerOps: Trove is open"); } function _requireNonZeroCollChange(uint256 _collChange) internal pure { diff --git a/contracts/src/test/interestRateAggregate.t.sol b/contracts/src/test/interestRateAggregate.t.sol index b3a613e9..b5e0ae88 100644 --- a/contracts/src/test/interestRateAggregate.t.sol +++ b/contracts/src/test/interestRateAggregate.t.sol @@ -2570,7 +2570,45 @@ contract InterestRateAggregate is DevTestSetup { // Check recorded debt sum has changed correctly assertEq(activePool.aggWeightedDebtSum(), expectedAggWeightedRecordedDebt); } - + + // A bug was caught while reading the implementation of `_updateActivePoolTrackersNoDebtChange()`, wherein + // a borrower incurred double interest on redistribution gains when adjusting their interest rate. + // This test case covers that scenario. + // + // We should properly address the TODO below ("tests with pending debt redist. gain >0"), but in the meantime, + // keep this testcase. + function testNoDoubleInterestOnPendingRedistribution() public { + TroveIDs memory troveIDs; + + uint256 coll = 100 ether; + uint256 borrow = 10_000 ether - 200 ether; + uint256 interestRate = 1 ether; + troveIDs.A = openTroveNoHints100pctMaxFee(A, coll, borrow, interestRate); + troveIDs.B = openTroveNoHints100pctMaxFee(B, coll, borrow, interestRate); + troveIDs.C = openTroveNoHints100pctMaxFee(C, coll, borrow, interestRate); + troveIDs.D = openTroveNoHints100pctMaxFee(D, coll, borrow, interestRate); + + emit log_named_decimal_uint("Trove D debt (initial) ", troveManager.getTroveEntireDebt(troveIDs.D), 18); + vm.warp(block.timestamp + 365 days); + emit log_named_decimal_uint("Trove D debt (post-1y) ", troveManager.getTroveEntireDebt(troveIDs.D), 18); + + priceFeed.setPrice(110 ether); + + uint256[] memory liquidatedTroves = new uint256[](3); + liquidatedTroves[0] = troveIDs.A; + liquidatedTroves[1] = troveIDs.B; + liquidatedTroves[2] = troveIDs.C; + troveManager.batchLiquidateTroves(liquidatedTroves); + + uint256 debtBefore = troveManager.getTroveEntireDebt(troveIDs.D); + emit log_named_decimal_uint("Trove D debt (post-liq) ", debtBefore, 18); + changeInterestRateNoHints(D, troveIDs.D, 0.1 ether); + uint256 debtAfter = troveManager.getTroveEntireDebt(troveIDs.D); + emit log_named_decimal_uint("Trove D debt (post-adj) ", debtAfter, 18); + + assertEq(debtBefore, debtAfter, "Adjusting interest rate shouldn't change Trove's debt"); + } + // TODO: mixed collateral & debt adjustment opps // TODO: tests with pending debt redist. gain >0 // TODO: tests that show total debt change under user ops