Platform: Code4rena
Start Date: 16/12/2022
Pot Size: $60,500 USDC
Total HM: 12
Participants: 58
Period: 5 days
Judge: Trust
Total Solo HM: 4
Id: 196
League: ETH
Rank: 40/58
Findings: 1
Award: $43.54
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: yixxas
Also found by: 0x52, 0xAgro, 0xSmartContract, 0xhacksmithh, Aymen0909, Bnke0x0, Bobface, Breeje, Diana, Franfran, HE1M, HollaDieWaldfee, IllIllI, Jeiwan, RaymondFam, Rolezn, SaharDevep, Secureverse, SmartSek, ak1, bin2chen, brgltd, chrisdior4, gz627, imare, ladboy233, lukris02, oyc_109, rvierdiiev, shark, tnevler, unforgiven, wait
43.5439 USDC - $43.54
Borrowers may be liquidated more assets than needed. purchaseLiquidationAuctionNFT may perform incorrectly.
Because the collateral count is reduced in startLiquidationAuction @ PaprController.sol#L326, the total value and the _maxDebt() of the vault is changed.
This is not correct. The collateral being liquidating should count towards the value of the vault's totalCollateraValue.
For example, maxLTV is 50%, a vault has 10 NFT collaterals (worth 200 papr now) and has a debt of 101 papr, it is liquidatable now. If the borrower add 1 more collateral to the vault, the vault will be safe (un-liquidatable) again. But, if someone calls PaprController.sol#startLiquidationAuction first, at this point, if the borrower adds 1 more collateral to the vault, the vault will still be liquidatable.
In extreme cases this can lead to a loop:
a vault become liquidatable -> someone calls startLiquidationAuction() -> borrower add 1 collateral -> someone calls startLiquidationAuction() -> borrower add 1 collateral -> someone calls startLiquidationAuction() -> ...
In general, the borrower is vulnerable to liquidating more collaterals due to the LTV become lower.
The problem may not show up if the previous liquidation auction is always purchased in time, and can be mitigated by using the liquidationAuctionMinSpacing to limit aution intervals. But the implementation itself is logically incorrect, these mitigation don't really solve it.
In addition, it could cause the isLastCollateral value of purchaseLiquidationAuctionNFT to be incorrect.
When all (multiple) collaterals of a vault have been started the liquidating auctions, because the count is subtracted advanced, the count of the vault is 0 now.
Then all these liquidating collaterals will be treated as the last collateral in purchaseLiquidationAuctionNFT.
As a result, the execution of x will make errors, including fund related errors.
Manual
auctionCount for each vault, increase it when startLiquidationAuction(), reduce it after purchaseLiquidationAuctionNFT().auctionCount when calculating the total value or _maxDebt() of the vault.auctionCount is not 0, removeCollateral, increaseDebt and increaseDebtAndSell are prohibited.#0 - c4-judge
2022-12-25T16:56:31Z
trust1995 marked the issue as duplicate of #186
#1 - c4-judge
2022-12-25T16:56:35Z
trust1995 marked the issue as satisfactory
#2 - c4-judge
2023-01-04T12:05:07Z
trust1995 changed the severity to 2 (Med Risk)
#3 - c4-judge
2023-01-07T17:01:51Z
trust1995 changed the severity to QA (Quality Assurance)
#4 - Simon-Busch
2023-01-09T14:57:00Z
Remove duplicate-186 label as requested by @trust1995
#5 - Simon-Busch
2023-01-09T15:04:55Z
Removed the satisfactory label as well and add grade-b label as requested by @trust1995
#6 - wilsoncusack
2023-01-18T15:20:14Z
Dup to #185. We want to remove the NFT at the start of the auction otherwise the borrower could pay down debt, withdraw the NFT, and break the auction logic.
#7 - trust1995
2023-01-18T16:37:32Z
It is treated as such. Since #186 was downgraded to QA all finds have been dupped to the warden's QA issue.