TOB-AJNA-2: global scalar (at index 8192) is never updated

[grandizzy]

Description of change

High level

• Do not load scaling value in Deposits.treeSum
  • save an SLOAD in Deposits.treeSum as scalar is never updated (always 1)
  • add F5 Fenwick invariant to make sure global scalar is never updated. new Pool.depositScale external function added to support such

Description of issue from ToB report:

• The findIndexAndSumOfSums method ignores the global scalar of the scaled Fenwick tree
  while calculating the smallest index at which the prefix sum is at least the given target
  value.
• In a scaled Fenwick tree, values at power-of-two indices contain the prefix sum of the entire
  underlying array up to that index. Similarly, scalars at power-of-two indices contain a
  scaling factor by which all lower-index values should be multiplied to get the correct
  underlying values and prefix sums.
• The findIndexAndSumOfSums method performs a binary search starting from the middle
  power-of-two index at 4096 (2^12). If the prefix sum up to that point is too small, the
  algorithm checks higher indices and vice versa. But the global scalar at index 8192 (2^13) is
  not visited by this method and its value is never considered. If the global scalar contains a
  non-default value, then the indices and sums returned by findIndexAndSumOfSums will
  be incorrect.

ToB reponse to us given we didn't resolve the issue:

leaving this issue unresolved introduces an informal invariant that says “the global scalar is never updated” and if this invariant does not hold, the findIndexAndSumOfSums method will break as described by this issue. Consider making this invariant explicit via tests & code comments and revisit existing Deposit methods with this new invariant in mind. For example, it appears that an SLOAD in the treeSum method could be optimized out

Gas usage

Pre Change

| Function Name                        | min             | avg    | median | max     | # calls |
| addQuoteToken                        | 121399          | 178656 | 163199 | 710556  | 60004   |
| bucketTake                           | 327701          | 348523 | 348463 | 369465  | 4       |
| drawDebt                             | 252138          | 297491 | 271989 | 802390  | 136003  |
| kick                                 | 198074          | 224339 | 223376 | 1034990 | 48000   |
| kickWithDeposit                      | 245496          | 280027 | 274810 | 1001127 | 8000    |
| moveQuoteToken                       | 272846          | 272846 | 272846 | 272846  | 1       |
| removeQuoteToken                     | 147439          | 169353 | 173863 | 196457  | 4000    |
| repayDebt                            | 572876          | 605020 | 594915 | 753186  | 16002   |
| settle                               | 356150          | 356150 | 356150 | 356150  | 1       |
| take                                 | 98513           | 103445 | 103127 | 330130  | 15998   |

  
| Function Name                        | min             | avg    | median | max     | # calls |
| treeSum                              | 566             | 2965      | 4566      | 4566      | 36942   |

Post Change

| Function Name                        | min             | avg    | median | max     | # calls |
| addQuoteToken                        | 121212          | 178481 | 163012 | 710685  | 60004   |
| bucketTake                           | 327701          | 348523 | 348463 | 369465  | 4       |
| drawDebt                             | 252138          | 297491 | 271989 | 802390  | 136003  |
| kick                                 | 198074          | 224339 | 223376 | 1034990 | 48000   |
| kickWithDeposit                      | 245496          | 280027 | 274810 | 1001127 | 8000    |
| moveQuoteToken                       | 282469          | 282469 | 282469 | 282469  | 1       |
| removeQuoteToken                     | 147251          | 169182 | 173642 | 196203  | 4000    |
| repayDebt                            | 572876          | 605019 | 594915 | 753186  | 16002   |
| settle                               | 356150          | 356150 | 356150 | 356150  | 1       |
| take                                 | 98513           | 103445 | 103127 | 330130  | 15998   |

| Function Name                        | min             | avg    | median | max     | # calls |
| treeSum                              | 349             | 1548     | 2349     | 2349      | 36942   |

Contract size

Pre Change

============ Deployment Bytecode Sizes ============
  ERC721Pool               -  24,507B  (99.72%)
  ERC20Pool                -  23,862B  (97.09%)

Post Change

============ Deployment Bytecode Sizes ============
  ERC721Pool               -  24,537B  (99.84%)
  ERC20Pool                -  23,892B  (97.21%)

[ith-harvey]

This change looks fine, although upon further reviewing their comment they suggest that we create an invariant to ensure this upholds rather than us just making the change.

invariant that says “the global scalar is never updated” ... Consider making this invariant explicit via tests & code comments and revisit existing Deposit methods with this new invariant in mind.

I suggest this PR also have the invariant check they recommend adding.

[grandizzy]

This change looks fine, although upon further reviewing their comment they suggest that we create an invariant to ensure this upholds rather than us just making the change.

invariant that says “the global scalar is never updated” ... Consider making this invariant explicit via tests & code comments and revisit existing Deposit methods with this new invariant in mind.

I suggest this PR also have the invariant check they recommend adding.

added with 1b21017

[prateek105]

LGTM! Invariant is a nice addition.

[ith-harvey]

Reviewed on a call with eng together, invariant check should always pass and is added as a good practice / air cover since the global scalar can't be updated in the code / doesn't have the functionality.

[mattcushman]

lgtm