Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge develop to master #1047

Merged
merged 77 commits into from
Jan 4, 2024
Merged

Merge develop to master #1047

merged 77 commits into from
Jan 4, 2024

Conversation

EdNoepel
Copy link
Contributor

@EdNoepel EdNoepel commented Dec 20, 2023

Description

Merge rc9 release to master.

Audit Fixes and Bug Resolutions

  • Dmitri 1 and 2, Sherlock 018-H (Reserves): Issue with reserves being pumped by borrower fees exceeding kicker fees, potentially drained by liquidating a barely-collateralized loan and manipulating LUP.
  • Certora Crit-01 (Reserves): Potential for an attacker to manipulate liquidity, drawing debt far above fair market value and impacting lender interests and reserve fees.
  • Kirill M-05 (Pools): Manipulation of book with collateral to avoid unutilized deposit fee, leading to universal deposit fee implementation.
  • Sherlock 006-M (Pools): First pool borrower facing extra interest, necessitating adjustments in interest calculations.
  • Sherlock 007-M (Pools): Unsafe truncation casting used for state variables, highlighting need for safe casting methods.
  • Sherlock 001-M (Pools): Incorrect use of auctionPrice in BPF calculations affecting bond rewards and penalties.
  • Dmitri 9, Sherlock 001-M (Pools): Borrowers exploiting high price takes to avoid penalties and impact kicker liquidation bonds.
  • Kirill M-02 (Pools): Difficulty in kicking loans with TP below MIN_PRICE, leading to proposal for new loan management criteria.
  • Sherlock 016-H (Pools): Artificial amplification of kicker rewards through batched takes, requiring structural adjustments.
  • Sherlock 005-M (Pools): Risks of HPB bankruptcy due to unscaled values in debt forgiveness processes.
  • Kirill M-07 (Pools): Debate over the ability to kick a CRA with unsettled liquidations, contingent on reserve mechanism decisions.
  • Kirill L-08 (Pools): Reserve auction kick conditions not aligning with intended documentation or functionality.
  • Sherlock 009-M (Pools): lenderKick function incorrectly setting LUP, affecting interest calculations against hypothetical values.
  • Prototech 55 (Pools): Concerns about interest accrual potentially overflowing and impacting pool stability.
  • Kirill L-01 (Pools): Inconsistencies in dust checks for quote tokens, raising questions about user responsibility in token management.
  • Kirill L-05 (Pools): Presence of redundant or unused variables and parameters, prompting a need for code cleanup.

Improvements

  • Pools: Cleaned up event logging for take in ERC721 pools
  • Pools: decreased time inbetween auctions
  • Pools: Removed revertBelowLup argument for moveLiquidity, addQuoteToken
  • Pools: addQuoteToken cannot be called on a bucket whose price exceeds that of the oldest auction
  • Pools: DepositFee now applies to all buckets and charges 8 hours of interest
  • Pools: thresholdPrice is now calculated with 1.04 collateralization factor
  • Pools: altered auction price function to match WP
  • Rewards: RewardsManager.sol has been removed
  • Invariant: Decoupled RewardsManager invariants from PositionManager as RewardsManager was removed

Purpose

Deploy rc9 contracts.

Impact

Refer to merged RC9 PRs in commit history.

Tasks

  • Changes to protocol contracts are covered by unit tests executed by CI.
  • Protocol contract size limits have not been exceeded.
  • Gas consumption for impacted transactions have been compared with the target branch, and nontrivial changes cited in the Impact section above.
  • Scope labels have been assigned as appropriate.
  • Invariant tests have been manually executed as appropriate for the nature of the change.

ith-harvey and others added 30 commits July 10, 2023 08:01
* added logging for positionManager

* added rewards and position logging

* clean up

* updated rewards mapping back to public so tests pass

* how modifier was being called in rewardsPoolHandler

* revised so logging pools is not required when logging positions

* cleanup

* readme cleanup

---------

Co-authored-by: Ian Harvey <iharvey@comcast.net>
…ards manager invariant testing (#927)

* Add multiple pools in position and rewards manager invariant testing

* Fix RW6 regression test

* Fix rewardsClaimed and updateRewardsClaimed in Rewards manager

* Fix compile error

* PR feedback

* Add configurable number of pools for position and rewards manager invariant testing
* added randomness

* added the ability to transfer positions

* increased chance of rewards being claimed in handlers

* cleanup

* responded to comments

---------

Co-authored-by: Ian Harvey <iharvey@comcast.net>
* Add fuzz test for borrower borrows fuzzed amount and getting kick after some time

* Add fuzz test for take fuzzed amount of collateral from auction

* Add fuzz test for settle with fuzzed pool deposit

* Add fuzz test for add and remove collateral in ERC721Pool

* Fuzzed buckets used in borrow and kick fuzz test

* PR feedback
…931)

* Update position and rewards manager invariant logging for multiple pools

* Fix regression test to run for any token precision and Quote token limits

