First unset allowance then set to MAX

[MartinquaXD]

Description

Any time we need to increase an allowance we should first set the allowance to 0 to please tokens that enforce that behavior to prevent this attack.

Came up due to this alert.

Changes

For every approval emit 2 interactions:

1. set approval to 0
2. set approval to U256::MAX

While this seems slightly wasteful we can't detect whether a token absolutely requires this (with reasonable effort) and AFAIK the old encoding logic also did this for every approval.
And finally token approvals happen pretty rarely so the extra gas cost here is worth the reduced implementation complexity.

How to test

TODO: set up a forked e2e test that tries to sell USDT which enforces this behavior.

[fleupold]

we can't detect whether a token absolutely requires this

We could try to eth_call approve from the context of the settlement contract and if it fails encode a reset to 0 additionally. This way we wouldn't encode it twice unless we have to.

Alternatively, we could consider only encoding the amount that is required for this interaction (which should make it go back to 0 after the settlement). However, this would also waste gas.

Overall I'm not sure what the best approach here is.

[fleupold]

USDT doesn't have this issue since it doesn't update the allowance if it's set to U256::max (code). This is only needed for tokens that require resetting the allowance to 0 before changing it and that don't have this "don't update max allowance" optimisation which is very rare I believe.

[MartinquaXD]

Solvers can encode approvals themselves so we can't assume that the allowance of USDT would always be set to U256::MAX, though.

[fleupold]

Sure, but imo there are only two reasonable strategies

1. Set it to what you need for this settlement (in which case it's 0 at the end of the settlement)
2. Set it to max

I'd be surprised if solvers were using any other values.

[MartinquaXD]

Having any amount of uncertainty or an off-by-1 error on the approvals would already cause issues. TBH if this logic becomes too involved I would likely just scrap this PR since this error obviously doesn't happen very often.

[MartinquaXD]

Oh, also another strategy a sophisticated solver could implement is to round the approval amount up until it includes more 0 bytes to save on gas for the calldata.

[MartinquaXD]

We could try to eth_call approve from the context of the settlement contract and if it fails encode a reset to 0 additionally. This way we wouldn't encode it twice unless we have to.

Yes, there are ways to detect this but given how tiny the gas savings are I called them unreasonable effort and added complexity.

Given that we had this approach implemented for a very long time without ever giving a second thought to the gas savings I'm very much convinced this is the way to go in the spirit of simplicity.

[fleupold]

Given that we had this approach implemented for a very long time

Do you have a code pointer? I'm not sure this is the case.

Also wouldn't this line in the new encode make it so that the 0 approval always gets filtered out? If you want to go with this approach we probably have to add an extra case for 0 there.

[MartinquaXD]

Also wouldn't this line in the new encode make it so that the 0 approval always gets filtered out?

Yeah, good catch. Initially I just wanted to chuck the new code into the encoding logic which would not have this issue AFAIK but that didn't feel super clean. Will take a look how to best integrate this somehow.

Do you have a code pointer? I'm not sure this is the case.

Mmmh, I found a handful of places that always encode 0 approvals for that reason but they all seem to be related to encoding quotes and verifying them. So I must have misremembered as I indeed did not find this in code paths that would be executed when encoding a tx that will be submitted onchain. :/

Intuitively I would still assume always encoding the unset interaction should still be fine given that tokens need to be approved rarely. Since building logic that detects these cases seems pretty involved from the top of my head (at least 1 simulation per approval) maybe it makes sense to look at historic data to see how big the impact of that change would be.

[fleupold]

@cowprotocol/backend what do you guys think is the right approach here (specifically @m-lord-renkse as you debugged this issue yesterday)?

Probably the "easiest" fix would be to set the exact amount instead of max (and rely on other solvers improving this)

[MartinquaXD]

Probably the "easiest" fix would be to set the exact amount instead of max (and rely on other solvers improving this)

While I agree that this would be the easier fix it does not make a lot of sense to me. If we are concerned about gas costs then why would we implement the fix such that we require an approval on every single transaction instead of having slightly higher gas costs every once in a while when we have to approve a new token?

[m-lord-renkse]

Does encoding the unset interaction also uses too much gas or is it negligible? I guess it is comparable to if not the same as approving every single transaction. So, in this case, unset-ing it and then allowing max would be the mos convenient for the final user 🤔

[github-actions]

This pull request has been marked as stale because it has been inactive a while. Please update this pull request or it will be automatically closed.