Withdraw function is unfriendly to potential pooling strategy

[koeppelmann]

The withdrawal function will throw if:

        require(
            lastCreditBatchId[user][token] < getCurrentBatchId(),
            "Withdraw not possible for token that is traded in the current auction"

This is to not allow the user to withdrawal a balance that was credited from a not yet finalized solution.
If a user wants to guarantee that they can withdrawal token A they would need to cancel all orders that buy token A. This is inconvenient already for a regular user.
One could imagine in addition a contract user that regularly liquidate all kind of incoming tokens for token A. Such a contract could potentially never get a chance to withdrawal A. See this proposal for a contract which might have those characteristics.

Another type might be a pooling contract. Imagine a contract that runs the simple liquidity strategy across 7 stable coins but enables pooling. Under a lot of usage, it might become impossible to withdrawal any tokens reliable without halting all order (which of course would not ever be desired if e.g 100 users are pooling together liquidity and just 1 user wants to cash out)

An alternative implementation could not strictly forbid withdrawal under incoming tokens from the solver but instead, look at the amount that is incoming (would need to go over all orders??) and allow to withdrawal the amount that is not affected.

At the very least we could allow withdrawals from minute 4-5 in a batch since here solutions are final. Such a decision would also lower the minimum trade time from deposit to withdrawal from 5-10 minutes to 4-9minutes.

[josojo]

Thanks for opening this issue. I agree that we need to find a solution for it!

Before we decided on one option, I just wanna brainstorm about the solution space:

Orders that are only allowed to be settled in even batches
For the pooling contract, it will be a challenge to determine the his current balances in the exchange. Reliable, it can only be determined in the minute 4-5 in batch... But reading reliably the current balances will be required to determine the pool shares for new deposits.
If the pooling contract would place only orders, which can be only executed in "even" batches, then we would make sure that withdraws can always be processed in even batches and we can determine the balances very reliable the balances in the even batches as well. It would solve two problems in one catch!
But having a pool, which trades only halve the time might be seen to be very inefficient.

Do not allow trades if there is an open withdrawRequest:
We could check in the settlement of a batch, whether the buyer of an order has an open withdrawRequest for the bought token. If this is the case, then we would not allow the trade to be executed.
This solution makes the 1-transaction trade a little bit more complicated, as we need to place a futureWithdrawRequest for the batchID : n+2, if the deposits happened in batchID n. If then the trade is not settled immediately, it becomes problematic...

These are really just two ideas having significant downsides but might be a little bit more gas efficient as Martins proposals.

[fleupold]

An alternative implementation could not strictly forbid withdrawal under incoming tokens from the solver but instead, look at the amount that is incoming (would need to go over all orders??) and allow to withdrawal the amount that is not affected.

I don't think we would have to go over all orders. When applying a solution we could on top of setting lastCreditBatchId[user][token] also set lastCreditAmount[user][token] and then limit the withdrawal amount to that.

Or on withdraw we iterate over previous solution to identify how much of the desired token was credited in the last batch. This would however break the abstraction that EpochTokenLocker doesn't need to know anything about the concrete BatchExchange protocol implementation.

Either way this change would be quite significant from a logic perspective and likely require a careful re-audit.

I wonder if for now as a workaround a withdrawal request in a pooling contract could be implemented as a future cancellation of orders (e.g. to the next full hour) + re-instantiation of all orders 5 minutes later?

[josojo]

I wonder if for now as a workaround a withdrawal request in a pooling contract could be implemented as a future cancellation of orders (e.g. to the next full hour) + re-instantiation of all orders 5 minutes later?

What would happen if I would like to withdraw 0.001 pool shares every 5 minutes, will this block the pool from executing trades, as I am canceling them constantly?
Probably, one consequence would be that one can only withdraw "once an hour" or another time period, which could be fine.

I thought a little bit more about all the different options. Iterating over the max 30 orders and calculate the 'credited sum' if the lastCreditBatchId[user][token] indicates that there was an order, seem to be the most gas efficient way, if we want to solve the issue. Also, it really does not drastically modify any part of the logic.

[fleupold]

Iterating over the max 30 orders and calculate the 'credited sum'

The issue with this is that it breaks the unidirectional dependency of BatchExchange --> EpochTokenLocker. Now EpochTokenLocker would have to know about BatchExchange's implementation detail which is a larger change to the architecture.

Maybe we can do the computation inside submitSolution and thus keep the dependencies as is. Are we sure this is an important enough issue to redeploy the contracts? It should definitely be carefully re-audited.

[josojo]

Maybe we can do the computation inside submitSolution and thus keep the dependencies as is. Are we sure this is an important enough issue to redeploy the contracts? It should definitely be carefully re-audited.

Your proposal has two main disadvantages:

• user experience for the pools is worse, as withdraws take longer. [People will just have the comfort of uni swap in the back of their mind]
• Canceling the orders and reactivating them is not a challenge, but triggering the withdraw in just the right 5 min window could be a challenge.

If we really want to have pooled contracts, currently I would lean towards implementing a solution in the batchExchange contract itself.
But maybe it is sufficient if we just have single pools for the PoC :)

