improve(SpokePool): Prevent future deposit quoteTimestamps

[pxrl]

improve(SpokePool): Prevent future deposit quoteTimestamps

Future quoteTimestamps create unusual challenges for the bots that
process Deposit events. Fundamentally, it's impossible to compute the
correct realizedLpFeePct for a deposit with a future quoteTimestamp, so
all implementations must correctly park these early deposits and pull
them out at the right time. Given that we are now seeing diversity in
relayer implementations, it seems improbable that all will implement
this correctly, and the downside is that they will lose funds for a
slight mistake here.

From the depositor side, for the most part it is hard to justify
deliberately setting a quoteTimestamp that is in the future.

The scenarios where this might impact users is:

• A chain lags slightly behind the current time, meaning that deposits
  will fail due to very small time deltas.
• A deposit from a multisig normally wants the widest possible time
  range to collect the necessary signatures, and is technically losing
  half of that window.

For the former, this could be managed by the API, which supplies the
suggested quoteTimestamp to the depositor. In cases where the chain has
stalled, the API could simply throw a descriptive error.

For the latter, this could be mitigated by doubling the current deposit
window.

References ACX-1533

[linear]

ACX-1533 Revise future quoteTimestamp fix

The change implemented by Matt was a quick fix to resolve an immediate issue, but we've agreed to circle back to it and make some updates so it fits cohesively into the code.

[james-a-morris]

The getCurrentTime() should not change between these two calls. Would storing them in a local variable make sense so we don't have to call getCurrentTime() twice?

[pxrl]

I'd understood that it was counter-intuitively cheaper to call getCurrentTime() twice than to store it in a local variable, but I have now run the gas analytics over the two options and it doesn't show any difference for me. Per your comment below, we should however be OK to remove the 2nd call to getCurrentTime() and just compute that the delta between the current time and quoteTimestamp is less than the configured buffer.

[james-a-morris]

Potential Refactor:

Suggested change

	getCurrentTime() >= quoteTimestamp && getCurrentTime() <= quoteTimestamp + depositQuoteTimeBuffer,
	quoteTimestamp - getCurrentTime() <= depositQuoteTimeBuffer

Reasoning:
The original expression is really looking to check if the quote timestamp exists in the window between [c_t, c_(t+b)], or visually:

---------------- c ------------------
       [                    ]
      ct                ct+b

c = quote timestamp
ct = current time
b = deposit time buffer

If we apply algebra, we can simplify to:

c_t <= c <= c_t + b
0 <= c - c_t <= b

Because getCurrentTime() and quoteTimestamp() are both unsigned ints, I believe any case where c - c_t where the answer should be negative will result in an underflow and thus make the resulting value incredibly large.

As a result, we can leverage this underflow effectively to ignore the lower comparison and we are left with the revised expression.

cc @nicholaspai

[pxrl]

The buffer is backwards-looking rather than forwards-looking, so quoteTimestamp must always be less than getCurrentTime() and we're actually verifying that ct-b <= c <= ct. But adjusting the change to getCurrentTime() - quoteTimestamp <= depositQuoteTimeBuffer works and avoids the 2nd call to getCurrentTime(). This saves almost 100 gas on a deposit 👍

nb. The change to the comment above was also incorrect; there was no more possibility of underflow, but if we apply the above change there will be once more. I think that's probably OK, since we only expect advanced users to end up submitting future timestamps, so they should also be advanced enough to identify where it reverts.

[nicholaspai]

One comment change but LGTM