* PR feedback
…osition (#928)

* Update Position invariants handler to memorialize and redeem multiple positions

* PR feedback

* Add partial random positions redeem in redeem position handler

* Add random time skips between epochs in rewards manager
* Add bucket bankruptcy scenario for rewards manager

* Fix evm reverts

* PR feedback

* Update prepare test methods to add position in NFT if there is no position in it
* stopped updatedExchangeRates emit when the rates are not updated

* removed epoch check in _updateBucketExchangeRates

---------

Co-authored-by: Ian Harvey <iharvey@comcast.net>
* Add settle invariant scenario

* Reduce Loans and skip time to make undercollateralize
* Add unit test to check exp function limit

* Add unit tests to check pool deposit and debt limits

* PR cleanup
* Merge Develop into Master (#934)

* Logging for RewardsManager and PositionManager invariants (#925)

* added logging for positionManager

* added rewards and position logging

* clean up

* updated rewards mapping back to public so tests pass

* how modifier was being called in rewardsPoolHandler

* revised so logging pools is not required when logging positions

* cleanup

* readme cleanup

---------

Co-authored-by: Ian Harvey <iharvey@comcast.net>

* Invariants Improvement: Add multiple pool support in position and rewards manager invariant testing (#927)

* Add multiple pools in position and rewards manager invariant testing

* Fix RW6 regression test

* Fix rewardsClaimed and updateRewardsClaimed in Rewards manager

* Fix compile error

* PR feedback

* Add configurable number of pools for position and rewards manager invariant testing

* Positions Invariants: Multiple positions, transfer positions (#926)

* added randomness

* added the ability to transfer positions

* increased chance of rewards being claimed in handlers

* cleanup

* responded to comments

---------

Co-authored-by: Ian Harvey <iharvey@comcast.net>

* Fuzz test additions (#924)

* Add fuzz test for borrower borrows fuzzed amount and getting kick after some time

* Add fuzz test for take fuzzed amount of collateral from auction

* Add fuzz test for settle with fuzzed pool deposit

* Add fuzz test for add and remove collateral in ERC721Pool

* Fuzzed buckets used in borrow and kick fuzz test

* PR feedback

* Invariant Improvements: Position rewards logging for multiple pools (#931)

* Update position and rewards manager invariant logging for multiple pools

* Fix regression test to run for any token precision and Quote token limits

* PR feedback

* Invariants Improvements: Add Multiple position in single handler in Position (#928)

* Update Position invariants handler to memorialize and redeem multiple positions

* PR feedback

* Add partial random positions redeem in redeem position handler

* Add random time skips between epochs in rewards manager

* Add bucket bankruptcy scenario for rewards manager (#930)

* Add bucket bankruptcy scenario for rewards manager

* Fix evm reverts

* PR feedback

* Update prepare test methods to add position in NFT if there is no position in it

---------

Co-authored-by: Ian Harvey <iharvey@comcast.net>
Co-authored-by: Prateek Gupta <prateek105@users.noreply.github.com>

* manually merged from private-contracts/immutable-kicks

* eliminated alreadyTaken (drawio needs updating)

* annotated failing unit tests

* new limit to induce testAccruePoolInterestRevertDueToExpLimit revert

* fixed testUpdateInterestTuLimit

* fixed testAccrueInterestNewInterestLimit

* removed alreadyTaken from drawios

* updated drawio-generated html

* merged Prateek's post-merge changes

---------

Co-authored-by: grandizzy <38490174+grandizzy@users.noreply.github.com>
Co-authored-by: Ian Harvey <ith.harvey@gmail.com>
Co-authored-by: Ian Harvey <iharvey@comcast.net>
Co-authored-by: Prateek Gupta <prateek105@users.noreply.github.com>
* updated license

* adjustment for public chains
* Add PoolInfoUtilsMulticall contract to call multiple PoolInfoUtils methods in a single call to reduce rpc calls from subgraph

* Add tests for PoolInfoutilsMulticall

* Added comments and code improvements

* altered license

---------

Co-authored-by: Ian Harvey <iharvey@comcast.net>
Co-authored-by: Ian Harvey <iharvey@comcast.net>
* this underflows instead of giving expected revert

* move isCollateralized check after updating borrower collateral

* remove local calculation of encumbered collateral

* trying to properly fix testBorrowRepayPrecision

* resolve rounding issue in fuzz test

* testCollateralization improvements

* updated unit tests for new _collateralization implementation

* more collateralization tests
* changed PR template

* commented out example test, as requested in PR review
* added a fenwick OOB test

* unit test to prove 0 loan heap insertions don't break anythnig

* test harness for the auction queue and simple unit tests

* comment to resolve Kirill L-07

* removed redundant code per Kirill L-05

* added SafeCasts
* Return debt and collateral settled from settle method

* Add isBorrowerSettled return in settle method
* Add try catch for interest accrual and update interest method to avoid pool locking

* PR feedback
EdNoepel and others added 10 commits December 17, 2023 12:55
* implement reserve auction pricing as originally described in whitepaper

* bug fixes

* wip updating RewardsManager tests

* disable rewards unit tests

* handle 0 bids on reserve auctions

* updated erc721 reserve auction unit tests

* fixed issue bidding on more than the quote token trading increment

* updated new unit test

* added tearDown to testZeroBid

* removed rayToWad

---------

Co-authored-by: Ian Harvey <iharvey@comcast.net>
* test_regression_exchange_rate_failure

* Modify computation of quotetoken amount in TakerActions.sol
to compute bond reward accurately for collateral constrained
takes with collateral tokens with decimals != 18.

* Fix tests (#1036)

* updated baseline

---------

Co-authored-by: Ed Noepel <ed@noepel.net>
Co-authored-by: mwc <matt@ajna.finance>
Co-authored-by: grandizzy <38490174+grandizzy@users.noreply.github.com>
* Fix test_regression_failure_A8_5

* Remove repayDebtByThirdParty handler in SettleERC20PoolHandler, as repayment for borrower in auction is restricted

* Add failing regression test_regression_bucket_take_arithmetic_over_underflow

* Fix test_regression_bucket_take_arithmetic_over_underflow

* Invariant RE9 improvement

* Add failing regression test_regression_bucket_take_reserves_failure

* Fix test_regression_bucket_take_reserves_failure

* Add failing regression test_regression_bucket_take_re9_failure

* Move failing regression tests to RegressionTestReservesWith8QuotePrecision12CollateralPrecisionERC20Pool

* Fix test_regression_bucket_take_re9_failure

* Fix regression tests failing due to reserve error margin calculation when auction price is 0

* Universal deposit fee (#983)

* charge fee on all deposit

* unit tests compile

* test harness updates

* working on ERC20PoolQuoteTokenTest

* removed deposit fee cap

* more work on ERC20PoolQuoteTokenTest

* do not charge deposit fee if moving liquidity to higher price

* Eliminate reserves bad debt allocation and add margin to TP  (#962)

* this underflows instead of giving expected revert

* move isCollateralized check after updating borrower collateral

* remove local calculation of encumbered collateral

* trying to properly fix testBorrowRepayPrecision

* resolve rounding issue in fuzz test

* testCollateralization improvements

* updated unit tests for new _collateralization implementation

* more collateralization tests

* Add 1.04 factor in borrower collateralization

* Update nptp ratio to '1 + sqrt(r)/2'

* Remove Settle debt with pool reserves

* Remove 0.995 factor from claimable reserves calculation

* Update bond factor calculation to minimum 0.005

* added testcase where debt exceeds deposit

* updated test so debt exceeds deposit

* allow  up to half of current orig fee to be used to settle bad debt

* updated testTakeAndSettle

* more test fixes

* Enabled settling with all reserves if
 Deposits.treeSum==0 or 72 hrs pass

* cleanup

* Half orig fee res | Matt example (#966)

* added Matts test as proof that attack no longer works on his branch

* Revert "Remove multicall from position manager (#948)" (#961)

This reverts commit f540c8a.

* added test testSpendOrigFeePushBadDebtToBorrowers test

* cleaned up testStealReservesWithMarginm to match minted balances

* responded to Matts comments

---------

Co-authored-by: Ian Harvey <iharvey@comcast.net>
Co-authored-by: Mike Hathaway <mahathaway93@gmail.com>

* Revert "Remove Settle debt with pool reserves"

This reverts commit 290d6cf.

* Update half origination fees reserves settlement time to 144 hours from kickTime

* Fix alignment and extra spaces

* Fix some unit tests

* PR feedback

* Update encumberance and collateralization method in poolInfoUtils

* Fix some unit tests

---------

Co-authored-by: Ed Noepel <ed@noepel.net>
Co-authored-by: Ian Harvey <iharvey@comcast.net>
Co-authored-by: mwc <matt@ajna.finance>
Co-authored-by: Ian Harvey <ith.harvey@gmail.com>
Co-authored-by: Mike Hathaway <mahathaway93@gmail.com>

* Fix invariant setup

* Fix some unit tests

* ERC20PoolQuoteTokenTest updated

* updated ERC20PoolPurchaseQuoteTokenTest

* Fixed tests in ERC20PoolReserveAuction.t.sol

* updated ERC20PoolBorrowTest and ERC20PoolBorrowFuzzyTest

* Fix ERC20PoolCollateral and ERC20PoolInfoUtils tests

* Fixed ERC20 arbtake and depositTake tests

* Fix ERC20PoolLiquidationsKickTest, ERC20PoolLiquidationKickFuzzyTest and ERC20PoolLiquidationsLenderKickAuctionTest

* updated ERC20PoolMulticallTest

* cleaned up ERC20PoolDebtExceedsDepositTest

* fixed testTakeLoanColConstraintBpfPosNoResidual

* fixed testTakeCallerColConstraintBpfPosNoResidual

* fixed testTakeCallerColConstraintBpfPosResidual

* fixed testTakeCallerColConstraintBpfNegResidual

* fixed testTakeLoanDebtConstraintBpfPosResidual

* fixed testTakeAndSettle

* cleaned up ERC20PoolDebtExceedsDepositTest

* updated ERC20PoolPrecisionTest

* Update ERC20PoolLiquidationsSettleTest

* Update ERC20PoolLiquidationsMisc

* Update ERC20PoolLiquidationSettleFuzzyTest

* Update ERC20PoolLiquidationTakeFuzzyTest

* fixeed revert tests

* ERC20PoolLiquidationsTake -- fixed rest

* Mh update tests (#985)

* fix most position manager tests

* fix additional pm tests

* fix rewards requiredCollateralRewards setup

* fix ClaimRewards tests

* update additional rewards manager tests

* fix additional tests

* more test fixes

* commit wip bankruptcy tests

* fixed testMoveLiquidityToOverwriteBankruptBucket

* fix additional tests

* fix testMoveLiquidityWithDebtInPool

* fix remaining rewards manager tests

---------

Co-authored-by: Mike <mikehathaway@makerdao.com>
Co-authored-by: mwc <matt@ajna.finance>

* Add 1.04 factor in HTP calculations (#987)

* Add 1.04 factor in HTP calculations

* Add COLLATERALIZATION_FACTOR constant in PoolHelpers

* Add collateralization factor in dwatp

* Fix poolPricesInfo

* Update ERC20PoolBorrowFuzzyTest

* Fix some unit tests

* Fix some unit tests

* Fix some unit tests

* Update ERC20PoolTransferLPs

* fix most rewards manager tests

* update remaining rewards manager tests

* update ERC721SubsetPoolBorrowTest and commit wip changes to testMergeOrRemoveERC721Collateral

* updated testSettlePartialDebtSubsetPool (#988)

* updated testSettlePartialDebtSubsetPool

* re-added teardown

---------

Co-authored-by: Ian Harvey <iharvey@comcast.net>

* fix ERC721PoolCollateral tests

* fix borrowRepayDebtFuzzy and additional PM tests

* cleaned up testBorrowAndRepayWith4DecimalQuote

---------

Co-authored-by: Mike <mikehathaway@makerdao.com>
Co-authored-by: Ian Harvey <ith.harvey@gmail.com>
Co-authored-by: Ian Harvey <iharvey@comcast.net>

* Fix some unit tests

* Deployment updates for RC8+ releases (#986)

* cherry-picked from master

* updated README

* Fix ERC721PoolLiquidationsTakeTest

* updated ERC721PoolReserveAuctionTest

* updated testMergeOrRemoveERC721Collateral (#989)

Co-authored-by: Ian Harvey <iharvey@comcast.net>

* updated ERC721PoolPurchaseQuoteTest and fixed bug in ERC721 tearDown

* cleaned up testLiquidationLenderKickAuction, testLiquidationSingleBorrower, testSettleAuctionWithoutTakes

* updated testMoveLiquidityToOverwriteBankruptBucket

* updated PoolHelperTest

* cleaned up asserts in addLiquidity

* update testMoveLiquidityInBankruptBucket_LP_report_179_494

* updated ERC721PoolEMAsTest

* fixed testKickHighThresholdPriceBorrower

* updated testBorrowerInterestCalculationAfterRepayingAllDebtOnce and testBorrowerInterestCalculation

* updated testMultipleBorrowerInterestAccumulation

* Fix regression tests

* updated RE3, fixed _addQuoteToken

* addQuoteToken - return amount added (#993)

* addQuoteToken now returns amount added

* update unit tests to validate return values

* Fix up add liquidity (#992)

* cleaned up addLiquidity() method in ERC20PoolLiquidationsScaled.t.sol

* remove console

---------

Co-authored-by: Ian Harvey <iharvey@comcast.net>

* fixed issue with test_regression_fenwick_index_2 in non-18-decimal env

* added regression tests for two failing reserves scenarios

* rename tests such that automake runs them with other regression tests

* UnboundedLiquidationPoolHandler bucketTake fix (#994)

* updated UnboundedLiquidationPoolHandler to handle compensated collateral in bucketTake

* cleanup, trap unhandled use case

* settle event now emits actual debt settled rather than t0 amount (#999)

* settle event now emits actual debt settled rather than t0 amount

* updated test_regression_settle_with_reserves

* Updated auctionInfo (#996)

* add thresholdPrice to auctionInfo; update test iterface usages

* temp fix tests

* remove unneeded comment

* Update auction status (#998)

* wip auction status update w/ stack too deep

* update auctionStatus; add auctionInfo method to poolInfoUtils

---------

Co-authored-by: Mike <mikehathaway@makerdao.com>

---------

Co-authored-by: Mike <mikehathaway@makerdao.com>

* updated brownie tests, removed obsolete invariant tests (#1001)

* Block addqt above auction price (#997)

* 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

* added unit test showing adding qt above auction price reverts

* updated nit spellings

---------

Co-authored-by: mwc <matt@ajna.finance>
Co-authored-by: Ed Noepel <46749157+EdNoepel@users.noreply.github.com>
Co-authored-by: Ed Noepel <ed@noepel.net>
Co-authored-by: Mike Hathaway <mahathaway93@gmail.com>
Co-authored-by: Mike <mikehathaway@makerdao.com>
Co-authored-by: Ian Harvey <iharvey@comcast.net>

* 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>

* RC9 suggested improvements (#1005)

* Revert AuctionNotTakeable in same place, load auction kickTime only once from storage

* Calculate ERC721 collateralTaken only once
cosmetize code (read from result struct in local var and reuse)
Results in shrinking a little bit contract sizes

* Read borrower Np Tp ratio from storage only once when kick

* If block style, proper indentation

* Proposed changes to PR #972:
- avoid calculating current LUP twice in lender kick
- change _kick function to accept proposed LUP (for regular kick proposed LUP is current LUP, for lender kick proposed LUP is calculated based on additional debt)
- in both kick cases return current LUP in kick result
- reduce gas costs by saving a Fenwick traversal
- reduce contract size by removing LUP calculation within Pool

* Cosmetic flashloan code changes, PoolCommons.flashLoan doesn't return false but always reverts if flashloan fails

* PR#983 style, remove redundant line

* PR#999 suggested improvement - calculate current settled debt only once and include in settle result for state update

* PR #962 proposed changes:
- get Fenwick deposits only once when settle with reserves
- define constants for min bond factor and max npTp ratio values

* PR #987 proposed improvement:
- add and reuse _htp helper function (instead duplicated maths)

* Continuation of PR #962: (#1008)

- Record settle amount limmit in Liquidtion struct, at the time of kick (that is instead incrementing accumulator in Borrower struct, each time debt is drawn)
- This way accumulator will be reseted when auction is settled
- fix tests

TODO:
- assert Liquidation.t0ReserveSettleAmount in unit tests (_assertAuction), make sure is set to 0 after auction settled and decreased when partial settles done
- update invariant test to check new introduced accumulator

* Misc test fixes (#1009)

* handle another reward rounding error use case

* fix intermittant fuzz test failure - cannot draw debt from liquidity in bucket 7388

* PositionManager should expect AddAboveAuctionPrice, which can happen in moveLiquidity

* Invariant fix: round up quote tokens calculated from rewarded LP (because LP rewarded are calculated in bucketTake as rewarded quote tokens -> LP rounded down)

* Take 1 - invariant code cleanup

* Take 2: reserves and other helpers

* Merge branch 'merge-rc9' into failing-regressions

* Fix pool address in tests

* Take 3 - more changes

* Pr feedback

* Fix failing unit tests

* Fix failing unit tests

* Take 4

* Fix test_regression_rw_reserves_failure_4

* buckettake reserves failure

* Fix test_regression_failure_reserves_on_bucketTake, update Invariant RE9

* Fix _takeAuction

* Fix unit tests

* PR feedback

* Fix bucketIndex in _bucketTake handler

* Add getLoansInfo helper, remove duplicated settle code, more style

* Additional code style and helper use

* Add back Positions tests

* Update TP naming in invariants

---------

Co-authored-by: Ed Noepel <46749157+EdNoepel@users.noreply.github.com>
Co-authored-by: Ed Noepel <ed@noepel.net>
Co-authored-by: Ian Harvey <iharvey@comcast.net>
Co-authored-by: mwc <matt@ajna.finance>
Co-authored-by: Ian Harvey <ith.harvey@gmail.com>
Co-authored-by: Mike Hathaway <mahathaway93@gmail.com>
Co-authored-by: Mike <mikehathaway@makerdao.com>
Co-authored-by: mattcushman <36414299+mattcushman@users.noreply.github.com>
Co-authored-by: grandizzy <grandizzy.the.egg@gmail.com>
Co-authored-by: grandizzy <38490174+grandizzy@users.noreply.github.com>
* shortened CRA interval to 5 days

* update comment

* resolve build warning

* update interval to a total of 120 hours

* Update src/libraries/external/KickerActions.sol

Co-authored-by: Kirill Fedoseev <kirill@blockscout.com>

---------

Co-authored-by: Kirill Fedoseev <kirill@blockscout.com>
* deposits invariant failure on kick for both pool types

* 18-decimal deposit on kick failure

* Fix HTP calculation in _fenwickAccrueInterest

* Update test comments

---------

Co-authored-by: Ed Noepel <ed@noepel.net>
* add external getters for borrower and bucketTokenIds

* update erc721 tests to use new methods

* update comment pr feedback

---------

Co-authored-by: Mike <mikehathaway@makerdao.com>
* Universal deposit fee (#983)

* charge fee on all deposit

* unit tests compile

* test harness updates

* working on ERC20PoolQuoteTokenTest

* removed deposit fee cap

* more work on ERC20PoolQuoteTokenTest

* do not charge deposit fee if moving liquidity to higher price

* Eliminate reserves bad debt allocation and add margin to TP  (#962)

* this underflows instead of giving expected revert

* move isCollateralized check after updating borrower collateral

* remove local calculation of encumbered collateral

* trying to properly fix testBorrowRepayPrecision

* resolve rounding issue in fuzz test

* testCollateralization improvements

* updated unit tests for new _collateralization implementation

* more collateralization tests

* Add 1.04 factor in borrower collateralization

* Update nptp ratio to '1 + sqrt(r)/2'

* Remove Settle debt with pool reserves

* Remove 0.995 factor from claimable reserves calculation

* Update bond factor calculation to minimum 0.005

* added testcase where debt exceeds deposit

* updated test so debt exceeds deposit

* allow  up to half of current orig fee to be used to settle bad debt

* updated testTakeAndSettle

* more test fixes

* Enabled settling with all reserves if
 Deposits.treeSum==0 or 72 hrs pass

* cleanup

* Half orig fee res | Matt example (#966)

* added Matts test as proof that attack no longer works on his branch

* Revert "Remove multicall from position manager (#948)" (#961)

This reverts commit f540c8a.

* added test testSpendOrigFeePushBadDebtToBorrowers test

* cleaned up testStealReservesWithMarginm to match minted balances

* responded to Matts comments

---------

Co-authored-by: Ian Harvey <iharvey@comcast.net>
Co-authored-by: Mike Hathaway <mahathaway93@gmail.com>

* Revert "Remove Settle debt with pool reserves"

This reverts commit 290d6cf.

* Update half origination fees reserves settlement time to 144 hours from kickTime

* Fix alignment and extra spaces

* Fix some unit tests

* PR feedback

* Update encumberance and collateralization method in poolInfoUtils

* Fix some unit tests

---------

Co-authored-by: Ed Noepel <ed@noepel.net>
Co-authored-by: Ian Harvey <iharvey@comcast.net>
Co-authored-by: mwc <matt@ajna.finance>
Co-authored-by: Ian Harvey <ith.harvey@gmail.com>
Co-authored-by: Mike Hathaway <mahathaway93@gmail.com>

* Fix invariant setup

* Fix some unit tests

* ERC20PoolQuoteTokenTest updated

* updated ERC20PoolPurchaseQuoteTokenTest

* Fixed tests in ERC20PoolReserveAuction.t.sol

* updated ERC20PoolBorrowTest and ERC20PoolBorrowFuzzyTest

* Fix ERC20PoolCollateral and ERC20PoolInfoUtils tests

* Fixed ERC20 arbtake and depositTake tests

* Fix ERC20PoolLiquidationsKickTest, ERC20PoolLiquidationKickFuzzyTest and ERC20PoolLiquidationsLenderKickAuctionTest

* updated ERC20PoolMulticallTest

* cleaned up ERC20PoolDebtExceedsDepositTest

* fixed testTakeLoanColConstraintBpfPosNoResidual

* fixed testTakeCallerColConstraintBpfPosNoResidual

* fixed testTakeCallerColConstraintBpfPosResidual

* fixed testTakeCallerColConstraintBpfNegResidual

* fixed testTakeLoanDebtConstraintBpfPosResidual

* fixed testTakeAndSettle

* cleaned up ERC20PoolDebtExceedsDepositTest

* updated ERC20PoolPrecisionTest

* Update ERC20PoolLiquidationsSettleTest

* Update ERC20PoolLiquidationsMisc

* Update ERC20PoolLiquidationSettleFuzzyTest

* Update ERC20PoolLiquidationTakeFuzzyTest

* fixeed revert tests

* ERC20PoolLiquidationsTake -- fixed rest

* Mh update tests (#985)

* fix most position manager tests

* fix additional pm tests

* fix rewards requiredCollateralRewards setup

* fix ClaimRewards tests

* update additional rewards manager tests

* fix additional tests

* more test fixes

* commit wip bankruptcy tests

* fixed testMoveLiquidityToOverwriteBankruptBucket

* fix additional tests

* fix testMoveLiquidityWithDebtInPool

* fix remaining rewards manager tests

---------

Co-authored-by: Mike <mikehathaway@makerdao.com>
Co-authored-by: mwc <matt@ajna.finance>

* Add 1.04 factor in HTP calculations (#987)

* Add 1.04 factor in HTP calculations

* Add COLLATERALIZATION_FACTOR constant in PoolHelpers

* Add collateralization factor in dwatp

* Fix poolPricesInfo

* Update ERC20PoolBorrowFuzzyTest

* Fix some unit tests

* Fix some unit tests

* Fix some unit tests

* Update ERC20PoolTransferLPs

* fix most rewards manager tests

* update remaining rewards manager tests

* update ERC721SubsetPoolBorrowTest and commit wip changes to testMergeOrRemoveERC721Collateral

* updated testSettlePartialDebtSubsetPool (#988)

* updated testSettlePartialDebtSubsetPool

* re-added teardown

---------

Co-authored-by: Ian Harvey <iharvey@comcast.net>

* fix ERC721PoolCollateral tests

* fix borrowRepayDebtFuzzy and additional PM tests

* cleaned up testBorrowAndRepayWith4DecimalQuote

---------

Co-authored-by: Mike <mikehathaway@makerdao.com>
Co-authored-by: Ian Harvey <ith.harvey@gmail.com>
Co-authored-by: Ian Harvey <iharvey@comcast.net>

* Fix some unit tests

* Deployment updates for RC8+ releases (#986)

* cherry-picked from master

* updated README

* Fix ERC721PoolLiquidationsTakeTest

* updated ERC721PoolReserveAuctionTest

* updated testMergeOrRemoveERC721Collateral (#989)

Co-authored-by: Ian Harvey <iharvey@comcast.net>

* updated ERC721PoolPurchaseQuoteTest and fixed bug in ERC721 tearDown

* cleaned up testLiquidationLenderKickAuction, testLiquidationSingleBorrower, testSettleAuctionWithoutTakes

* updated testMoveLiquidityToOverwriteBankruptBucket

* updated PoolHelperTest

* cleaned up asserts in addLiquidity

* update testMoveLiquidityInBankruptBucket_LP_report_179_494

* updated ERC721PoolEMAsTest

* fixed testKickHighThresholdPriceBorrower

* updated testBorrowerInterestCalculationAfterRepayingAllDebtOnce and testBorrowerInterestCalculation

* updated testMultipleBorrowerInterestAccumulation

* Fix regression tests

* updated RE3, fixed _addQuoteToken

* addQuoteToken - return amount added (#993)

* addQuoteToken now returns amount added

* update unit tests to validate return values

* Fix up add liquidity (#992)

* cleaned up addLiquidity() method in ERC20PoolLiquidationsScaled.t.sol

* remove console

---------

Co-authored-by: Ian Harvey <iharvey@comcast.net>

* fixed issue with test_regression_fenwick_index_2 in non-18-decimal env

* added regression tests for two failing reserves scenarios

* rename tests such that automake runs them with other regression tests

* UnboundedLiquidationPoolHandler bucketTake fix (#994)

* updated UnboundedLiquidationPoolHandler to handle compensated collateral in bucketTake

* cleanup, trap unhandled use case

* settle event now emits actual debt settled rather than t0 amount (#999)

* settle event now emits actual debt settled rather than t0 amount

* updated test_regression_settle_with_reserves

* Updated auctionInfo (#996)

* add thresholdPrice to auctionInfo; update test iterface usages

* temp fix tests

* remove unneeded comment

* Update auction status (#998)

* wip auction status update w/ stack too deep

* update auctionStatus; add auctionInfo method to poolInfoUtils

---------

Co-authored-by: Mike <mikehathaway@makerdao.com>

---------

Co-authored-by: Mike <mikehathaway@makerdao.com>

* updated brownie tests, removed obsolete invariant tests (#1001)

* Block addqt above auction price (#997)

* 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

* added unit test showing adding qt above auction price reverts

* updated nit spellings

---------

Co-authored-by: mwc <matt@ajna.finance>
Co-authored-by: Ed Noepel <46749157+EdNoepel@users.noreply.github.com>
Co-authored-by: Ed Noepel <ed@noepel.net>
Co-authored-by: Mike Hathaway <mahathaway93@gmail.com>
Co-authored-by: Mike <mikehathaway@makerdao.com>
Co-authored-by: Ian Harvey <iharvey@comcast.net>

* 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>

* RC9 suggested improvements (#1005)

* Revert AuctionNotTakeable in same place, load auction kickTime only once from storage

* Calculate ERC721 collateralTaken only once
cosmetize code (read from result struct in local var and reuse)
Results in shrinking a little bit contract sizes

* Read borrower Np Tp ratio from storage only once when kick

* If block style, proper indentation

* Proposed changes to PR #972:
- avoid calculating current LUP twice in lender kick
- change _kick function to accept proposed LUP (for regular kick proposed LUP is current LUP, for lender kick proposed LUP is calculated based on additional debt)
- in both kick cases return current LUP in kick result
- reduce gas costs by saving a Fenwick traversal
- reduce contract size by removing LUP calculation within Pool

* Cosmetic flashloan code changes, PoolCommons.flashLoan doesn't return false but always reverts if flashloan fails

* PR#983 style, remove redundant line

* PR#999 suggested improvement - calculate current settled debt only once and include in settle result for state update

* PR #962 proposed changes:
- get Fenwick deposits only once when settle with reserves
- define constants for min bond factor and max npTp ratio values

* PR #987 proposed improvement:
- add and reuse _htp helper function (instead duplicated maths)

* Continuation of PR #962: (#1008)

- Record settle amount limmit in Liquidtion struct, at the time of kick (that is instead incrementing accumulator in Borrower struct, each time debt is drawn)
- This way accumulator will be reseted when auction is settled
- fix tests

TODO:
- assert Liquidation.t0ReserveSettleAmount in unit tests (_assertAuction), make sure is set to 0 after auction settled and decreased when partial settles done
- update invariant test to check new introduced accumulator

* Misc test fixes (#1009)

* handle another reward rounding error use case

* fix intermittant fuzz test failure - cannot draw debt from liquidity in bucket 7388

* PositionManager should expect AddAboveAuctionPrice, which can happen in moveLiquidity

* Invariant fix: round up quote tokens calculated from rewarded LP (because LP rewarded are calculated in bucketTake as rewarded quote tokens -> LP rounded down)

* Pr feedback

* Fix failing unit tests

* update poolInfoUtils.borrowerInfo interface

* return thresholdPrice instead of t0ThresholdPrice

* fix compile warnings

* update borrowerInfo threshold price references

* resolved comments on borrowerInfo

---------

Co-authored-by: Ed Noepel <46749157+EdNoepel@users.noreply.github.com>
Co-authored-by: Prateek Gupta <prateek105@users.noreply.github.com>
Co-authored-by: Ed Noepel <ed@noepel.net>
Co-authored-by: Ian Harvey <iharvey@comcast.net>
Co-authored-by: mwc <matt@ajna.finance>
Co-authored-by: Ian Harvey <ith.harvey@gmail.com>
Co-authored-by: prateek105 <prateek@ajna.finance>
Co-authored-by: Mike <mikehathaway@makerdao.com>
Co-authored-by: mattcushman <36414299+mattcushman@users.noreply.github.com>
Co-authored-by: grandizzy <grandizzy.the.egg@gmail.com>
Co-authored-by: grandizzy <38490174+grandizzy@users.noreply.github.com>
* compiles and unit tests run

* remove additional rewards manager references

* remove additional references

* fix ERC20PoolPositionHandler init logic

* cleaned up folder naming and removed redundant function calls and depreciated tests (#1043)

Co-authored-by: Ian Harvey <iharvey@comcast.net>

* resolved naming and re-introduced Eds changes from 04c6ccd

* Rename Positions dir

---------

Co-authored-by: Ed Noepel <ed@noepel.net>
Co-authored-by: Mike <mikehathaway@makerdao.com>
Co-authored-by: Ian Harvey <ith.harvey@gmail.com>
Co-authored-by: Ian Harvey <iharvey@comcast.net>
Co-authored-by: grandizzy <grandizzy.the.egg@gmail.com>
* cleaned up comment

* comment and formatting cleanup

* renamed some instances of token to NFT

* cleaned up nit comment changes

---------

Co-authored-by: Ian Harvey <iharvey@comcast.net>
* Unmask id variable (#932)

* Update info multicall (#958)

* add poolRatesAndFeesMulticall; add Multicall to poolInfoUtils

* add poolDetailsMulticall method

* add natspec; remove unused functions

* update tests

* add poolBalanceDetails for remaining subgraph rpc calls

* expand poolInfoUtilsMulticall unit tests

* normalize token balances in poolBalanceDetails

* replace internal multicall usage

---------

Co-authored-by: Mike <mikehathaway@makerdao.com>

* replace PoolInfoUtils.unutilizedDepositFeeRate with .depositFeeRate, update tests

* eliminate custom multicall implementation

* remove unused multicall utility functions

---------

Co-authored-by: Brian L. McMichael <brian@brianmcmichael.com>
Co-authored-by: Mike Hathaway <mahathaway93@gmail.com>
Co-authored-by: Mike <mikehathaway@makerdao.com>
* Continuation of PR #1040 - code cleanup and comments
- remove unused errors from Kicker and Taker actions libraries
- improve natspec with revert error on CRA

* Update Pool natspec
Copy link
Contributor

@prateek105 prateek105 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1049 still needs to be merged, other changes looks good to me

grandizzy and others added 3 commits December 27, 2023 09:14
* Fix slither CI - install forge to be able to compile

* Fix slither for external libraries, remove unused IERC20Token.transfer* functions to fix https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-erc20-interface warning
…posit` library (#1049)

* add test_regression_B2_failure_moveQuote test

* Revert when unscaled quote token amount to move is 0

* Remove redundant import

* Update natspec doc

* Add 'InvalidAmount' revert in Deposits library when amount to add / remove is zero, add check to remove only non-zero deposits in settleActions

* added comment to test

* added min protection to restrict underflow and cleaned up comments in settle flows

* revised imp per Dizzy's request

* comment clean up

---------

Co-authored-by: grandizzy <grandizzy.the.egg@gmail.com>
Co-authored-by: Ian Harvey <iharvey@comcast.net>
* Add test_regression_failure_reserves_bucketTake_18precision

* Add regular take test failure

* adjust reserves tolerance to allow for large inflators

* fix issue with extreme TPs and reserves accrusing

* Add test_regression_reservesTakeAction_failure

* added tolerance for large inflator value in reserves change for takeAuction

* added comments to regression test sequences

* Commented change to reserves error margin

* fixed up invariants.md to reflect code

---------

Co-authored-by: mwc <matt@ajna.finance>
Co-authored-by: Ian Harvey <iharvey@comcast.net>
Copy link
Contributor Author

@EdNoepel EdNoepel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Cannot approve because I'm an author.

@prateek105 prateek105 self-requested a review January 4, 2024 13:32
Copy link
Contributor

@prateek105 prateek105 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@ith-harvey ith-harvey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@MikeHathaway MikeHathaway left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@EdNoepel EdNoepel merged commit 2d6bbcb into master Jan 4, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants