Sherlock 131.md: #577
Merged
Sherlock 131.md: #577
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR creates three github PR templates that should be followed: * `logic_src_changes.md` * `cosmetic_src_changes.md` * `testing_or_misc_nonsrc_changes.md` To learn more about how GH templates work -> https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/creating-a-pull-request-template-for-your-repository Co-authored-by: Ian Harvey <iharvey@comcast.net>
* merged all of the templates into one so the text will auto fill when a PR is created. * was running into this issue documented here with auto fill -> https://stackoverflow.com/questions/52139192/github-pull-requests-template-not-showing
MikeHathaway
requested review from
grandizzy,
EdNoepel,
ith-harvey and
prateek105
grandizzy
approved these changes
Feb 8, 2023
ith-harvey
added a commit
that referenced
this pull request
Feb 13, 2023
…review (#594) * Develop (#565) * reworked the RNG to use a deterministic seed (#523) * reworked the RNG to use a deterministic seed * reordered params such that they are in the same order as "bound" is called * Nonfuzzy fill (#529) * work in progress * reproduced fenwick tree failures at last index * proposed changes in light that max index is special case for prefix suim (#533) * proposed changes in light that max index is special case for prefix suim * removed comment Co-authored-by: mwc <matt@ajna.finance> Co-authored-by: Ed Noepel <ed@lpl.io> * removed two temp tests * ported Matt's changes to the fuzzed implementation Co-authored-by: mattcushman <36414299+mattcushman@users.noreply.github.com> Co-authored-by: mwc <matt@ajna.finance> Co-authored-by: Ed Noepel <ed@lpl.io> Co-authored-by: mattcushman <36414299+mattcushman@users.noreply.github.com> Co-authored-by: mwc <matt@ajna.finance> Co-authored-by: Ed Noepel <ed@lpl.io> * Docs (#535) * Diagrams * Add components * Updates * Center components * add pool architectire diagrams (#536) Co-authored-by: prateek105 <prateek@ajna.finance> Co-authored-by: Prateek Gupta <prateek105@users.noreply.github.com> Co-authored-by: prateek105 <prateek@ajna.finance> * Test Coverage and Tests Style improvements (#541) * - Ajna balance less than rewards tests * NFT take tests * Loan heap test coverage * PoolCommons 100% test coverage - no interest earned when htp > max price * Test coverage move dusty amount * Changes after review: - comments - common style on touched tests * Common style across all unit tests * PositionManager and RewardsManager deployment (#547) * replaced bash script with forge script, update for PositionManager and RewardsManager * intentionally broke example addresses so no one tries to use them for anything * CI updates (#549) * testing set-output for size-check * update to cachev3 * make it look like an env var * use it like an env var * revert to old usage syntax * deleted redundant forge test, since code coverage already runs the tests * Made Token limitations more robust (#557) # Description of change ## High level * Altered `README.MD` to reflect a more robust definition of incompatible tokens --------- Co-authored-by: Ed Noepel <46749157+EdNoepel@users.noreply.github.com> Co-authored-by: mattcushman <36414299+mattcushman@users.noreply.github.com> Co-authored-by: mwc <matt@ajna.finance> Co-authored-by: Ed Noepel <ed@lpl.io> Co-authored-by: Prateek Gupta <prateek105@users.noreply.github.com> Co-authored-by: prateek105 <prateek@ajna.finance> Co-authored-by: Ian Harvey <ith.harvey@gmail.com> * Consistent naming, replace lpTokens with LPs (#543) * Consistent naming, replace lpTokens with LPs * Changes after review: replace _lpTokenAllowances with _lpAllowances * Apply test style * gas improvements (#542) * Gas improvements: - LenderActions.removeQuoteToken: reuse depositTime instead loading again from storage * RewardsManager: calculateAndClaimRewards, increment and then use next epoch * Remove duped calculation of toPrice from moveQuoteToken function * Take underflows when full pool debt repaid (#551) Take underflows when full pool debt repaid Scenario is exposed in testTakeAuctionRepaidAmountGreaterThanPoolDebt test: - when auction debt is fully repaid by take action then accrued pool debt value is less than repaid amount with 1 unit of WAD. When the repaid amount is subtracted from pool debt value (normally leaving no debt in pool) the operation underflows. - this happens because repaid debt is calculated using t0 value (t0 repaid debt * inflator) and then subtracted from already calculated pool debt - fix is to calculate pool debt as (t0 pool debt - t0 repaid debt) * inflator, hence preventing rounding issues The scope of this PR is extended to uniformize the t0 / accrued debt values calculation across all contracts by using pattern below: - when values relative to t0 are changed then t0 is updated first and then transformed in actual value (by multiplying it with inflator value) - all accumulations of pool or borrower debt are done on spot and not deferred to Pool contracts (which only save states) * Sherlock 103.md: Remaining collateral used by ERC721Pool is missed in Auctions take and bucketTake return structures (#566) * Nft pledge accumulator (#567) * Add test to expose pledgeCollateral inconsistency when settle NFT auction * Fix pledgedCollateral accumulator * Fix settle auction on bucket take * Add test for auction settle and compensate borrower when settle pool debt. Check pool collateral conistency across buckets * Fix for pledged collateral accumulator when settle auction with regular take * Reorg test, add _assertCollateralInvariants helper * Sherlock 162.md: ERC721Pool taker callback misreports quote funds whenever there was collateral amount rounding (#568) * Flashloan non18 decimals (#569) Fix flashloan, reserve auction and settle auction for non standard collateral / quote token precision Always use token precision when transferring during flashloans (and do not scale amounts to pool precision). For pool reserves use scaled pool token balance (WAD) so the reserves are accurately calculated. * Fix flashloan and reserve auction for non standard collateral / quote token precision * Changes after review: rename _getScaledPoolQuoteTokenBalance to _getNormalizedPoolQuoteTokenBalance * remove redundant code (#558) Co-authored-by: prateek105 <prateek@ajna.finance> * Reduce flashloans logic, introduce _isFlashloanSupported function (default returns true for quote token address, overriden in ERC20 pool to allow both quote and collateral token). Reorg the flashloan reverts conditions * Safety measure: (#588) - setting the remaining collateral to the current by default in drawDebt/repayDebt/_takeLoan functions - update alongside with borrower.colalteral when more collateral pledged Note: the remainingCollateral is used by NFT pool and only in the case of auction settlement. This commit doesn't change any logic but it's a safety measure for future developments that could alter this logic * Sherlock 104.md: Settled collateral of a borrower aren't available for lenders until borrower's debt is fully cleared (#570) - in ERC721Pool settle rebalance token ids if there's collateral settled - remove unused return param from Auction.settle - fix failing test * Sherlock 105.md: ERC721Pool's mergeOrRemoveCollateral allows to remove collateral while auction is clearable (#571) - check _revertIfAuctionClearable in mergeOrRemoveCollateral function - update tests: add _assertMergeRemoveCollateralAuctionNotClearedRevert and assert in ERC721 mergeOrRemove unit tests * Sherlock 101.md: Flashloan end result isn't controlled (#572) - record pool balance before transfer flash loaned ERC20 tokens - compare pool balance after flashloan with the initial / recorded balance - intorduce FlashloanIncorrectBalance error to revert if initial and current pool balances are different - update tests, introduce strategy that transfers tokens to pool, hence increasing pool balance, and expect FlashloanIncorrectBalance revert * Sherlock 183.md: RewardsManager doesn't delete old bucket snapshot info on unstaking (#573) - add getBucketStateStakeInfo view function to return info of recorded bucket at stake time - when unstake loop through positions and delete BucketState structs - in test helper _unstakeToken make sure there's no bucket state info remaining after token unstaked * Sherlock 151.md: Permanent freezing of unclaimed yield (#574) * Sherlock 151.md: Permanent freezing of unclaimed yield - new EpochNotAvailable custom error added - in claimRewards revert if epoch to claim is greater than latest burn epoch - add test * Changes after review: freeze typo * Sherlock 134.md: (#575) reported as Transferring funds to yourself increases your balance actually: Transferring funds to yourself reset balance to 0 (effect is that owner lose all their LPs as it is removed as bucket lender at LenderActions#L550) - added new custom error TransferToSameOwner, revert in Pool.transferLPs if new owner same as owner - added revert test on transfer to same address as owner address - tests style format * Sherlock 075.md: If borrower or kicker got blacklisted by asset contract their collateral or bond funds can be permanently frozen with the pool (#578) - Add recipient arg to withdrawBonds and repayDebt functions - add tests TBD: should we check and revert if recipient is 0x address * Sherlock 139.md (#579) * Sherlock 139.md: scaledQuoteTokenAmount isn't updated to be collateral sell value in the quote token constraint case of _calculateTakeFlowsAndBondChange - setting the `scaledQuoteTokenAmount` to the `C * p` as a final step of quote token amount constraint case - update tests * Added an example to show that the new exchange rate is invariant * Add tearDown to test, remove return --------- Co-authored-by: mwc <matt@ajna.finance> * Sherlock 096.md: Interest rate for pool is bounded wrongly (#580) - allow creation of pools with min / max rate values - add tests * Sherlock 068.md (#581) * Create standardized PR templates for contracts (#539) This PR creates three github PR templates that should be followed: * `logic_src_changes.md` * `cosmetic_src_changes.md` * `testing_or_misc_nonsrc_changes.md` To learn more about how GH templates work -> https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/creating-a-pull-request-template-for-your-repository Co-authored-by: Ian Harvey <iharvey@comcast.net> * Single template (#546) * merged all of the templates into one so the text will auto fill when a PR is created. * was running into this issue documented here with auto fill -> https://stackoverflow.com/questions/52139192/github-pull-requests-template-not-showing * Sherlock 068.md: ERC721Pool's take will proceed with truncated collateral amount and full debt when borrower's collateral is fractional - Early revert take action when borrower's collateral is less than `1` if the pool is ERC721 - ensure collateral, quote tokens and bond calculations are alwyas performed for the number of NFTs that will be taken: instead passing Maths.min(borrower_.collateral, params_.takeCollateral) to _calculateTakeFlowsAndBondChange function calculate the rounded down collateral that can be taken from borrower Example: if borrower has 1.2 worth of colalteral and taker pass an amount of 2 collateral to take then calculations happens for Min(rounded(1.2), 2) = 1 NFT token If borrower debt can be covered with less than 1 NFT then _calculateTakeFlowsAndBondChange will return a fractioned NFT (0.5 for example) and the difference between 1 NFT taken and 0.5 will be paid with excess quote tokens --------- Co-authored-by: Ian Harvey <ith.harvey@gmail.com> Co-authored-by: Ian Harvey <iharvey@comcast.net> * fix sherlock issues 120 and 121 (#576) * Create standardized PR templates for contracts (#539) This PR creates three github PR templates that should be followed: * `logic_src_changes.md` * `cosmetic_src_changes.md` * `testing_or_misc_nonsrc_changes.md` To learn more about how GH templates work -> https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/creating-a-pull-request-template-for-your-repository Co-authored-by: Ian Harvey <iharvey@comcast.net> * Single template (#546) * merged all of the templates into one so the text will auto fill when a PR is created. * was running into this issue documented here with auto fill -> https://stackoverflow.com/questions/52139192/github-pull-requests-template-not-showing * fix sherlock issue 121 precision loss by multiplying before dividing * add _transferAjnaRewards method --------- Co-authored-by: Ian Harvey <ith.harvey@gmail.com> Co-authored-by: Ian Harvey <iharvey@comcast.net> Co-authored-by: Mike <mikehathaway@makerdao.com> * Sherlock 163,140, 34 & 31: Remove support for non standard NFTs (#585) * merged all of the templates into one so the text will auto fill when a PR is created. * was running into this issue documented here with auto fill -> https://stackoverflow.com/questions/52139192/github-pull-requests-template-not-showing * Remove support for non standard NFTs (will have to be wrapped) * Sherlock 151: Changes for initial fix: (#596) * Sherlock 151: Changes for initial fix: - pass isManualEpoch_ param to _claimRewards function and do the epoch validation only if true (no need to validate for unstake which doesn't receive epoch as a param) - load stake struct from storage only once (and pass as a param to _claimRewards function) minimize calls to load ajna pool from stake storage * Changes after review: rename isManualEpoch to validateEpoch param * Sherlock 134.md changes after review: move check in LenderActions external library to keep logic in same place (#597) * Sherlock 98.md: (#577) * Create standardized PR templates for contracts (#539) This PR creates three github PR templates that should be followed: * `logic_src_changes.md` * `cosmetic_src_changes.md` * `testing_or_misc_nonsrc_changes.md` To learn more about how GH templates work -> https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/creating-a-pull-request-template-for-your-repository Co-authored-by: Ian Harvey <iharvey@comcast.net> * Single template (#546) * merged all of the templates into one so the text will auto fill when a PR is created. * was running into this issue documented here with auto fill -> https://stackoverflow.com/questions/52139192/github-pull-requests-template-not-showing * add reentrancy guard to PositionManager.mint(); update mint natspec --------- Co-authored-by: Ian Harvey <ith.harvey@gmail.com> Co-authored-by: Ian Harvey <iharvey@comcast.net> Co-authored-by: Mike <mikehathaway@makerdao.com> * Sherlock 145: Take adjustments: `msg.sender` provides QT instead of `_callee` (#589) <!--- No need to add special tag src/ & non src/ changes you need the following (that apply): --> # Description of change ## High level * inside of `take` for ERC721 and ERC20 pools changed the argument passed in `_transferQuoteTokenFrom()` from `_callee` to `msg.sender`. * This update now makes the `_take` call match other implementations such as Maker * NOTE: 10-30K spike in gas cost because of change. <!--- Add the `Status: Needs Auditor Approval` tags CHANGES IN /SRC DIR: - renaming (not retyping or resizing) of variables & methods - reordering and moving of functions in files - lite moving of functions accross files - comments src/ changes you need the following (that apply): --> # Description of bug or vulnerability and solution [ERC20Pool](https://github.com/sherlock-audit/2023-01-ajna/blob/main/contracts/src/ERC20Pool.sol#L403) and [ERC721Pool](https://github.com/sherlock-audit/2023-01-ajna/blob/main/contracts/src/ERC721Pool.sol#L405) implement the take functions, which buy collateral from auction in exchange for quote tokens. The address to pull quote tokens from is specified in the `_callee` argument, which allows anyone to call the functions and pass an address that has previously approved spending of the quote token to the pool. As a result, such an address will pay for the liquidation and will receive the collateral. The solution makes it so the `msg.sender` provides the QT when calling the `take()` method. # Contract size ## Pre Change ``` ============ Deployment Bytecode Sizes ============ ERC721Pool - 22,267B (90.60%) ERC20Pool - 20,972B (85.33%) ``` ## Post Change ``` ============ Deployment Bytecode Sizes ============ ERC721Pool - 22,267B (90.60%) ERC20Pool - 20,972B (85.33%) ``` # Gas usage ## Pre Change ``` | src/ERC20Pool.sol:ERC20Pool contract | | | | | | |--------------------------------------|-----------------|--------|--------|--------|---------| | take | 8550 | 158087 | 165977 | 520357 | 32 | src/ERC721Pool.sol:ERC721Pool contract | | | | | | |----------------------------------------|-----------------|--------|--------|--------|---------| | take | 11246 | 292029 | 375721 | 632601 | 11 | ``` ## Post Change ``` | src/ERC20Pool.sol:ERC20Pool contract | | | | | | |--------------------------------------|-----------------|--------|--------|--------|---------| | take | 8550 | 164634 | 184194 | 520356 | 33 | | src/ERC721Pool.sol:ERC721Pool contract | | | | | | |----------------------------------------|-----------------|--------|--------|----------|---------| | take | 11246 | 320810 | 387200 | 637400 | 12 | ``` * Sherlock 70: user can drawDebt that is below dust amount (#598) * ensure borrower debt exceeds dust amount regardless of loan count * Revert "ensure borrower debt exceeds dust amount regardless of loan count" This reverts commit c0a64f9. * re-applied changes lost when rebasing * Sherlock 68: (#599) - review update: add suggested revert to be excessively safe (for the case if current or future version of _calculateTakeFlowsAndBondChange() malfunctions producing collateral bigger than its collateral argument) * Sherlock 92: startClaimableReserveAuction using old inflator (#587) * Sherlock 92: startClaimableReserveAuction using old inflator * Add test to show interest accrued when start reserve auction * Revert "Sherlock 92: startClaimableReserveAuction using old inflator (#587)" This reverts commit 1414435. * change safeTransferFrom to transferFrom to support Oasis (#592) * Create standardized PR templates for contracts (#539) This PR creates three github PR templates that should be followed: * `logic_src_changes.md` * `cosmetic_src_changes.md` * `testing_or_misc_nonsrc_changes.md` To learn more about how GH templates work -> https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/creating-a-pull-request-template-for-your-repository Co-authored-by: Ian Harvey <iharvey@comcast.net> * Single template (#546) * merged all of the templates into one so the text will auto fill when a PR is created. * was running into this issue documented here with auto fill -> https://stackoverflow.com/questions/52139192/github-pull-requests-template-not-showing * change safeTransferFrom to transferFrom --------- Co-authored-by: Ian Harvey <ith.harvey@gmail.com> Co-authored-by: Ian Harvey <iharvey@comcast.net> Co-authored-by: Mike <mikehathaway@makerdao.com> * Sherlock 148: Use pool debt when calculating MOMP in Loans.update (#586) * Update comments: (#590) - ERC721.addCollateral changes bucketTokenIds - ERC721.repayDebt changes borrowerTokenIds and bucketTokenIds (when auction settled) - fix comment in Auctions._calculateTakeFlowsAndBondChange for take below NP * fix safeTransferFromWithPermit (#593) * Create standardized PR templates for contracts (#539) This PR creates three github PR templates that should be followed: * `logic_src_changes.md` * `cosmetic_src_changes.md` * `testing_or_misc_nonsrc_changes.md` To learn more about how GH templates work -> https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/creating-a-pull-request-template-for-your-repository Co-authored-by: Ian Harvey <iharvey@comcast.net> * Single template (#546) * merged all of the templates into one so the text will auto fill when a PR is created. * was running into this issue documented here with auto fill -> https://stackoverflow.com/questions/52139192/github-pull-requests-template-not-showing * fix safeTransferFromWithPermit --------- Co-authored-by: Ian Harvey <ith.harvey@gmail.com> Co-authored-by: Ian Harvey <iharvey@comcast.net> Co-authored-by: Mike <mikehathaway@makerdao.com> * LPS and Exchange Rate as WADs - Sherlock 133 & 83 (#606) * LPs as WADs * - record lender LPs in remove collateral and quote token only if bucket doesn't go bankrupt - for remove colalteral use the same way to calculate LPs as when rewarding, to avoid precision issues - tearDown should consider that bucket can go bankrupt, hence lender LPs should be checked = 0 taking this into account (lenderInfo function is doing this) - all tests passing but testDepositTwoActorSameBucket fails in tearDown - that's because there are quote tokens remaining without being backed by LPs (were previously redeemed in tearDown for collateral) * Use consistent way to calculate LPs reported to scaled amounts, remove unscaled rate exchange, fix tests Couple of take tests fail in tearDown leaving small dust deposit * Cleanup Maths library, update comments * Fix remaining tests * Fix sequence to empty buckets, one single settle test remaining failing in tearDown * Add bucket clean out sequence, make consistent for removeCollateral too (per Sherlock 133) * Add bankruptcy check for move quote token from bucket and unit test * Guard against underflows in removeQuoteToken (due to roundings when transforming from scaled to unscaled amount) * Changes after review: - calculate amount using collateralAmount_ rather than bucketCollateral - remove dead code in remove quote token * Sherlock 39: expiration timestamp and slippage control (#600) * ignore forge script tx broadcasts * provide LUP limit when pulling collateral * expiration checks for adding to buckets * unit tests for addQuote and addCollateral expiration * expiration checks for moveQuote * ensure borrower debt exceeds dust amount regardless of loan count * Revert "ensure borrower debt exceeds dust amount regardless of loan count" This reverts commit c0a64f9. * feedback from PR #600 (#604) * removed comment * Address review comments, update comment for _revertIfLupDroppedBelowL… (#607) * Address review comments, update comment for _revertIfLupDroppedBelowLimit, changed LimitIndexReached to LimitIndexExceeded * Remove lup Id variable, calculate LUP price directly * Remove unused struct member --------- Co-authored-by: grandizzy <38490174+grandizzy@users.noreply.github.com> * Fix test compile * Protocol invariants testing (#609) * Invariants * Exclude invariants from code coverage and GH action * Fix coverage GH target * Changes after review * Disallow auctioned borrowers to draw more debt or pull collateral if auction is not settled. (#611) This can happen in a scenario where user becomes recollateralized to the new LUP. Without these checks, and since borrow and pull doesn't settle auctions, borrower is not removed from auction queue but added in loan queue as well, which violate invariant - **A3**: number of borrowers with debt = number of loans + number of auctioned borrowers * Sherlock 73 (#591) * Create standardized PR templates for contracts (#539) This PR creates three github PR templates that should be followed: * `logic_src_changes.md` * `cosmetic_src_changes.md` * `testing_or_misc_nonsrc_changes.md` To learn more about how GH templates work -> https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/creating-a-pull-request-template-for-your-repository Co-authored-by: Ian Harvey <iharvey@comcast.net> * Single template (#546) * merged all of the templates into one so the text will auto fill when a PR is created. * was running into this issue documented here with auto fill -> https://stackoverflow.com/questions/52139192/github-pull-requests-template-not-showing * update getNFTSubsetHash to uniquely sort array of tokenIds * add natspec and slight gas optimization * change encodePacked to encode to add byte padding * pool deployer gas optimization * check sort order instead of sorting on chain * pr feedback to check for duplicate tokenIds --------- Co-authored-by: Ian Harvey <ith.harvey@gmail.com> Co-authored-by: Ian Harvey <iharvey@comcast.net> Co-authored-by: Mike <mikehathaway@makerdao.com> --------- Co-authored-by: Ed Noepel <46749157+EdNoepel@users.noreply.github.com> Co-authored-by: mattcushman <36414299+mattcushman@users.noreply.github.com> Co-authored-by: mwc <matt@ajna.finance> Co-authored-by: Ed Noepel <ed@lpl.io> Co-authored-by: Prateek Gupta <prateek105@users.noreply.github.com> Co-authored-by: prateek105 <prateek@ajna.finance> Co-authored-by: Ian Harvey <ith.harvey@gmail.com> Co-authored-by: Ian Harvey <iharvey@comcast.net> Co-authored-by: Mike Hathaway <mahathaway93@gmail.com> Co-authored-by: Mike <mikehathaway@makerdao.com>
grandizzy
added a commit
that referenced
this pull request
Mar 16, 2023
* Create standardized PR templates for contracts (#539) This PR creates three github PR templates that should be followed: * `logic_src_changes.md` * `cosmetic_src_changes.md` * `testing_or_misc_nonsrc_changes.md` To learn more about how GH templates work -> https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/creating-a-pull-request-template-for-your-repository Co-authored-by: Ian Harvey <iharvey@comcast.net> * Single template (#546) * merged all of the templates into one so the text will auto fill when a PR is created. * was running into this issue documented here with auto fill -> https://stackoverflow.com/questions/52139192/github-pull-requests-template-not-showing * Sherlock reviewed fixes - to be merged in master only after Sherlock review (#594) * Develop (#565) * reworked the RNG to use a deterministic seed (#523) * reworked the RNG to use a deterministic seed * reordered params such that they are in the same order as "bound" is called * Nonfuzzy fill (#529) * work in progress * reproduced fenwick tree failures at last index * proposed changes in light that max index is special case for prefix suim (#533) * proposed changes in light that max index is special case for prefix suim * removed comment Co-authored-by: mwc <matt@ajna.finance> Co-authored-by: Ed Noepel <ed@lpl.io> * removed two temp tests * ported Matt's changes to the fuzzed implementation Co-authored-by: mattcushman <36414299+mattcushman@users.noreply.github.com> Co-authored-by: mwc <matt@ajna.finance> Co-authored-by: Ed Noepel <ed@lpl.io> Co-authored-by: mattcushman <36414299+mattcushman@users.noreply.github.com> Co-authored-by: mwc <matt@ajna.finance> Co-authored-by: Ed Noepel <ed@lpl.io> * Docs (#535) * Diagrams * Add components * Updates * Center components * add pool architectire diagrams (#536) Co-authored-by: prateek105 <prateek@ajna.finance> Co-authored-by: Prateek Gupta <prateek105@users.noreply.github.com> Co-authored-by: prateek105 <prateek@ajna.finance> * Test Coverage and Tests Style improvements (#541) * - Ajna balance less than rewards tests * NFT take tests * Loan heap test coverage * PoolCommons 100% test coverage - no interest earned when htp > max price * Test coverage move dusty amount * Changes after review: - comments - common style on touched tests * Common style across all unit tests * PositionManager and RewardsManager deployment (#547) * replaced bash script with forge script, update for PositionManager and RewardsManager * intentionally broke example addresses so no one tries to use them for anything * CI updates (#549) * testing set-output for size-check * update to cachev3 * make it look like an env var * use it like an env var * revert to old usage syntax * deleted redundant forge test, since code coverage already runs the tests * Made Token limitations more robust (#557) # Description of change ## High level * Altered `README.MD` to reflect a more robust definition of incompatible tokens --------- Co-authored-by: Ed Noepel <46749157+EdNoepel@users.noreply.github.com> Co-authored-by: mattcushman <36414299+mattcushman@users.noreply.github.com> Co-authored-by: mwc <matt@ajna.finance> Co-authored-by: Ed Noepel <ed@lpl.io> Co-authored-by: Prateek Gupta <prateek105@users.noreply.github.com> Co-authored-by: prateek105 <prateek@ajna.finance> Co-authored-by: Ian Harvey <ith.harvey@gmail.com> * Consistent naming, replace lpTokens with LPs (#543) * Consistent naming, replace lpTokens with LPs * Changes after review: replace _lpTokenAllowances with _lpAllowances * Apply test style * gas improvements (#542) * Gas improvements: - LenderActions.removeQuoteToken: reuse depositTime instead loading again from storage * RewardsManager: calculateAndClaimRewards, increment and then use next epoch * Remove duped calculation of toPrice from moveQuoteToken function * Take underflows when full pool debt repaid (#551) Take underflows when full pool debt repaid Scenario is exposed in testTakeAuctionRepaidAmountGreaterThanPoolDebt test: - when auction debt is fully repaid by take action then accrued pool debt value is less than repaid amount with 1 unit of WAD. When the repaid amount is subtracted from pool debt value (normally leaving no debt in pool) the operation underflows. - this happens because repaid debt is calculated using t0 value (t0 repaid debt * inflator) and then subtracted from already calculated pool debt - fix is to calculate pool debt as (t0 pool debt - t0 repaid debt) * inflator, hence preventing rounding issues The scope of this PR is extended to uniformize the t0 / accrued debt values calculation across all contracts by using pattern below: - when values relative to t0 are changed then t0 is updated first and then transformed in actual value (by multiplying it with inflator value) - all accumulations of pool or borrower debt are done on spot and not deferred to Pool contracts (which only save states) * Sherlock 103.md: Remaining collateral used by ERC721Pool is missed in Auctions take and bucketTake return structures (#566) * Nft pledge accumulator (#567) * Add test to expose pledgeCollateral inconsistency when settle NFT auction * Fix pledgedCollateral accumulator * Fix settle auction on bucket take * Add test for auction settle and compensate borrower when settle pool debt. Check pool collateral conistency across buckets * Fix for pledged collateral accumulator when settle auction with regular take * Reorg test, add _assertCollateralInvariants helper * Sherlock 162.md: ERC721Pool taker callback misreports quote funds whenever there was collateral amount rounding (#568) * Flashloan non18 decimals (#569) Fix flashloan, reserve auction and settle auction for non standard collateral / quote token precision Always use token precision when transferring during flashloans (and do not scale amounts to pool precision). For pool reserves use scaled pool token balance (WAD) so the reserves are accurately calculated. * Fix flashloan and reserve auction for non standard collateral / quote token precision * Changes after review: rename _getScaledPoolQuoteTokenBalance to _getNormalizedPoolQuoteTokenBalance * remove redundant code (#558) Co-authored-by: prateek105 <prateek@ajna.finance> * Reduce flashloans logic, introduce _isFlashloanSupported function (default returns true for quote token address, overriden in ERC20 pool to allow both quote and collateral token). Reorg the flashloan reverts conditions * Safety measure: (#588) - setting the remaining collateral to the current by default in drawDebt/repayDebt/_takeLoan functions - update alongside with borrower.colalteral when more collateral pledged Note: the remainingCollateral is used by NFT pool and only in the case of auction settlement. This commit doesn't change any logic but it's a safety measure for future developments that could alter this logic * Sherlock 104.md: Settled collateral of a borrower aren't available for lenders until borrower's debt is fully cleared (#570) - in ERC721Pool settle rebalance token ids if there's collateral settled - remove unused return param from Auction.settle - fix failing test * Sherlock 105.md: ERC721Pool's mergeOrRemoveCollateral allows to remove collateral while auction is clearable (#571) - check _revertIfAuctionClearable in mergeOrRemoveCollateral function - update tests: add _assertMergeRemoveCollateralAuctionNotClearedRevert and assert in ERC721 mergeOrRemove unit tests * Sherlock 101.md: Flashloan end result isn't controlled (#572) - record pool balance before transfer flash loaned ERC20 tokens - compare pool balance after flashloan with the initial / recorded balance - intorduce FlashloanIncorrectBalance error to revert if initial and current pool balances are different - update tests, introduce strategy that transfers tokens to pool, hence increasing pool balance, and expect FlashloanIncorrectBalance revert * Sherlock 183.md: RewardsManager doesn't delete old bucket snapshot info on unstaking (#573) - add getBucketStateStakeInfo view function to return info of recorded bucket at stake time - when unstake loop through positions and delete BucketState structs - in test helper _unstakeToken make sure there's no bucket state info remaining after token unstaked * Sherlock 151.md: Permanent freezing of unclaimed yield (#574) * Sherlock 151.md: Permanent freezing of unclaimed yield - new EpochNotAvailable custom error added - in claimRewards revert if epoch to claim is greater than latest burn epoch - add test * Changes after review: freeze typo * Sherlock 134.md: (#575) reported as Transferring funds to yourself increases your balance actually: Transferring funds to yourself reset balance to 0 (effect is that owner lose all their LPs as it is removed as bucket lender at LenderActions#L550) - added new custom error TransferToSameOwner, revert in Pool.transferLPs if new owner same as owner - added revert test on transfer to same address as owner address - tests style format * Sherlock 075.md: If borrower or kicker got blacklisted by asset contract their collateral or bond funds can be permanently frozen with the pool (#578) - Add recipient arg to withdrawBonds and repayDebt functions - add tests TBD: should we check and revert if recipient is 0x address * Sherlock 139.md (#579) * Sherlock 139.md: scaledQuoteTokenAmount isn't updated to be collateral sell value in the quote token constraint case of _calculateTakeFlowsAndBondChange - setting the `scaledQuoteTokenAmount` to the `C * p` as a final step of quote token amount constraint case - update tests * Added an example to show that the new exchange rate is invariant * Add tearDown to test, remove return --------- Co-authored-by: mwc <matt@ajna.finance> * Sherlock 096.md: Interest rate for pool is bounded wrongly (#580) - allow creation of pools with min / max rate values - add tests * Sherlock 068.md (#581) * Create standardized PR templates for contracts (#539) This PR creates three github PR templates that should be followed: * `logic_src_changes.md` * `cosmetic_src_changes.md` * `testing_or_misc_nonsrc_changes.md` To learn more about how GH templates work -> https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/creating-a-pull-request-template-for-your-repository Co-authored-by: Ian Harvey <iharvey@comcast.net> * Single template (#546) * merged all of the templates into one so the text will auto fill when a PR is created. * was running into this issue documented here with auto fill -> https://stackoverflow.com/questions/52139192/github-pull-requests-template-not-showing * Sherlock 068.md: ERC721Pool's take will proceed with truncated collateral amount and full debt when borrower's collateral is fractional - Early revert take action when borrower's collateral is less than `1` if the pool is ERC721 - ensure collateral, quote tokens and bond calculations are alwyas performed for the number of NFTs that will be taken: instead passing Maths.min(borrower_.collateral, params_.takeCollateral) to _calculateTakeFlowsAndBondChange function calculate the rounded down collateral that can be taken from borrower Example: if borrower has 1.2 worth of colalteral and taker pass an amount of 2 collateral to take then calculations happens for Min(rounded(1.2), 2) = 1 NFT token If borrower debt can be covered with less than 1 NFT then _calculateTakeFlowsAndBondChange will return a fractioned NFT (0.5 for example) and the difference between 1 NFT taken and 0.5 will be paid with excess quote tokens --------- Co-authored-by: Ian Harvey <ith.harvey@gmail.com> Co-authored-by: Ian Harvey <iharvey@comcast.net> * fix sherlock issues 120 and 121 (#576) * Create standardized PR templates for contracts (#539) This PR creates three github PR templates that should be followed: * `logic_src_changes.md` * `cosmetic_src_changes.md` * `testing_or_misc_nonsrc_changes.md` To learn more about how GH templates work -> https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/creating-a-pull-request-template-for-your-repository Co-authored-by: Ian Harvey <iharvey@comcast.net> * Single template (#546) * merged all of the templates into one so the text will auto fill when a PR is created. * was running into this issue documented here with auto fill -> https://stackoverflow.com/questions/52139192/github-pull-requests-template-not-showing * fix sherlock issue 121 precision loss by multiplying before dividing * add _transferAjnaRewards method --------- Co-authored-by: Ian Harvey <ith.harvey@gmail.com> Co-authored-by: Ian Harvey <iharvey@comcast.net> Co-authored-by: Mike <mikehathaway@makerdao.com> * Sherlock 163,140, 34 & 31: Remove support for non standard NFTs (#585) * merged all of the templates into one so the text will auto fill when a PR is created. * was running into this issue documented here with auto fill -> https://stackoverflow.com/questions/52139192/github-pull-requests-template-not-showing * Remove support for non standard NFTs (will have to be wrapped) * Sherlock 151: Changes for initial fix: (#596) * Sherlock 151: Changes for initial fix: - pass isManualEpoch_ param to _claimRewards function and do the epoch validation only if true (no need to validate for unstake which doesn't receive epoch as a param) - load stake struct from storage only once (and pass as a param to _claimRewards function) minimize calls to load ajna pool from stake storage * Changes after review: rename isManualEpoch to validateEpoch param * Sherlock 134.md changes after review: move check in LenderActions external library to keep logic in same place (#597) * Sherlock 98.md: (#577) * Create standardized PR templates for contracts (#539) This PR creates three github PR templates that should be followed: * `logic_src_changes.md` * `cosmetic_src_changes.md` * `testing_or_misc_nonsrc_changes.md` To learn more about how GH templates work -> https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/creating-a-pull-request-template-for-your-repository Co-authored-by: Ian Harvey <iharvey@comcast.net> * Single template (#546) * merged all of the templates into one so the text will auto fill when a PR is created. * was running into this issue documented here with auto fill -> https://stackoverflow.com/questions/52139192/github-pull-requests-template-not-showing * add reentrancy guard to PositionManager.mint(); update mint natspec --------- Co-authored-by: Ian Harvey <ith.harvey@gmail.com> Co-authored-by: Ian Harvey <iharvey@comcast.net> Co-authored-by: Mike <mikehathaway@makerdao.com> * Sherlock 145: Take adjustments: `msg.sender` provides QT instead of `_callee` (#589) <!--- No need to add special tag src/ & non src/ changes you need the following (that apply): --> # Description of change ## High level * inside of `take` for ERC721 and ERC20 pools changed the argument passed in `_transferQuoteTokenFrom()` from `_callee` to `msg.sender`. * This update now makes the `_take` call match other implementations such as Maker * NOTE: 10-30K spike in gas cost because of change. <!--- Add the `Status: Needs Auditor Approval` tags CHANGES IN /SRC DIR: - renaming (not retyping or resizing) of variables & methods - reordering and moving of functions in files - lite moving of functions accross files - comments src/ changes you need the following (that apply): --> # Description of bug or vulnerability and solution [ERC20Pool](https://github.com/sherlock-audit/2023-01-ajna/blob/main/contracts/src/ERC20Pool.sol#L403) and [ERC721Pool](https://github.com/sherlock-audit/2023-01-ajna/blob/main/contracts/src/ERC721Pool.sol#L405) implement the take functions, which buy collateral from auction in exchange for quote tokens. The address to pull quote tokens from is specified in the `_callee` argument, which allows anyone to call the functions and pass an address that has previously approved spending of the quote token to the pool. As a result, such an address will pay for the liquidation and will receive the collateral. The solution makes it so the `msg.sender` provides the QT when calling the `take()` method. # Contract size ## Pre Change ``` ============ Deployment Bytecode Sizes ============ ERC721Pool - 22,267B (90.60%) ERC20Pool - 20,972B (85.33%) ``` ## Post Change ``` ============ Deployment Bytecode Sizes ============ ERC721Pool - 22,267B (90.60%) ERC20Pool - 20,972B (85.33%) ``` # Gas usage ## Pre Change ``` | src/ERC20Pool.sol:ERC20Pool contract | | | | | | |--------------------------------------|-----------------|--------|--------|--------|---------| | take | 8550 | 158087 | 165977 | 520357 | 32 | src/ERC721Pool.sol:ERC721Pool contract | | | | | | |----------------------------------------|-----------------|--------|--------|--------|---------| | take | 11246 | 292029 | 375721 | 632601 | 11 | ``` ## Post Change ``` | src/ERC20Pool.sol:ERC20Pool contract | | | | | | |--------------------------------------|-----------------|--------|--------|--------|---------| | take | 8550 | 164634 | 184194 | 520356 | 33 | | src/ERC721Pool.sol:ERC721Pool contract | | | | | | |----------------------------------------|-----------------|--------|--------|----------|---------| | take | 11246 | 320810 | 387200 | 637400 | 12 | ``` * Sherlock 70: user can drawDebt that is below dust amount (#598) * ensure borrower debt exceeds dust amount regardless of loan count * Revert "ensure borrower debt exceeds dust amount regardless of loan count" This reverts commit c0a64f9. * re-applied changes lost when rebasing * Sherlock 68: (#599) - review update: add suggested revert to be excessively safe (for the case if current or future version of _calculateTakeFlowsAndBondChange() malfunctions producing collateral bigger than its collateral argument) * Sherlock 92: startClaimableReserveAuction using old inflator (#587) * Sherlock 92: startClaimableReserveAuction using old inflator * Add test to show interest accrued when start reserve auction * Revert "Sherlock 92: startClaimableReserveAuction using old inflator (#587)" This reverts commit 1414435. * change safeTransferFrom to transferFrom to support Oasis (#592) * Create standardized PR templates for contracts (#539) This PR creates three github PR templates that should be followed: * `logic_src_changes.md` * `cosmetic_src_changes.md` * `testing_or_misc_nonsrc_changes.md` To learn more about how GH templates work -> https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/creating-a-pull-request-template-for-your-repository Co-authored-by: Ian Harvey <iharvey@comcast.net> * Single template (#546) * merged all of the templates into one so the text will auto fill when a PR is created. * was running into this issue documented here with auto fill -> https://stackoverflow.com/questions/52139192/github-pull-requests-template-not-showing * change safeTransferFrom to transferFrom --------- Co-authored-by: Ian Harvey <ith.harvey@gmail.com> Co-authored-by: Ian Harvey <iharvey@comcast.net> Co-authored-by: Mike <mikehathaway@makerdao.com> * Sherlock 148: Use pool debt when calculating MOMP in Loans.update (#586) * Update comments: (#590) - ERC721.addCollateral changes bucketTokenIds - ERC721.repayDebt changes borrowerTokenIds and bucketTokenIds (when auction settled) - fix comment in Auctions._calculateTakeFlowsAndBondChange for take below NP * fix safeTransferFromWithPermit (#593) * Create standardized PR templates for contracts (#539) This PR creates three github PR templates that should be followed: * `logic_src_changes.md` * `cosmetic_src_changes.md` * `testing_or_misc_nonsrc_changes.md` To learn more about how GH templates work -> https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/creating-a-pull-request-template-for-your-repository Co-authored-by: Ian Harvey <iharvey@comcast.net> * Single template (#546) * merged all of the templates into one so the text will auto fill when a PR is created. * was running into this issue documented here with auto fill -> https://stackoverflow.com/questions/52139192/github-pull-requests-template-not-showing * fix safeTransferFromWithPermit --------- Co-authored-by: Ian Harvey <ith.harvey@gmail.com> Co-authored-by: Ian Harvey <iharvey@comcast.net> Co-authored-by: Mike <mikehathaway@makerdao.com> * LPS and Exchange Rate as WADs - Sherlock 133 & 83 (#606) * LPs as WADs * - record lender LPs in remove collateral and quote token only if bucket doesn't go bankrupt - for remove colalteral use the same way to calculate LPs as when rewarding, to avoid precision issues - tearDown should consider that bucket can go bankrupt, hence lender LPs should be checked = 0 taking this into account (lenderInfo function is doing this) - all tests passing but testDepositTwoActorSameBucket fails in tearDown - that's because there are quote tokens remaining without being backed by LPs (were previously redeemed in tearDown for collateral) * Use consistent way to calculate LPs reported to scaled amounts, remove unscaled rate exchange, fix tests Couple of take tests fail in tearDown leaving small dust deposit * Cleanup Maths library, update comments * Fix remaining tests * Fix sequence to empty buckets, one single settle test remaining failing in tearDown * Add bucket clean out sequence, make consistent for removeCollateral too (per Sherlock 133) * Add bankruptcy check for move quote token from bucket and unit test * Guard against underflows in removeQuoteToken (due to roundings when transforming from scaled to unscaled amount) * Changes after review: - calculate amount using collateralAmount_ rather than bucketCollateral - remove dead code in remove quote token * Sherlock 39: expiration timestamp and slippage control (#600) * ignore forge script tx broadcasts * provide LUP limit when pulling collateral * expiration checks for adding to buckets * unit tests for addQuote and addCollateral expiration * expiration checks for moveQuote * ensure borrower debt exceeds dust amount regardless of loan count * Revert "ensure borrower debt exceeds dust amount regardless of loan count" This reverts commit c0a64f9. * feedback from PR #600 (#604) * removed comment * Address review comments, update comment for _revertIfLupDroppedBelowL… (#607) * Address review comments, update comment for _revertIfLupDroppedBelowLimit, changed LimitIndexReached to LimitIndexExceeded * Remove lup Id variable, calculate LUP price directly * Remove unused struct member --------- Co-authored-by: grandizzy <38490174+grandizzy@users.noreply.github.com> * Fix test compile * Protocol invariants testing (#609) * Invariants * Exclude invariants from code coverage and GH action * Fix coverage GH target * Changes after review * Disallow auctioned borrowers to draw more debt or pull collateral if auction is not settled. (#611) This can happen in a scenario where user becomes recollateralized to the new LUP. Without these checks, and since borrow and pull doesn't settle auctions, borrower is not removed from auction queue but added in loan queue as well, which violate invariant - **A3**: number of borrowers with debt = number of loans + number of auctioned borrowers * Sherlock 73 (#591) * Create standardized PR templates for contracts (#539) This PR creates three github PR templates that should be followed: * `logic_src_changes.md` * `cosmetic_src_changes.md` * `testing_or_misc_nonsrc_changes.md` To learn more about how GH templates work -> https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/creating-a-pull-request-template-for-your-repository Co-authored-by: Ian Harvey <iharvey@comcast.net> * Single template (#546) * merged all of the templates into one so the text will auto fill when a PR is created. * was running into this issue documented here with auto fill -> https://stackoverflow.com/questions/52139192/github-pull-requests-template-not-showing * update getNFTSubsetHash to uniquely sort array of tokenIds * add natspec and slight gas optimization * change encodePacked to encode to add byte padding * pool deployer gas optimization * check sort order instead of sorting on chain * pr feedback to check for duplicate tokenIds --------- Co-authored-by: Ian Harvey <ith.harvey@gmail.com> Co-authored-by: Ian Harvey <iharvey@comcast.net> Co-authored-by: Mike <mikehathaway@makerdao.com> --------- Co-authored-by: Ed Noepel <46749157+EdNoepel@users.noreply.github.com> Co-authored-by: mattcushman <36414299+mattcushman@users.noreply.github.com> Co-authored-by: mwc <matt@ajna.finance> Co-authored-by: Ed Noepel <ed@lpl.io> Co-authored-by: Prateek Gupta <prateek105@users.noreply.github.com> Co-authored-by: prateek105 <prateek@ajna.finance> Co-authored-by: Ian Harvey <ith.harvey@gmail.com> Co-authored-by: Ian Harvey <iharvey@comcast.net> Co-authored-by: Mike Hathaway <mahathaway93@gmail.com> Co-authored-by: Mike <mikehathaway@makerdao.com> * ERC20Pool.removeCollateral should revert with InsufficientLPs error if the redeemed collateral amount is 0. * Add test to expose swap scenario with small amounts * Mul before div when calculating amount of collateral to remove * precalculate exchange rate logic added * return moved amount from moveQuoteToken * update reserve invariants, update liquidation handlers, fix local fenwick bugs, add new regression tests * Add unit test for failing regression sequence test_fenwick_bug_1 * change to repay debt with zero params from local fenwick for reserve and exchange rate precalculation * Brownie invariant tests sample (#548) * remove regression tests from make test command * add fenwick tree invariants F1, F2, F3, F4, auction invariant A6, bucket invariant B4, B5, Interest rate invariant I2 * Add settleAuction, kickWithDeposit, withdrawBonds, transferLps and approveLps handlers * Add useTimestamps modifier for handlers and useCurrentTimestamp modifier for invariant test to fix overflow/underflow issue * Fix: Local fenwick accure interest calculations, Add: Failing invariant sequences in regression test * Adapt to new develop branch * Use newly updateInterestRate function, catch InvalidAmounts, fix invariant_collateralBalance_CT1_CT7 (collateral amount is the 2nd in bucketInfo returned tuple) * Resolve 2 regression tests, new one to investigate * Use LPs before and after bucket take to check if kicker rewarded * Fix local fenwick accure interest bug by removing resetFenwickDepositUpdate method which is no longer needed * Fix: Deposit time updates only when non zero lps added * Added regression test for PR #674 * HPB bankruptcy on settle - if there's only a tiny amount of deposit (2) backed by 2 LPs in HPB then the collateralUsed to settle is calculated as 0 due to rounding (vars.collateralUsed = Maths.wdiv(vars.scaledDeposit, vars.price)) - this results in removing deposit (2) and not adding any collateral into bucket, leaving an amount of 2 LPs that is not backed by any asset - fixed by declaring bucket bankruptcy in case settle leaves bucket with 0 collateral, 0 deposit but LPs > 0 - added check for deposit to remove to be min of available deposit in bucket / calculated deposit for settle in order to prevent any potential underflow - unit test * Update addQuoteToken and moveQuoteToken handler to add depositfee if deposit is added/moved below lup * Bucket take with tiny deposit - if deposit in bucket cannot cover at least 1 unit of collateral (that is calculated amount to take is zero) then taker reward is zero and bucket is left with no collateral, no deposit and unbacked LPs - fix by reverting if bucket take on a bucket with not enough deposit to cover at least one unit of collateral - unit test * Invariants code style * Invariants code restructure (#679) * Invariants code restructure * Consistent Invariants naming * Introduce _auctionSettleStateReset to set alreadyTaken on false if auction settled * rename TestBase to InvariantsTestBase and InvariantsTestBase to InvariantsTestHelpers --------- Co-authored-by: prateek105 <prateek@ajna.finance> * Only update bucket taker deposit time when LPs reward not 0 * Charge fee also if depositing at lup * Fix: moveQuotetoken handler to account for deposit fee * Update: Lup precalculation in moveQuoteToken handler * Fix invariant test move quote token amounts / fee calculation * Fix move quote token handler, use returned value from pool function which already contains the amount with applied fee (if case) * Catch specific errors * Add helper to ensure pool error * Use _borrowFeeRate, _depositFeeRate functions, reserves should increase if add and move quote token below the LUP * Invariants take2 cleanup (#682) * Code reorg, fix kick, add utility functions * Remove storing current exchange rate and reserves, compare directly with pool values * Do not bound in unbounded handlers, accrue fenwick interest and record reserves and exchange rate in modifier * Remove should exchange rate, add mapping bucketIndex -> exchangeShouldNotChange. In rate invariant check buckets with exchangeShouldNotChange flag set on true and compare previousExchangeRate with currentExchangeRate Move more bounds from unbounded handler * Add more invariants, code comment * Fix tests, intorduce pre action phases and reuse them in tests * Comments * Use global bucket index, do not fuzzed in _pre functions * Call preDrawDebt before drawDebt in kickAuction * Workaround reserve invariant rounding of 1 unit of WAD * Accrue local fenwick interest in at the end of the test * Accrue local fenwick interest before updating pool state * Changes after review: - rename invariant_Bucket_deposit_time_B5 to invariant_Bucket_deposit_time_B5_B6_B7 - rename modifier resetAllPreviousLocalState to updateLocalStateAndPoolInterest - rename function _recordReservesAndExchangeRate to _resetAndRecordReservesAndExchangeRate * Remove amount used for kick with deposit from local Fenwick * Remove amount of quote tokens needed for bucket take * Add reserve invariant RE11 * Add reserve invariant RE12 * Fix: reserves calculation in _settleAuction handler * Update: Reserve change calculation in _settleAuction handler --------- Co-authored-by: Ian Harvey <ith.harvey@gmail.com> Co-authored-by: Ian Harvey <iharvey@comcast.net> Co-authored-by: Ed Noepel <46749157+EdNoepel@users.noreply.github.com> Co-authored-by: mattcushman <36414299+mattcushman@users.noreply.github.com> Co-authored-by: mwc <matt@ajna.finance> Co-authored-by: Ed Noepel <ed@lpl.io> Co-authored-by: Prateek Gupta <prateek105@users.noreply.github.com> Co-authored-by: prateek105 <prateek@ajna.finance> Co-authored-by: Mike Hathaway <mahathaway93@gmail.com> Co-authored-by: Mike <mikehathaway@makerdao.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of change
High level
mint()function sherlock-audit/2023-01-ajna-judging#131Description of bug or vulnerability and solution
The mint() function makes an external call to _safeMint() which contains a callback "_checkOnERC721Received(address(0), to, tokenId, data)", to the "to" address. Mint() function has no reentrancy guard to protect against reentrancy attacks A malicious user can reenter the mint() function after the initial minting. https://twitter.com/Sm4rty_/status/1615665389635002372
Gas usage
Pre Change
Post Change