Invariant testing fixes (#1006)

* initial commit * tweaks to Matt's PR to block adding quote token above auction price (#1000) * reduce cost of reference price assignment * reduce pool contract size * fixed testDepositTakeAndSettleByRegularTakeSubsetPool * fixed tests in ERC20PoolLiquidationsArbTake.t.sol * fixed tests in ERC20PoolLiquidationsDepositTake.sol * fixed two more * updated testDepositTakeAndSettleSubsetPool * updated testKickAndSettleSubsetPoolFractionalCollateral * updated testSettleWithDepositFuzzy * Fixed final tests * add "AddAboveAuctionPrice" as expected pool error * implemented invariant A9: reference prices in liquidation queue shall not decrease * Update assertAuction to use ThresholdPrice from auctionInfo (#1003) * use auctionInfo thresholdprice instead of recalculating * fix most tests * update remaining tests --------- Co-authored-by: Mike <mikehathaway@makerdao.com> * Contract size mitigation (#1004) * moved debtInfo to PoolCommons, saving 10 bytes * moved withdrawBonds to KickerActions * documented a sample of invariant failures in regression tests * added unit test showing adding qt above auction price reverts * fixed _isCollateralized bug not returning true in all 0-debt use cases * updated nit spellings * fixed underflow calculating kicker reward * _repayDebtByThirdParty should check for expected pool errors * Round down when reward kicker, round up when kicker is penalized Fix roundings in tests * update test comments * fix and enable A9 invariant --------- Co-authored-by: mwc <matt@ajna.finance> Co-authored-by: Mike Hathaway <mahathaway93@gmail.com> Co-authored-by: Mike <mikehathaway@makerdao.com> Co-authored-by: Ian Harvey <iharvey@comcast.net> Co-authored-by: grandizzy <grandizzy.the.egg@gmail.com> Co-authored-by: grandizzy <38490174+grandizzy@users.noreply.github.com>
ajna-finance · Dec 5, 2023 · 196376b · 196376b
1 parent 5a270be
commit 196376b
Show file tree

Hide file tree

Showing 19 changed files with 166 additions and 81 deletions.
diff --git a/src/libraries/external/TakerActions.sol b/src/libraries/external/TakerActions.sol
@@ -775,10 +775,10 @@ library TakerActions {
 
  if (vars.isRewarded) {
  // take is below neutralPrice, Kicker is rewarded
- vars.bondChange = Maths.wmul(vars.quoteTokenAmount, uint256(vars.bpf));
+ vars.bondChange = Maths.floorWmul(vars.quoteTokenAmount, uint256(vars.bpf));
  } else {
  // take is above neutralPrice, Kicker is penalized
- vars.bondChange = Maths.wmul(vars.quoteTokenAmount, uint256(-vars.bpf));
+ vars.bondChange = Maths.ceilWmul(vars.quoteTokenAmount, uint256(-vars.bpf));
  }
 
  return vars;

diff --git a/src/libraries/helpers/PoolHelper.sol b/src/libraries/helpers/PoolHelper.sol
@@ -201,16 +201,16 @@ import { Maths } from '../internal/Maths.sol';
  uint256 price_,
  uint8 type_
  ) pure returns (bool) {
- // `False` if LUP = MIN_PRICE
- if (price_ == MIN_PRICE) return false;
+ // `False` if LUP = MIN_PRICE unless there is no debt
+ if (price_ == MIN_PRICE && debt_ != 0) return false;
 
  // Use collateral floor for NFT pools
  if (type_ == uint8(PoolType.ERC721)) {
  //slither-disable-next-line divide-before-multiply
  collateral_ = (collateral_ / Maths.WAD) * Maths.WAD; // use collateral floor
  }
 
- return Maths.wmul(collateral_, price_) >= Maths.wmul(COLLATERALIZATION_FACTOR, debt_);
+  return Maths.wmul(collateral_, price_) >= Maths.wmul(COLLATERALIZATION_FACTOR, debt_);
  }
 
  /**

diff --git a/tests/forge/interactions/ERC721TakeWithExternalLiquidity.sol b/tests/forge/interactions/ERC721TakeWithExternalLiquidity.sol
@@ -82,7 +82,7 @@ contract ERC721TakeWithExternalLiquidityTest is ERC721HelperContract {
  vm.expectEmit(true, true, false, true);
  uint256 quoteTokenPaid = 1_082.785034492073132320 * 1e18;
  uint256 collateralPurchased = 2 * 1e18;
- uint256 bondChange = 12.105904710718649454 * 1e18;
+ uint256 bondChange = 12.105904710718649453 * 1e18;
  emit Take(_borrower, quoteTokenPaid, collateralPurchased, bondChange, true);
  _pool.take(_borrower, 2, address(taker), data);
 
@@ -112,7 +112,7 @@ contract ERC721TakeWithExternalLiquidityTest is ERC721HelperContract {
  vm.expectEmit(true, true, false, true);
  uint256 quoteTokenPaid = 1_082.785034492073132320 * 1e18;
  uint256 collateralPurchased = 2 * 1e18;
- uint256 bondChange = 12.105904710718649454 * 1e18;
+ uint256 bondChange = 12.105904710718649453 * 1e18;
  emit Take(_borrower, quoteTokenPaid, collateralPurchased, bondChange, true);
  _pool.take(_borrower, 2, address(taker), data);
 

diff --git a/tests/forge/invariants/ERC20Pool/handlers/SettleERC20PoolHandler.sol b/tests/forge/invariants/ERC20Pool/handlers/SettleERC20PoolHandler.sol
@@ -244,7 +244,10 @@ contract SettleERC20PoolHandler is UnboundedLiquidationPoolHandler, UnboundedBas
  // ensure actor always has amount of quote to repay
  _ensureQuoteAmount(payer_, borrowerDebt + 10 * 1e18);
 
- _erc20Pool.repayDebt(borrower_, amountToRepay_, 0, borrower_, 7388);
+ try _erc20Pool.repayDebt(borrower_, amountToRepay_, 0, borrower_, 7388) {
+ } catch (bytes memory err) {
+ _ensurePoolError(err);
+ }
  }
 
  function _setupLendersAndDeposits(uint256 count_) internal virtual {

diff --git a/tests/forge/invariants/base/LiquidationInvariants.t.sol b/tests/forge/invariants/base/LiquidationInvariants.t.sol
@@ -21,7 +21,7 @@ abstract contract LiquidationInvariants is BasicInvariants {
  _invariant_A5();
  _invariant_A7();
  _invariant_A8();
- // _invariant_A9(); // TODO: uncomment once other invariant issues are sorted
+ _invariant_A9();
  }
 
  /// @dev checks sum of all borrower's t0debt is equals to total pool t0debtInAuction
@@ -144,9 +144,10 @@ abstract contract LiquidationInvariants is BasicInvariants {
 
  /// @dev reference prices in liquidation queue shall not decrease
  function _invariant_A9() internal view {
+ uint256 referencePrice;
  (,,,, uint256 lastReferencePrice,,, address nextBorrower,,) = _pool.auctionInfo(address(0));
  while (nextBorrower != address(0)) {
- (,,,, uint256 referencePrice,,,,,) = _pool.auctionInfo(nextBorrower);
+ (,,,, referencePrice,,,, nextBorrower,) = _pool.auctionInfo(nextBorrower);
  require(lastReferencePrice <= referencePrice, "Auction Invariant A9");
  lastReferencePrice = referencePrice;
  }

diff --git a/tests/forge/invariants/base/handlers/LiquidationPoolHandler.sol b/tests/forge/invariants/base/handlers/LiquidationPoolHandler.sol
@@ -62,7 +62,7 @@ abstract contract LiquidationPoolHandler is UnboundedLiquidationPoolHandler, Bas
  amount_ = auctionedCollateral / 2;
 
  (, , , uint256 kickTime, , , , , , ) = _pool.auctionInfo(borrower);
- // skip to make auction takeable
+ // TODO: eliminate this unnecessary skip, perhaps advance by single block instead
  if (block.timestamp - kickTime < 1 hours) {
  vm.warp(block.timestamp + 61 minutes);
  }

diff --git a/tests/forge/invariants/base/handlers/unbounded/UnboundedLiquidationPoolHandler.sol b/tests/forge/invariants/base/handlers/unbounded/UnboundedLiquidationPoolHandler.sol
@@ -225,7 +225,7 @@ abstract contract UnboundedLiquidationPoolHandler is BaseHandler {
  // when kicker and taker are same, kicker Reward = total Reward (lps) - taker Reward (Collateral Price * difference of bucket used and auction price)
  if (!depositTake_ && kicker == _actor) {
  uint256 totalReward = lpToQuoteToken(afterBucketTakeVars.kickerLps - beforeBucketTakeVars.kickerLps, bucketIndex_);
- uint256 takerReward = Maths.wmul(beforeBucketTakeVars.borrowerCollateral - afterBucketTakeVars.borrowerCollateral, _priceAt(bucketIndex_) - auctionPrice);
+ uint256 takerReward = Maths.floorWmul(beforeBucketTakeVars.borrowerCollateral - afterBucketTakeVars.borrowerCollateral, _priceAt(bucketIndex_) - auctionPrice);
 
  // **A8**: kicker reward <= Borrower penalty
  kickerReward = totalReward - takerReward;

diff --git a/tests/forge/regression/ERC20Pool/RegressionTestReservesERC20Pool.t.sol b/tests/forge/regression/ERC20Pool/RegressionTestReservesERC20Pool.t.sol
@@ -1103,6 +1103,38 @@ contract RegressionTestReserveERC20Pool is ReserveERC20PoolInvariants {
  invariant_reserves();
  }
 
+ /**
+ Test was failing because rewards differ by 1 gwei.
+ Fixed by updating TakerActions to round in favor of the protocol.
+ **/
+ function test_regression_equivalent_rewards_on_take_A8() external {
+ _reserveERC20PoolHandler.withdrawBonds(115792089237316195423570985008687907853269984665640564039457584007913129639935, 186, 115792089237316195423570985008687907853269984665640564039457584007913129639935);
+ _reserveERC20PoolHandler.kickAuction(702555080221925111442235101208458356417149500340156719083570911869585030, 1000032244817342892, 5728, 5907050982353226005633690);
+ _reserveERC20PoolHandler.moveQuoteToken(688008771392821209054006930063060492511262218297247835920927151049965569, 1000259178895617557, 7180441242553988447746122786, 216450160307007747500389357, 999970054977710845412932917383);
+ _reserveERC20PoolHandler.transferLps(115792089237316195423570985008687907853269984665640564039457584007913129639935, 18694763467974013375909729483405538071349537050941877688230901475624371810, 3063772036798766170756187646253602, 115792089237316195423570985008687907853269984665640564039457584007913129639933, 4461285713774321097323261260815305817048191133452574325);
+ _reserveERC20PoolHandler.removeQuoteToken(187125087688063596684224028224017465563990405145, 1, 3, 0);
+ _reserveERC20PoolHandler.takeAuction(126553, 115792089237316195423570985008687907853269984665640564039457584007913129639935, 6092025869035498002801041293341729609328640575790487069100672274, 111933338551295177700896412130593678998178179083162406077575);
+ invariant_auction();
+ }
+
+ /**
+ Test was failing because kicker reward went negative.
+ Fixed by updating calculation in UnboundedLiquidationPoolHandler._bucketTake.
+ **/
+ function test_regression_fenwick_overflow_on_bucketTake() external {
+ _reserveERC20PoolHandler.pledgeCollateral(6350058051515749101632494, 394728019280243247197391935, 1000226947060929945);
+ _reserveERC20PoolHandler.lenderKickAuction(551739522123447953718609926799, 29959904068493160226, 695277117643056667684065838815807991886030299764502895676394782692460545);
+ _reserveERC20PoolHandler.addQuoteToken(115792089237316195423570985008687907853269984665640564039457584007913129639934, 346390061016453699097263788381540543801879524975, 115792089237316195423570985008687907853269984665640564039457584007913129639935, 0);
+ _reserveERC20PoolHandler.stampLoan(115792089237316195423570985008687907853269984665640564039457584007913129639932, 115792089237316195423570985008687907853269984665640564039457584007913129639934);
+ _reserveERC20PoolHandler.kickAuction(11301737, 355771, 2, 2);
+ _reserveERC20PoolHandler.bucketTake(115792089237316195423570985008687907853269984665640564039457584007913129639935, 3, false, 115792089237316195423570985008687907853269984665640564039457584007913129639932, 1);
+ _reserveERC20PoolHandler.kickReserveAuction(3, 2231676212031384998427);
+ _reserveERC20PoolHandler.removeCollateral(2999999999999999999999999998988019665354522083, 1035322182001286518, 695165544206547503308377521130384690735039945359133611786714594690651433, 1004661270349627628);
+ _reserveERC20PoolHandler.bucketTake(447001332907764750034625708199, 1020546679384387976, true, 1012523217442524004724156, 18158702416679488818029017510144);
+ _reserveERC20PoolHandler.bucketTake(402642984652752865557170717473, 0, false, 2372112862459332523316520992118272, 15759742);
+ invariant_fenwick();
+ }
+
 }
 
 contract RegressionTestReserveWith10BucketsERC20Pool is ReserveERC20PoolInvariants { 

diff --git a/tests/forge/regression/ERC20Pool/RegressionTestSettleERC20Pool.t.sol b/tests/forge/regression/ERC20Pool/RegressionTestSettleERC20Pool.t.sol
@@ -0,0 +1,23 @@
+// SPDX-License-Identifier: UNLICENSED
+
+pragma solidity 0.8.18;
+
+import { SettleERC20PoolInvariants } from "../../invariants/ERC20Pool/SettleERC20PoolInvariants.t.sol";
+
+contract RegressionTestSettleERC20Pool is SettleERC20PoolInvariants { 
+
+ function setUp() public override {
+ // failures reproduced with default number of active buckets
+ vm.setEnv("NO_OF_BUCKETS", "3");
+ super.setUp();
+ }
+
+ /**
+ Test was failing because SettleERC20PoolHandler was not catching expected pool errors when repaying from a third party.
+ */
+ function test_regression_settle_then_repay() external {
+ _settleERC20PoolHandler.settleDebt(3113042312187095938847976769131078147978133970801631984161493412007580, 71508422573531484609164655, 55359934378837189558162829458006585270105);
+ _settleERC20PoolHandler.repayDebtByThirdParty(1333, 3439, 3116, 2819);
+ invariant_quote();
+ }
+}
diff --git a/tests/forge/regression/ERC721Pool/RegressionTestReservesERC721Pool.t.sol b/tests/forge/regression/ERC721Pool/RegressionTestReservesERC721Pool.t.sol
@@ -1842,4 +1842,20 @@ contract RegressionTestReserveEvmRevertERC721Pool is ReserveERC721PoolInvariants
 
  invariant_reserves();
  }
