Enable async withdrawals on superOETHb

[shahthepro]

Changes Overview

• Removes CLAIM_DELAY constant in favour of a withdrawalClaimDelay variable
• withdrawalClaimDelay has an hardcoded limit of 10m to 7d (to avoid accidentally or intentionally setting a huge withdrawal claim period)
• withdrawalClaimDelay can be set to zero to disable async withdrawals. This will prevent users from making any new withdrawal requests. However, they'd be able to claim any pending withdrawals instantly after disabling it

Code Change Checklist

To be completed before internal review begins:

• The contract code is complete
• Executable deployment file
• Fork tests that test after the deployment file runs
• Unit tests *if needed
• The owner has done a full checklist review of the code + tests

Internal review:

• Two approvals by internal reviewers

[github-actions]

	Warnings
⚠️	👀 This PR needs at least 2 reviewers

Generated by 🚫 dangerJS against b3e4d1d

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 53.47%. Comparing base (95ece09) to head (b3e4d1d).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2275      +/-   ##
==========================================
+ Coverage   53.26%   53.47%   +0.20%     
==========================================
  Files          79       79              
  Lines        4089     4090       +1     
  Branches     1074     1079       +5     
==========================================
+ Hits         2178     2187       +9     
+ Misses       1908     1900       -8     
  Partials        3        3              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sparrowDom]

Left a couple of comments inline. Looks great 👍

[naddison36]

LGTM

[DanielVF]

I think we will want to eventualy change the request.timestamp to become an end stamp rather than a start time stamp.

The current code has three awkward things:

• If we change the claim duration, it retroactively changes the end date of inflight requests.
• If we disable async claims by setting it to zero, it actually allows instant claims of all inflight claims.
• If someone wrapped the governance execute() of a zero set, then they could flash loan a mint and a redeem in the same block.

However, our current upgrade on OETH will be atomically setting a value for this in the upgrade, and we don't have to use the zero value for now.

Given that the current zero value is dangerous, how about we also have the require nonzero in the claim? In this way we never accidentally give someone the ability to flashloan an 1:1 mint/redeem. We could then remove it at some future date once we've changed the code to make it safe.

[shahthepro]

I looked at changing request.timestamp to the end time. Looks like there's no space in the WithdrawRequest to fit in the same slot now. If we reuse the existing var on that struct, someone can still wrap our governance execute() and do a flash loan attack. So, yeah, leaving that aside for sometime later makes sense. When we do that change, perhaps we could consider disabling the async withdrawals first (go through governance and execute it) and then doing the upgrade would help avoid the flash loan attacks.

For now, I have changed _claimWithdrawal to revert when it's disabled as well.

However, our current upgrade on OETH will be atomically setting a value for this in the upgrade, and we don't have to use the zero value for now.

This may not be true. We have a deployment file for Base. Not for mainnet. I have excluded that file intentionally since we have other changes to the Vault happening soon (rebasification). I can set a reminder to update the deployment file in those PRs to reset the withdrawalClaimDelay to 10 minutes (current value)

[DanielVF]

Agree on disabling withdraws right before the upgrade.