feat: L3 withdrawal helpers #605

bmzig · 2024-09-13T18:57:23Z

There will be shared logic between L2 token retrievers which need to bridge funds from L3 back to L1 and L2 spoke pool logic. We may be able to factor this out for each chain and create another adapter-like contract which can be delegatecall-ed by both L2 token retriever and spoke pool contracts.

…ract Signed-off-by: bennett <[email protected]>

Signed-off-by: bennett <[email protected]>

contracts/Intermediate_TokenRetriever.sol

Signed-off-by: bennett <[email protected]>

contracts/L2_TokenRetriever.sol

contracts/chain-adapters/l2/WithdrawalAdapter.sol

Signed-off-by: bennett <[email protected]>

mrice32

Looks great -- just a few comments

contracts/L2_TokenRetriever.sol

mrice32 · 2024-09-19T17:42:07Z

contracts/chain-adapters/l2/Arbitrum_WithdrawalAdapter.sol

+        } else {
+            // Check that the Ethereum counterpart of the L2 token is stored on this contract.
+            // Tokens will only be bridged if they are whitelisted by the spoke pool.
+            address ethereumTokenToBridge = spokePool.whitelistedTokens(l2TokenAddress);


I don't love the dependency on the spoke pool, especially since we may end up wanting the spoke pool to call into this contract.

But I can't really think of a way to get this list elsewhere without adding a lot of complexity or additional args that wouldn't apply on non-arbitrum chains... maybe we could add a bytes arg that can be used to supply extra context when needed, but that makes the interface sort of convoluted.

Any ideas?

@mrice32 The update for the Arbitrum withdrawal adapter seems quite clean, but the issue remains in the Optimism one. It seems tricky to avoid in the short term without introducing ownership to additional contracts, which adds complexity.

Do you think it could make sense for the adapter to have a public function that allows it to import state from the SpokePool? This is a bit clunky and a little bit more complex, but would at least avoid the adapter calling into the SpokePool as part of a withdrawl.

The public function would be on the spoke pool? Is that different than what's happening now?

Maybe I'm missing the point.

We use a predeploy for l2Eth and the wrapped native token is immutable in the spoke pool, but I did make it so that we query l1Gas every time in case that does end up changing in the future: 4db2572

Signed-off-by: bennett <[email protected]>

bmzig · 2024-10-02T19:44:22Z

WithdrawalAdapter name is TBD

nicholaspai

Couple first pass comments

nicholaspai · 2024-10-02T20:23:12Z

contracts/chain-adapters/l2/WithdrawalAdapterBase.sol

+ * token mapping is correct so that the bridge call itself can succeed.
+ */
+abstract contract WithdrawalAdapterBase is CircleCCTPAdapter, MultiCaller {
+    using SafeERC20 for IERC20;


Prefer naming this contract WithdrawHelper or L3WithdrawHelper as we've typically used Adapter to contain code that gets delegate called and otherwise acts as a library

nicholaspai · 2024-10-02T20:25:25Z

contracts/chain-adapters/l2/Ovm_WithdrawalAdapter.sol

+ * which has a special SNX bridge (and thus this adapter will NOT work for Optimism).
+ * @custom:security-contact [email protected]
+ */
+contract Ovm_WithdrawalAdapter is WithdrawalAdapterBase {


Should this be upgradeable? If not then is the process for upgrading this and then updating the withdrawTo address in the L3 spoke pool easy?

After discussing w/ @bmzig:

We're probably OK w/o upgradability here. To change this implementation we'd need to deploy a new instance and call setHubPool() on the L3 Spoke, which should be fine.

We are probably lacking a way to rescue any tokens from this contract in case of an implementation problem in withdrawToken(). We probably want to support an L1 -> L2 message initiated by the HubPool that instructs this contract to withdraw tokens to an L2 address. We could use this to withdraw tokens to the relevant SpokePool on the same chain and evacuate the tokens via there.

This, along with a basic unit test, was added here da1bc9a

nicholaspai · 2024-10-03T12:59:03Z

WithdrawalAdapter name is TBD

Next time don't open PR for review until names are ready to be merged, otherwise just leave in draft mode

pxrl

One tiny comment but otherwise no blockers from me.

contracts/chain-adapters/l2/Arbitrum_WithdrawalAdapter.sol

Signed-off-by: bennett <[email protected]>

pxrl

One fairly minor comment, otherwise looking good.

pxrl · 2024-10-04T12:46:26Z

contracts/chain-adapters/l2/WithdrawalHelperBase.sol

@@ -35,10 +44,28 @@ abstract contract WithdrawalAdapterBase is CircleCCTPAdapter, MultiCaller {
        ITokenMessenger _cctpTokenMessenger,
        uint32 _destinationCircleDomainId,
        address _l2TokenGateway,
-        address _tokenRecipient
+        address _tokenRecipient,
+        address _hubPool


I think this needs a corresponding param update in the natspec above. Also on the naming, I think the SpokePool contracts typically use crossDomainAdmin to specify the owner/admin and hubPool to specify the recipient on mainnet for withdrawals, so this seems a bit inconsistent with that in the sense that hubPool has permission to execute admin functions here.

Good catch. I added the natspec to reflect the new parameter. 2056784. Also, I agree with renaming hubPool, so I just changed the functions to match the same ones as the spoke pool (i.e. use crossDomainAdmin).

pxrl · 2024-10-04T12:56:16Z

contracts/chain-adapters/l2/WithdrawalHelperBase.sol

+        address l2Token,
+        address to,
+        uint256 amount
+    ) external onlyHubPool {


I'm unsure, but does external make sense when this would be called from another contract via a delegatecall from this contract? It's a little bit inception-y so I'm not immediately sure. @bmzig do you know?

I just ran a quick test to verify that we can delegatecall these, and it looks to be alright.

Signed-off-by: bennett <[email protected]>

contracts/chain-adapters/l2/Arbitrum_WithdrawalHelper.sol

contracts/chain-adapters/l2/WithdrawalHelperBase.sol

Signed-off-by: bennett <[email protected]>

mrice32

LGTM!

refactor: export _bridgeTokensToHubPool for Arbitrum to separate cont…

757e802

…ract Signed-off-by: bennett <[email protected]>

bmzig requested review from nicholaspai, mrice32 and pxrl September 13, 2024 18:57

bmzig added 2 commits September 13, 2024 14:51

feat: add Intermediate_TokenRetriever contract for L3-L1 withdrawals

1a49346

Signed-off-by: bennett <[email protected]>

generalize bridge adapter contract

ac13aac

Signed-off-by: bennett <[email protected]>

bmzig commented Sep 16, 2024

View reviewed changes

contracts/Intermediate_TokenRetriever.sol Outdated Show resolved Hide resolved

bmzig added 2 commits September 16, 2024 11:26

add functionality for retrieveMany. Refactor WithdrawalAdapter

3131eed

Signed-off-by: bennett <[email protected]>

feat: add L2 forwarder interface

f9c0d62

Signed-off-by: bennett <[email protected]>

pxrl reviewed Sep 17, 2024

View reviewed changes

contracts/L2_TokenRetriever.sol Outdated Show resolved Hide resolved

contracts/L2_TokenRetriever.sol Outdated Show resolved Hide resolved

contracts/chain-adapters/l2/WithdrawalAdapter.sol Outdated Show resolved Hide resolved

bmzig changed the title ~~[WIP] refactor: export bridging logic to l2 adapter contracts~~ feat: add an L2 token retriever contract which calls exported spoke pool functions Sep 17, 2024

bmzig added 6 commits September 17, 2024 15:01

Merge branch 'master' into bz/withdrawalAdapter

f6b2b30

Signed-off-by: bennett <[email protected]>

apply suggested changes

d3f624b

Signed-off-by: bennett <[email protected]>

sync with upstream changes

cea0337

Signed-off-by: bennett <[email protected]>

explicit imports

0f68634

Signed-off-by: bennett <[email protected]>

add a withdrawal adapter for OVM chains

a0a371d

Signed-off-by: bennett <[email protected]>

refactor and rename

0de9303

Signed-off-by: bennett <[email protected]>

mrice32 reviewed Sep 19, 2024

View reviewed changes

bmzig marked this pull request as ready for review September 20, 2024 16:53

bmzig added 2 commits September 20, 2024 11:57

Merge branch 'master' into bz/l2ForwarderInterface

0371348

Signed-off-by: bennett <[email protected]>

WIP remove spoke pool dependency

942f741

Signed-off-by: bennett <[email protected]>

bmzig changed the base branch from master to bz/ArbitrumForwarders September 23, 2024 13:50

bmzig changed the base branch from bz/ArbitrumForwarders to bz/l2ForwarderInterface September 23, 2024 14:57

bmzig added 7 commits September 23, 2024 09:58

Merge branch 'bz/l2ForwarderInterface' into bz/withdrawalAdapter

3cbf3dd

clean up interfaces

fd94012

Signed-off-by: bennett <[email protected]>

Merge branch 'bz/l2ForwarderInterface' into bz/withdrawalAdapter

b95cb01

arbitrum withdrawal adapter refactoring

4c41098

Signed-off-by: bennett <[email protected]>

simplify forwarder base and make it a proxy

f2e3c41

Signed-off-by: bennett <[email protected]>

disable implementation initialization

f251354

Signed-off-by: bennett <[email protected]>

Merge branch 'bz/l2ForwarderInterface' into bz/withdrawalAdapter

050c784

bmzig added 2 commits October 2, 2024 08:56

Merge branch 'master' into bz/withdrawalAdapter

d1a6143

Signed-off-by: bennett <[email protected]>

refactor of withdrawal adapter format

4db2572

Signed-off-by: bennett <[email protected]>

bmzig marked this pull request as ready for review October 2, 2024 18:29

remove unused test imports

db64f5f

Signed-off-by: bennett <[email protected]>

nicholaspai reviewed Oct 2, 2024

View reviewed changes

pxrl reviewed Oct 3, 2024

View reviewed changes

contracts/chain-adapters/l2/Arbitrum_WithdrawalAdapter.sol Outdated Show resolved Hide resolved

bmzig added 3 commits October 3, 2024 12:26

Merge branch 'master' into bz/withdrawalAdapter

6dc1482

Merge branch 'master' into bz/withdrawalAdapter

1882c5f

rename and add test

da1bc9a

Signed-off-by: bennett <[email protected]>

bmzig changed the title ~~feat: add an L2 token retriever contract which calls exported spoke pool functions~~ feat: L3 withdrawal helpers Oct 3, 2024

pxrl reviewed Oct 4, 2024

View reviewed changes

add natspec comment and change name to crossDomainAdmin

2056784

Signed-off-by: bennett <[email protected]>

bmzig requested review from mrice32 and pxrl October 4, 2024 13:20

bmzig added 3 commits October 4, 2024 08:50

add vm.assume to test to address fuzz where l1Token = invalidToken

450cfe5

Signed-off-by: bennett <[email protected]>

make the helper into a proxy

bcaa0f4

Signed-off-by: bennett <[email protected]>

move init functions to chain-specific adapters

96a4e1b

Signed-off-by: bennett <[email protected]>

pxrl reviewed Oct 4, 2024

View reviewed changes

contracts/chain-adapters/l2/Arbitrum_WithdrawalHelper.sol Outdated Show resolved Hide resolved

contracts/chain-adapters/l2/WithdrawalHelperBase.sol Outdated Show resolved Hide resolved

address comments

167b8a9

Signed-off-by: bennett <[email protected]>

bmzig requested a review from pxrl October 4, 2024 20:42

inherit errors

3470649

Signed-off-by: bennett <[email protected]>

pxrl approved these changes Oct 4, 2024

View reviewed changes

bmzig added 2 commits October 7, 2024 08:38

change _minGasLimit from a u256 to a u32

47c8537

Signed-off-by: bennett <[email protected]>

update test interfaces

5ab22c3

Signed-off-by: bennett <[email protected]>

mrice32 approved these changes Oct 7, 2024

View reviewed changes

bmzig merged commit abec4e5 into master Oct 7, 2024
9 checks passed

bmzig deleted the bz/withdrawalAdapter branch October 7, 2024 14:13

feat: L3 withdrawal helpers #605

feat: L3 withdrawal helpers #605

Uh oh!

Conversation

bmzig commented Sep 13, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmzig commented Oct 2, 2024

Uh oh!

nicholaspai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicholaspai commented Oct 3, 2024

Uh oh!

pxrl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pxrl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!