+
+ /*
+ Test failed because kick was not reverting with BorrowerOk when debt was 0 but price was MIN_PRICE.
+ Fixed by updating PoolHelper._isCollateralized to handle that use case.
+ */
+ function test_regression_kick_after_reserve_auction() external {
+ _reserveERC721PoolHandler.addQuoteToken(115792089237316195423570985008687907853269984665640564039457584007913129639935, 6935739002941423355049, 115792089237316195423570985008687907853269984665640564039457584007913129639935, 3);
+ _reserveERC721PoolHandler.bucketTake(1, 68298831663979631596289246956670551985825478107896188, true, 873193787008170413724397952426848542555828766842772550062607158488206182552, 1755961235259972539);
+ _reserveERC721PoolHandler.bucketTake(1059688629222756016, 1000000000000000000000027624872788925611714027, true, 1000000436829433309, 1726113828302394559154);
+ _reserveERC721PoolHandler.lenderKickAuction(798548716449031226788513776464931583931653468137708864548183282831, 115792089237316195423570985008687907853269984665640564039457584007913129639933, 336458097);
+ _reserveERC721PoolHandler.withdrawBonds(91351469402150005, 20135321806946154, 115792089237316195423570985008687907853269984665640564039457584007913129639933);
+ _reserveERC721PoolHandler.kickReserveAuction(2427901310042246836092666709297076039807059369179899651290575214128933, 115792089237316195423570985008687907853269984665640564039457584007913129639933);
+ _reserveERC721PoolHandler.kickAuction(1000000038825063504, 3815158141615494281861519, 33063346009737933132629913678857, 463918506144460971);
+
+ invariant_reserves();
+ }
 }
