Standard Deposit/Withdrawal Interface

[ben-chain]

Description

This PR adds a standard interface for the minimal pair of contracts required for deposit/withdrawals. If there's any wishlist functionality that could go in here, we can discuss, but I lean towards letting developers expand this out as needed if they desire bespoke functionality that would impose potential security risks. For example, considered some sort of depositAndCall, but I think the security implications of this mean we should just have people add it to theirs as needed.

Questions

• Anything else we might want here?

Metadata

Fixes

• Fixes roadmap 557

Contributing Agreement

• I have read and understood the Optimism Contributing Guide and Code of Conduct and am following those guidelines in this pull request.

[K-Ho]

looks good! Synthetix was using completeWithdrawal and completeDeposit instead of finalize, and I kind of like complete more? Not super opinionated here though

[tynes]

Nit: this todo makes the line too long, could easily go in the above comment below @title

[transmissions11]

I think a depositAndCall function would be a great add. I don't see why risk is a concern there because devs are opting in to using that (vs like an ERC777 that just does the call no matter what).

Other thing is an approveDepositTo approveWithdrawTo pair of funcs (esp if you're not going to add a depositAndCall func)

[ben-chain]

Other thing is an approveDepositTo approveWithdrawTo pair of funcs (esp if you're not going to add a depositAndCall func)

Ooh this is very interesting! @maurelian and I had brainstormed approveDepositBy; ultimately this didn't make sense because an approved L1 contract could just transferFrom and depositTo. But this sounds like approving depositing to a specific target, which is very interesting. :)

Can you walk me through a use case or sketch for the underlying functionality? Is this kind of like saying "this 1address can deposit on my behalf, IFF it is to this l2address?"

I guess the concern with depositAndCall was that this would lead to scenarios where the L2 ERC20 can be a msg.sender, which is a little weird. OTOH I guess this would normally just deliver a data payload wrapped in some methodId like OVM_onDeposit(depositer,data) implemented by the target. At that point, it feels hard to exploit unless there's a collision in the methodID or something crazy in a fallback.

[transmissions11]

Can you walk me through a use case or sketch for the underlying functionality? Is this kind of like saying "this 1address can deposit on my behalf, IFF it is to this l2address?"

That's interesting but I was just imagining those funcs having the same general implementation as withdrawTo/depositTo but just approving the token to the recipient instead of transferring it to them. This is important for depositing/withdrawing directly to a contract as it will not be able to know which contract sent it those tokens.

However now that I think about it, we would likely still need more metadata about a deposit/withdrawal in most cases where funds are being deposited or withdrawn directly to a contract.

depositApproveAndCall/withdrawApproveAndCall would be the ultimate solution here (approves the tokens to the recipient contract and calls a method on them).

Let me walk thru a scenario of why that level of complexity is necessary:

• Imagine if a user inits a withdrawal on l2 and the recipient is set to a broker contract on l1.
• The user on l1 calls a broker contract's mintIOU() method and passes in the amount and token address that withdrawal should resolve to in 7 days.
• User is given an ERC1155 subtoken as an IOU.
• After 7 days anyone holding that ERC1155 subtoken can call the redeemIOU() func which will destroy the IOU subtoken and the broker will try to transfer the amount of DAI the user who originally minted the IOU specified to the user calling redeemIOU().

However this architecture has a pretty big double spend problem:

• Imagine that scenario I outlined above happens. That user is being honest and passed the correct amount and token address to the L1 broker.
• However, now imagine that there is a malicious user who wants to steal that good user's funds.
• Evil user can just not deposit on l2 and just call the method on l1 to mint the IOU and specify the same amount and token as the good user did.
• Now when the good user's withdrawal actually goes thru after the 7 days, the evil user can just call redeemIOU()before the good user does and since there is now X amount of that token in the contract (because of the good user's withdrawal), the transfer goes thru and the evil user walks away with the good user's tokens.
• The good user has just had their deposit stolen

We can solve this problem with a withdrawApproveAndCall() method that:

• Approves the amount of the token to the recipient (instead of a transfer)
• Calls the recipient with some arbitrary data.

If this method is implemented we can adjust our previous architecture to check that a withdrawal has actually come through for a specific IOU by passing data about the withdrawal via the call and ensuring the contract is actually sending us the tokens via transferFrom()

Does that make sense? Let's discuss more on our call tomorrow :)

[ben-chain]

@TransmissionsDev What would you think is the right way to handle things if the call part of withdrawAndCall fails? Seems like you either only credit the withdrawal if the subcall succeeds, or you just always credit the withdrawal. Honestly both seem like they would have associated dangers.

[transmissions11]

Hmm I'm not really opinionated either way right now?

I guess a reversion (meaning the approval will not go through if the subcall fails) will be easiest to discover on chain and tokens can always be force given to the target via the other withdraw funcs so I think I lean towards that.

[ben-chain]

This was merged in under another PR. Most recent update to this interface was here: #297