Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
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 <[email protected]> Co-authored-by: Ed Noepel <[email protected]> * removed two temp tests * ported Matt's changes to the fuzzed implementation Co-authored-by: mattcushman <[email protected]> Co-authored-by: mwc <[email protected]> Co-authored-by: Ed Noepel <[email protected]> Co-authored-by: mattcushman <[email protected]> Co-authored-by: mwc <[email protected]> Co-authored-by: Ed Noepel <[email protected]> * Docs (#535) * Diagrams * Add components * Updates * Center components * add pool architectire diagrams (#536) Co-authored-by: prateek105 <[email protected]> Co-authored-by: Prateek Gupta <[email protected]> Co-authored-by: prateek105 <[email protected]> * 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 <[email protected]> Co-authored-by: mattcushman <[email protected]> Co-authored-by: mwc <[email protected]> Co-authored-by: Ed Noepel <[email protected]> Co-authored-by: Prateek Gupta <[email protected]> Co-authored-by: prateek105 <[email protected]> Co-authored-by: Ian Harvey <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> Co-authored-by: Ian Harvey <[email protected]> * 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 <[email protected]> * 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 <[email protected]> Co-authored-by: Ian Harvey <[email protected]> Co-authored-by: Mike <[email protected]> * 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 <[email protected]> * 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 <[email protected]> Co-authored-by: Ian Harvey <[email protected]> Co-authored-by: Mike <[email protected]> * 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 <[email protected]> * 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 <[email protected]> Co-authored-by: Ian Harvey <[email protected]> Co-authored-by: Mike <[email protected]> * 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 <[email protected]> * 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 <[email protected]> Co-authored-by: Ian Harvey <[email protected]> Co-authored-by: Mike <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> Co-authored-by: Ian Harvey <[email protected]> Co-authored-by: Mike <[email protected]> --------- Co-authored-by: Ed Noepel <[email protected]> Co-authored-by: mattcushman <[email protected]> Co-authored-by: mwc <[email protected]> Co-authored-by: Ed Noepel <[email protected]> Co-authored-by: Prateek Gupta <[email protected]> Co-authored-by: prateek105 <[email protected]> Co-authored-by: Ian Harvey <[email protected]> Co-authored-by: Ian Harvey <[email protected]> Co-authored-by: Mike Hathaway <[email protected]> Co-authored-by: Mike <[email protected]>
- Loading branch information
11 people
authored
Feb 13, 2023
1 parent
f5ace44
commit ec79122