[koeppelmann]

In my view going over the 30 orders (if there is at all a "pending submitted solution" would be very reasonable and should not be a big deal in gas costs.

We have this line already in the withdrawl function:
uint256 amount = Math.min(balanceStates[user][token].balance, balanceStates[user][token].pendingWithdraws.amount);

The addition would simply be that one would need to run over all touched orders and if user==user and buyToken==withdrawl token then sum up a variable "pending_receive_amount".

If we have the "pending_receive_amount" we can simply change the line to:
uint256 amount = Math.min(balanceStates[user][token].balance - pending_receive_amount, balanceStates[user][token].pendingWithdraws.amount);

This change would only affect the withdrawal function.

An alternative suggestion would be, that when balances are increased in the solver submission, that instead of manipulating balance directly the "deposit" function could be used that will only affect the balance of the next batch but not the ongoing one.

[josojo]

An alternative suggestion would be, that when balances are increased in the solver submission, that instead of manipulating balance directly the "deposit" function could be used that will only affect the balance of the next batch but not the ongoing one.

Here a problem might be that we could end up with negative balances, and positive deposits. This is a challenge and would require us to introduce "trade-deposits" or "trade-credits", which would work like deposits, but are accounted separately.

[josojo]

The addition would simply be that one would need to run over all touched orders and if user==user and buyToken==withdrawl token then sum up a variable "pending_receive_amount".
If we have the "pending_receive_amount" we can simply change the line to:
uint256 amount = Math.min(balanceStates[user][token].balance - pending_receive_amount, balanceStates[user][token].pendingWithdraws.amount);
This change would only affect the withdrawal function.

Yeah, this should be the way to go, if we want to modify it. Agreed

[bh2smith]

I get the impression that some of the comments here are hinting at the fact that the Exchange has access to the users balance and pending withdraws. With this information, would it not suffice to simply consider the pending amounts "frozen" and revert if the frozen amount is being tapped during a trade.

This change would only affect the withdrawal function.

I feel like this suggestion affects the submitSolution more than withdraw` (also recall that withdraw is part of the token locker and it can't know anything about the orders being touched - as @fleupold has pointed out).

[josojo]

My conclusion so far:

Keep the contract as it is
This would have three significant disadvantages:

1. The pooled bracket strategy will have a high likelihood of failing withdraws unless we are canceling orders for some periods and restore them later.
2. If we go with canceling orders, then withdraws can only be done in certain time windows, which has a much worse UX than uni swap pools and the liquidity would be "fragmented" over time.
   [Especially the logic for scheduling the withdraw into a certain time window seems to be a challenge, which would increase the complexity of all dependent contracts]
3. The complexity of the pool contracts would be increased.

Implement calculation of "credited funds" in last auction
This would have one significant disadvantages:

1. The two contracts, BatchExchange and EpochTokenLocker would be required to be merged, as their logic will be two depended on each other.

The additional complexity is overseeable. I would estimate 1,5 week development time + 0,5 weeks audit.

I am in favor of changing the smart contracts.

[fleupold]

Here is the plea for not changing the smart contract (I'm not saying I'm strongly opposed to it, but feel like that view point is missing in the arguments so far):

This issue outlined here is only a real concern if we have a pool that is trading in almost every batch (as any batch that it didn't trade in will be withdrawable). Moreover withdraws are only blocked after a solution has been submitted (as long as we are solving batches usually not until 3 minutes in). Pooling contracts could thus for the most part be driven by an aware party that times withdrawals in the first blocks of a new auction.

Therefore I believe this is only of real concern if the protocol is largely successful in which case we have to tackle the scaling issue with a new smart contract approach anyways.

On the flip-side, changing the contracts with such a fundamental change now will delay a public launch and thus our ability to learn what things really matter to people trying out our exchange. It also sets a (arguably low bar) precedence for last-minute changes in the future.
It will also require address updates on the downstream infrastructure pieces and require people already interacting with the contracts to migrate funds. I agree that all of this can probably be done in 2 weeks.

The upside of this change would be that we could address some of the other tasks we noticed in the last months which by themselves would not justify a redeployment (e.g. #539, #519).

[fleupold]

I'm however very concerned about the suggestion of merging EpochToken and BatchExchange as this is not an isolated change and will require re-reviewing the entire contract.

I think if the suggestion is implemented, addBalanceAndBlockWithdrawForThisBatch should manage a tuple of lastCreditBatchId and lastCreditAmount (instead of just lastCreditBatchId). The logic would be along the lines of:

if (hasValidWithdrawRequest(user, token)) {
  if (lastCredit[user][token].batchId != getCurrentBatchId()) {
    lastCredit[user][token].batchId = getCurrentBatchId()
    lastCredit[user][token].amount = amount;
  } else { 
    lastCredit[user][token].amount += amount()
  }
}

This way we wouldn't have to break up the de-coupling.

Implementing this feature would also pose the question if we should allow unblocked withdraws after the 4 minute finality period and if we should revert the blocked amounts in case of solution reversion.

[koeppelmann]

I think if we do not make the change we can not build pool contracts that have valid forever orders like e.g. the bracket strategy has them. While in practice it will work >95% this is not good enough to release that as a product for users. If we release something we can not say, most of the time your withdrawal will work.

However - if we address I agree with @fleupold assessment to keep the separation of epoch locker and batch exchange. This would require saving indeed a tuple and would also require undoing last credit values if a solution is reverted (at least setting the amount to 0)

A potential alternative solution would be to simply perform the pending withdrawal as the first step in "addBalanceAndBlockWithdrawForThisBatch"
This would remove the need to have the lastCreditBatchId data structure altogether.

This would increase the gas costs for the solver but I assume given that we can safe the storage update from lastCreditBatchId the change will be in many cases (if most touched orders do not trigger a withdrawal) will even be positive.

One thing to make sure is that a failed withdrawal (e.g. because of a malicious token) or even a token like PAX Gold should not make the tx fail.
Is this already guaranteed with: SafeERC20.safeTransfer(IERC20(token), user, amount);?

In this case, we should also double-check reentrancy protection. Since a withdrawal makes a call to an untrusted token contract we must make sure that the BatchExchange is in a locked mode.

[bh2smith]

What is the concern with PAX gold?

[koeppelmann]

What is the concern with PAX gold?

It has a build-in fee on its transfer and transfer from function.
This means if a user does a deposit into the exchange of e.g. 1000 units the exchange will call a transfer from but it will result in e.g. only 950 units being transferred to the exchange. However - the exchange will nevertheless credit 1000 units to the user. When the user wants to withdrawal 1000 units the tx. will fail. However - we need to assume this in any case. Tokens are external untrusted contracts and calling a function there can result in arbitrary actions.

While writing this I realized that my proposal will not work. One can write a malicious token that will make the withdrawal fail under specific circumstances. In this case, it is not clear how to proceed.

Although one simple suggestion could be to simply delay the withdrawal request another batch or even delete the token balance.

Note: this is something that should never happen on a propper token.

[koeppelmann]

To keep deterministic behavior the withdrawal would need to be called with a capped gas amount and there would need to be a way to handle withdrawal failures (e.g. delay the withdrawal request)

[josojo]

The conclusion from the meeting's discussion:

There are essentially two paths: The first one would be to redeploy the contracts with a fix and require everyone to switch to new the contract. As auditing and bug-bounty etc, are quite some work, this would potentially delay the product launch, however it would enable pooled contracts.
The second path is that we do a quicker launch with the current contracts and a quicker product evaluation such that we can develop a version 2 earlier. For this path, we would not enable pooled contracts but just enable individual liquidity provision strategies. For the individual strategies, the withdraw is much less of a hassle, as orders can easily be canceled.

We decided to go for the second path, as there are many potential advancements, which we might. already want to include in a second version in the second half of the year.