[backends/eth] Use approve instead of increaseAllowance for ERC20 deposit

[manoranjith]

Location

[backend/ethereum/channel]

Problem

Currently, for funding a channel with ERC20 tokens, we use increaseAllowance to set the amount that the perun contracts can transfer from the user's account.

However, the increaseAllowance functions is not a part of the ERC20 standard interface, but an extension provided by open zepplin reference implementation of ERC20 token standard. It is provided as an alternative to approve, in order to avoid some of the issues associated with approve function. However, the ERC20 token standard does not include this extension for maintaining backward compatibility and suggests careful usage of the approve method as the solution to deal with the issues.

Links: attack description, , discussion related to it and suggested mitigation technique.

Proposal

Replace the usage of increaseAllowance with approve method.

However, the implementation would be tricky and not straightforward. Especially when we consider the scenario where, a user wants to fund multiple channels with the same ERC20 token (described below).

Say a user wants to fund ch1 with 10 PRN tokens and ch2 with 20 PRN tokens.
The challenge here would be

1. First, we set the allowance to 10 PRN.
2. Then we want to increase it by 20 PRN. Because there is not a straightforward way to atomically increase the allowance using the interfaces defined in ERC20 standard.

[manoranjith]

@matthiasgeihs @ggwpez

[ggwpez]

So the problem in your example would be that the user briefly approved 30 instead of the 20?

I think it is not a problem since both transactions that the ERC20Depositor sends can only be created by the approver.
The depositEnact function of the AssetHolderERC20 debits msg.sender, hence no front-running is possible.

The only problem that i see is with parallel funding, this could mess up the current approval.
@manoranjith

[manoranjith]

The problem is not that user briefly approved 30. That part is acceptable because, perun contracts indeed want to transfer a total amount of 30.

But, as you rightly pointed out, it will be with parallel funding.

1. We approve 10 ETH.

2. Then, we want to approve an additional 20 ETH. Because, there is no atomic increase operation, we have two options

   1. We can read the allowance, add to it and then set the allowance. But what is the allowance has changed between use reading and the new transaction being mined.
   2. We can wait for the allowance to reach 0 and then set it. But, in this case also if we have parallel funding of 3 or more channels, it would be an issue. As to how it should be ordered.
      @ggwpez

[matthiasgeihs]

Seems relevant to me. Need to consider that for one of our upcoming releases.

[ggwpez]

The most obvious solution would be to disable parallel funding for ERC20, as it does not seem to be supported by the standard.
@manoranjith