contracts: Address PR #81 comments

contracts/src/BorrowerOperations.sol

@@ -272,6 +272,7 @@ contract BorrowerOperations is LiquityBase, Ownable, CheckContract, IBorrowerOpe
  _requireValidAnnualInterestRate(_newAnnualInterestRate);
  ITroveManager troveManagerCached = troveManager;
  _requireTroveisActive(troveManagerCached, _troveId);
+ _requireIsOwner(_troveId);
 
  // TODO: apply individual and aggregate pending interest, and take snapshots of current timestamp.
  // TODO: determine how applying pending interest should interact / be sequenced with applying pending rewards from redistributions. 
@@ -422,8 +423,7 @@ contract BorrowerOperations is LiquityBase, Ownable, CheckContract, IBorrowerOpe
  * Claim remaining collateral from a redemption or from a liquidation with ICR > MCR in Recovery Mode
  */
  function claimCollateral(uint256 _troveId) external override {
- address owner = troveManager.ownerOf(_troveId);
- require(owner == msg.sender, "BO: Only owner can claim trove collateral");
+ _requireIsOwner(_troveId);
 
  // send ETH from CollSurplus Pool to owner
  collSurplusPool.claimColl(msg.sender, _troveId);
@@ -534,6 +534,10 @@ contract BorrowerOperations is LiquityBase, Ownable, CheckContract, IBorrowerOpe
  require(_collChange != 0 || _boldChange != 0, "BorrowerOps: There must be either a collateral change or a debt change");
  }
 
+ function _requireIsOwner(uint256 _troveId) internal view {
+ require(troveManager.ownerOf(_troveId) == msg.sender, "BO: Only owner");
+ }
+
  function _requireTroveisActive(ITroveManager _troveManager, uint256 _troveId) internal view {
  uint status = _troveManager.getTroveStatus(_troveId);
  require(status == 1, "BorrowerOps: Trove does not exist or is closed");

contracts/src/TroveManager.sol

@@ -889,8 +889,11 @@ contract TroveManager is ERC721, LiquityBase, Ownable, CheckContract, ITroveMana
  _maxIterations--;
  // Save the uint256 of the Trove preceding the current one, before potentially modifying the list
  uint256 nextUserToCheck = contractsCache.sortedTroves.getPrev(currentTroveId);
- // TODO: check ICR?
- //getCurrentICR(currentTroveId, _price) < MCR
+ // Skip if ICR < 100%, to make sure that redemptions always improve the CR of hit Troves
+ if (getCurrentICR(currentTroveId, totals.price) < _100pct) {
+ currentTroveId = nextUserToCheck;
+ continue;
+ }
 
  _applyPendingRewards(contractsCache.activePool, contractsCache.defaultPool, currentTroveId);
 
@@ -1427,7 +1430,6 @@ contract TroveManager is ERC721, LiquityBase, Ownable, CheckContract, ITroveMana
  _updateTroveRewardSnapshots(_troveId);
 
  // mint ERC721
- // TODO: Should we use safeMint? I guess not
  _mint(_owner, _troveId);
 
  return _updateStakeAndTotalStakes(_troveId);

contracts/src/test/TestContracts/BaseTest.sol

@@ -32,6 +32,7 @@ contract BaseTest is Test {
 
  uint256 public constant MAX_UINT256 = type(uint256).max;
  uint256 public constant SECONDS_IN_1_YEAR = 31536000; // 60*60*24*365
+ uint256 _100pct = 100e16;
  uint256 MCR = 110e16;
  uint256 CCR = 150e16;
  address public constant ZERO_ADDRESS = address(0);

contracts/src/test/interestRateBasic.t.sol

@@ -71,6 +71,20 @@ contract InterestRateBasic is DevTestSetup {
  borrowerOperations.openTrove(A, 1, 1e18, 2e18, 2000e18, 0, 0, 42e18);
  }
 
+ function testRevertWhenAdjustInterestRateFromNonOwner() public {
+ priceFeed.setPrice(2000e18);
+
+ // A opens Trove
+ uint256 A_Id = openTroveNoHints100pctMaxFee(A, 2 ether, 2000e18, 37e16);
+ assertEq(troveManager.getTroveAnnualInterestRate(A_Id), 37e16);
+
+ // B (who is not delegate) tries to adjust it
+ vm.startPrank(B);
+ vm.expectRevert("BO: Only owner");
+ borrowerOperations.adjustTroveInterestRate(A_Id, 40e16, 0, 0);
+ vm.stopPrank();
+ }
+
  function testRevertWhenAdjustInterestRateGreaterThanMax() public {
  priceFeed.setPrice(2000e18);
 

contracts/src/test/troveManager.t.sol

@@ -0,0 +1,54 @@
+pragma solidity 0.8.18;
+
+import "./TestContracts/DevTestSetup.sol";
+
+
+contract TroveManagerTest is DevTestSetup {
+
+ function testRedeemSkipTrovesUnder100pct() public {
+ priceFeed.setPrice(2000e18);
+ uint256 ATroveId = openTroveNoHints100pctMaxFee(A, 2 ether, 2001e18, 1e17);
+ uint256 BTroveId = openTroveNoHints100pctMaxFee(B, 5 ether, 2000e18, 2e17);
+ uint256 CTroveId = openTroveNoHints100pctMaxFee(C, 5 ether, 2000e18, 3e17);
+
+ uint256 debtA1 = troveManager.getTroveDebt(ATroveId);
+ assertGt(debtA1, 0);
+ uint256 collA1 = troveManager.getTroveColl(ATroveId);
+ assertGt(collA1, 0);
+ uint256 debtB1 = troveManager.getTroveDebt(BTroveId);
+ assertGt(debtB1, 0);
+ uint256 collB1 = troveManager.getTroveColl(BTroveId);
+ assertGt(collB1, 0);
+
+ // fast-forward until redemptions are enabled
+ vm.warp(block.timestamp + troveManager.BOOTSTRAP_PERIOD() + 1);
+ // Reduce ETH price so A’s ICR goes below 100%
+ uint256 newPrice = 1000e18;
+ priceFeed.setPrice(newPrice);
+ assertLt(troveManager.getCurrentICR(ATroveId, newPrice), _100pct);
+ assertGt(troveManager.getCurrentICR(BTroveId, newPrice), _100pct);
+
+ uint256 redemptionAmount = 1000e18; // 1k BOLD
+
+ // C redeems 1k BOLD
+ vm.startPrank(C);
+ troveManager.redeemCollateral(
+ redemptionAmount,
+ 10,
+ 1e18
+ );
+ vm.stopPrank();
+
+ // Check A's coll and debt are the same
+ uint256 debtA2 = troveManager.getTroveDebt(ATroveId);
+ assertEq(debtA2, debtA1, "A debt mismatch");
+ uint256 collA2 = troveManager.getTroveColl(ATroveId);
+ assertEq(collA2, collA1, "A coll mismatch");
+
+ // Check B's coll and debt reduced
+ uint256 debtB2 = troveManager.getTroveDebt(BTroveId);
+ assertLt(debtB2, debtB1, "B debt mismatch");
+ uint256 collB2 = troveManager.getTroveColl(BTroveId);
+ assertLt(collB2, collB1, "B coll mismatch");
+ }
+}