diff --git a/tests/forge/unit/ERC20Pool/ERC20PoolDebtExceedsDeposit.t.sol b/tests/forge/unit/ERC20Pool/ERC20PoolDebtExceedsDeposit.t.sol
@@ -377,10 +377,10 @@ contract ERC20PoolDebtExceedsDepositTest is ERC20HelperContract {
  index: 3231,
  collateralArbed: 10_400.000000000000000000 * 1e18,
  quoteTokenAmount: 524_670.145016980273417600 * 1e18,
- bondChange: 3_308.944954975446157085 * 1e18,
+ bondChange: 3_308.944954975446157084 * 1e18,
  isReward: true,
  lpAwardTaker: 523_983.006723242908178800 * 1e18,
- lpAwardKicker: 3_308.813860486169934000 * 1e18
+ lpAwardKicker: 3_308.813860486169933999 * 1e18
  });
 
  // borrower now has bad debt
@@ -394,7 +394,7 @@ contract ERC20PoolDebtExceedsDepositTest is ERC20HelperContract {
 
  _assertBucket({
  index: 3231,
- lpBalance: 1_527_246.158483272457112800 * 1e18,
+ lpBalance: 1_527_246.158483272457112799 * 1e18,
  collateral: 10_400.000000000000000000 * 1e18,
  deposit: 478_632.755812061670839884 * 1e18,
  exchangeRate: 1.000039619783645660 * 1e18
@@ -427,7 +427,7 @@ contract ERC20PoolDebtExceedsDepositTest is ERC20HelperContract {
  // 2c. There is no deposit remaing to withdraw
  _assertBucket({
  index: 3231,
- lpBalance: 1_527_246.158483272457112800 * 1e18,
+ lpBalance: 1_527_246.158483272457112799 * 1e18,
  collateral: 10_400.000000000000000000 * 1e18,
  deposit: 0,
  exchangeRate: 0.686643672998356028 * 1e18
@@ -446,7 +446,7 @@ contract ERC20PoolDebtExceedsDepositTest is ERC20HelperContract {
  from: _attacker,
  amount: 10_400.000000000000000000 * 1e18,
  index: 3231,
- lpRedeem: 1_527_246.158483272457112800 * 1e18
+ lpRedeem: 1_527_246.158483272457112799 * 1e18
  });
 
  _assertReserveAuction({

diff --git a/tests/forge/unit/ERC20Pool/ERC20PoolLiquidationsArbTake.t.sol b/tests/forge/unit/ERC20Pool/ERC20PoolLiquidationsArbTake.t.sol
@@ -281,10 +281,10 @@ contract ERC20PoolLiquidationsArbTakeTest is ERC20HelperContract {
  index: _i9_91,
  collateralArbed: 2 * 1e18,
  quoteTokenAmount: 17.598718399009752440 * 1e18,
- bondChange: 0.102293476350866899 * 1e18,
+ bondChange: 0.102293476350866898 * 1e18,
  isReward: true,
  lpAwardTaker: 2.224045908450701035 * 1e18,
- lpAwardKicker: 0.101762465718392482 * 1e18
+ lpAwardKicker: 0.101762465718392481 * 1e18
  });
 
  _assertLenderLpBalance({
@@ -296,15 +296,15 @@ contract ERC20PoolLiquidationsArbTakeTest is ERC20HelperContract {
  _assertLenderLpBalance({
  lender: _lender,
  index: _i9_91,
- lpBalance: 2_000.010438264805150482 * 1e18, // rewarded with LP in bucket
+ lpBalance: 2_000.010438264805150481 * 1e18, // rewarded with LP in bucket
  depositTime: _startTime + 100 days + 6.5 hours
  });
  _assertBucket({
  index: _i9_91,
- lpBalance: 2_002.234484173255851517 * 1e18,
+ lpBalance: 2_002.234484173255851516 * 1e18,
  collateral: 2 * 1e18,
  deposit: 1_992.848051181875920483 * 1e18,
- exchangeRate: 1.005218138423884932 * 1e18
+ exchangeRate: 1.005218138423884933 * 1e18
  });
  // reserves should remain the same after arb take
  _assertReserveAuction({

diff --git a/tests/forge/unit/ERC20Pool/ERC20PoolLiquidationsDepositTake.t.sol b/tests/forge/unit/ERC20Pool/ERC20PoolLiquidationsDepositTake.t.sol
@@ -482,7 +482,7 @@ contract ERC20PoolLiquidationsDepositTakeTest is ERC20HelperContract {
  index: _i1505_26,
  collateralArbed: 0.009964646438247887 * 1e18,
  quoteTokenAmount: 14.999420850513035210 * 1e18,
- bondChange: 0.167698623224374283 * 1e18,
+ bondChange: 0.167698623224374284 * 1e18,
  isReward: false,
  lpAwardTaker: 0,
  lpAwardKicker: 0
@@ -493,11 +493,11 @@ contract ERC20PoolLiquidationsDepositTakeTest is ERC20HelperContract {
  borrower: _borrower,
  active: true,
  kicker: _lender,
- bondSize: 0.047128646698885501 * 1e18,
+ bondSize: 0.047128646698885500 * 1e18,
  bondFactor: 0.011180339887498948 * 1e18,
  kickTime: block.timestamp - 5 hours,
  referencePrice: 10.681503928998397483 * 1e18,
- totalBondEscrowed: 0.047128646698885501 * 1e18,
+ totalBondEscrowed: 0.047128646698885500 * 1e18,
  auctionPrice: 15.105927722931035016 * 1e18,
  debtInAuction: 4.467355778582383914 * 1e18,
  thresholdPrice: 9.607367579382098527 * 1e18,

diff --git a/tests/forge/unit/ERC20Pool/ERC20PoolLiquidationsMisc.t.sol b/tests/forge/unit/ERC20Pool/ERC20PoolLiquidationsMisc.t.sol
@@ -494,7 +494,7 @@ contract ERC20PoolLiquidationsMiscTest is ERC20HelperContract {
  from: _lender,
  borrower: _borrower2,
  maxCollateral: 1_000.0 * 1e18,
- bondChange: 25.172604731198478139 * 1e18,
+ bondChange: 25.172604731198478138 * 1e18,
  givenAmount: 2_251.506213987704828000 * 1e18,
  collateralTaken: 1_000 * 1e18,
  isReward: true

diff --git a/tests/forge/unit/ERC20Pool/ERC20PoolLiquidationsSettle.t.sol b/tests/forge/unit/ERC20Pool/ERC20PoolLiquidationsSettle.t.sol
@@ -714,7 +714,7 @@ contract ERC20PoolLiquidationsSettleTest is ERC20HelperContract {
  from: _lender,
  borrower: _borrower2,
  maxCollateral: 1_000 * 1e18,
- bondChange: 12.304107698704044033 * 1e18,
+ bondChange: 12.304107698704044032 * 1e18,
  givenAmount: 1_100.512848671229788000 * 1e18,
  collateralTaken: 1_000 * 1e18,
  isReward: true