The count should be reduced in PaprController.sol#purchaseLiquidationAuctionNFT instead of in PaprController.sol#startLiquidationAuction

[code423n4]

Lines of code

https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L326

Vulnerability details

Impact

Borrowers may be liquidated more assets than needed.
purchaseLiquidationAuctionNFT may perform incorrectly.

Proof of Concept

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.

Tools Used

Manual

Recommended Mitigation Steps

1. Add a mapping to record auctionCount for each vault, increase it when startLiquidationAuction(), reduce it after purchaseLiquidationAuctionNFT().
2. Include the auctionCount when calculating the total value or _maxDebt() of the vault.
3. If auctionCount is not 0, removeCollateral, increaseDebt and increaseDebtAndSell are prohibited.

[c4-judge]

trust1995 marked the issue as duplicate of #186

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-judge]

trust1995 changed the severity to 2 (Med Risk)

[c4-judge]

trust1995 changed the severity to QA (Quality Assurance)

[Simon-Busch]

Remove duplicate-186 label as requested by @trust1995

[Simon-Busch]

Removed the satisfactory label as well and add grade-b label as requested by @trust1995

[wilsoncusack]

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.

[trust1995]

It is treated as such. Since #186 was downgraded to QA all finds have been dupped to the warden's QA issue.