Use unscaled remaining amount when remove deposit

[ith-harvey]

Description of change

High level

• Use unscaled remaining amount to determine if the entire deposit was removed
  • Due to rounding, while removing quote token, there are cases when scaled remaining amount is calculated as 1 unit of WAD while the entire unscaled deposit from index was removed. This can lead to situations where bucket is not marked as bankrupt (no collateral, calculated amount of scaled collateral remaining 1, LPs in bucket).
  • Use remaining unscaled amount instead as that shows if the entire deposit was removed from index (colalteral = 0, remaining unscaled amount = 0, LPs > 0 then bucket will be bankrupt and remaining LPs forfeited)

Description of bug or vulnerability and solution

• Use unscaled remaining amount to determine if the entire deposit was removed
• from LenderActions._removeMaxDeposit return unscaledRemaining_ = unscaledDepositAvailable - unscaledRemovedAmount; instead scaledRemaining_ = scaledDepositAvailable - removedAmount_;

Contract size

Pre Change

LenderActions            -  12,403B  (50.47%)

Post Change

LenderActions            -  12,403B  (50.47%)

Gas usage

Pre Change

| Function Name                        | min             | avg    | median | max     | # calls |
| addQuoteToken                        | 119315          | 176540 | 161115 | 707453  | 60004   |
| bucketTake                           | 323735          | 344533 | 344473 | 365451  | 4       |
| drawDebt                             | 248277          | 293711 | 268224 | 798667  | 136003  |
| kick                                 | 196115          | 222377 | 221402 | 1031167 | 48000   |
| kickWithDeposit                      | 244973          | 279516 | 274317 | 996221  | 8000    |
| moveQuoteToken                       | 270679          | 270679 | 270679 | 270679  | 1       |
| removeQuoteToken                     | 145480          | 167395 | 171694 | 194228  | 4000    |
| repayDebt                            | 569765          | 601908 | 591804 | 750075  | 16002   |
| settle                               | 352711          | 352711 | 352711 | 352711  | 1       |
| take                                 | 97715           | 102844 | 102501 | 326762  | 15998   |

Post Change

| Function Name                        | min             | avg    | median | max     | # calls |
| addQuoteToken                        | 119315          | 176540 | 161115 | 707453  | 60004   |
| bucketTake                           | 323735          | 344533 | 344473 | 365451  | 4       |
| drawDebt                             | 248277          | 293711 | 268224 | 798667  | 136003  |
| kick                                 | 196115          | 222377 | 221402 | 1031167 | 48000   |
| kickWithDeposit                      | 244973          | 279516 | 274317 | 996221  | 8000    |
| moveQuoteToken                       | 258130          | 258130 | 258130 | 258130  | 1       |
| removeQuoteToken                     | 145480          | 167391 | 167931 | 191765  | 4000    |
| repayDebt                            | 569765          | 601908 | 591804 | 750075  | 16002   |
| settle                               | 352711          | 352711 | 352711 | 352711  | 1       |
| take                                 | 97715           | 102844 | 102501 | 326762  | 15998   |

[ith-harvey]

Since the code change in the PR makes this regression pass I suggest we remove the regression test.

[grandizzy]

Since the code change in the PR makes this regression pass I suggest we remove the regression test.

We should keep regression test too, no harm in having it

[ith-harvey]

PR needs comments

[EdNoepel]

_removeMaxDeposit is called from two places:

• moveQuoteToken, which caches it as vars.fromBucketRemainingDeposit
• removeQuoteToken, which was updated to rename the variable

Both methods perform a zero check for the purposes of inducing a bucket bankruptcy if bucket LPs != 0 yet there is no remaining deposit. This fix seems appropriate for both cases.

Note there is merely one call to moveQuoteToken in the gas reports, but I am not concerned about gas usage for this change.

LGTM.

[prateek105]

lgtm.

[mattcushman]

lgtm