Bug (prop 2): rounding invariant failures in `kickWithDeposit` and `removeQuoteToken`

[grandizzy]

Description of change

High level

• Kick with deposit reverts due to rounding: calculated unscaled amount to remove from deposit (for amount to debit from deposit to cover bond) could be bigger than actual unscaled amount of deposit (since ceilWdiv is used). This results in tx reverting and kick with deposit failure
  • Fixed by using floorWmul to round down scaled amount
  • Invariants changes:
  • set F1 and F2 deviation back to 1e12 (were changed to quote token scale but this lowers deviation to 1 in case of 18 decimals)
  • use ceilMul when computing amount for F3 (since Fenwick change to allow higher debt but lower precision https://github.com/ajna-finance/contracts/pull/857)
  • bump reserves invariant deviation to 1e13, inline with QT1 invariant deviation

Contract size

Pre Change

  TakerActions             -  15,104B  (61.46%)
  LenderActions            -  10,581B  (43.05%)
  KickerActions            -  10,435B  (42.46%)

Post Change

  TakerActions             -  15,129B  (61.56%)
  LenderActions            -  10,593B  (43.10%)
  KickerActions            -  10,427B  (42.43%)

Gas usage

Pre Change

| Function Name                        | min             | avg    | median | max     | # calls |
| addQuoteToken                        | 122500          | 179775 | 164363 | 817387  | 60004   |
| bucketTake                           | 309224          | 327865 | 327866 | 346507  | 4       |
| drawDebt                             | 253215          | 298204 | 272111 | 812414  | 136003  |
| kick                                 | 149467          | 223488 | 221980 | 1034805 | 48000   |
| kickWithDeposit                      | 199746          | 277377 | 273523 | 1000731 | 8000    |
| moveQuoteToken                       | 290958          | 290958 | 290958 | 290958  | 1       |
| removeQuoteToken                     | 147296          | 171440 | 176084 | 195785  | 4000    |
| repayDebt                            | 576921          | 609077 | 598969 | 757339  | 16002   |
| settle                               | 350544          | 350544 | 350544 | 350544  | 1       |
| take                                 | 81772           | 83349  | 83084  | 315391  | 15998   |

Post Change

| Function Name                        | min             | avg    | median | max     | # calls |
| addQuoteToken                        | 122497          | 179770 | 164360 | 715160  | 60004   |
| bucketTake                           | 309308          | 327949 | 327950 | 346591  | 4       |
| drawDebt                             | 253002          | 297991 | 271898 | 812201  | 136003  
| kick                                 | 149467          | 223488 | 221980 | 1034805 | 48000   |
| kickWithDeposit                      | 199674          | 277305 | 273451 | 1000659 | 8000    |
| moveQuoteToken                       | 258898          | 258898 | 258898 | 258898  | 1       |
| removeQuoteToken                     | 147373          | 171515 | 176161 | 195862  | 4000    |
| repayDebt                            | 576921          | 609077 | 598969 | 757339  | 16002   |
| settle                               | 350544          | 350544 | 350544 | 350544  | 1       |
| take                                 | 81772           | 83349  | 83084  | 315391  | 15998   |

[grandizzy]

Regarding kickWithDeposit: As much as I like not having to use a Maths.min to prevent underflow, I am slightly concerned that kickWithDeposit does not reuse LenderActions logic for removing quote token. Seems it would be much cleaner if more of the math for redeeming LP for quote token was shared with removeQuoteToken.

that's a good point, I agree that would be the best approach. I quickly looked to see what would imply reusing LenderActions._removeMaxDeposit and there are quite some changes to be done, I'd like to explore this / test thoroughly in a subsequent PR