Signature frontrun alternative fix

[MathisGD]

Recap of the problem: anyone can frontrun authorizeWithSig and make the transactions revert. It is "just" a UX problem, and not really a security issue because the final state of Morpho is the same.

Quentin's fix #230 reverts on a wrong signature only if you were trying to change the authorization. The spec is a little bit complicated to me.

I propose something else: let people pass the nonce of their signature (btw now they pass the whole Authorization struct, which I find quite natural), revert only if the signature was not good with this nonce, but change the authorization and increment nonce only if the nonce of the signature matches the nonce of the contract.

The base branch was changed.

[Rubilmax]

Here's an edge case that is now possible and IMO unexpected:

1. User signs authorization, sends the tx
2. User changes mind, signs message to cancel authorization, sends tx 2
3. Tx 1 message gets frontrunned
4. Tx1 gets included without reverting
5. Tx 2 gets included without reverting (because nonce is outdated), thus the user is not aware that their authorization is not cancelled

[MerlinEgalite]

Here's an edge case that is now possible and IMO unexpected:

1. User signs authorization, sends the tx
2. User changes mind, signs message to cancel authorization, sends tx 2
3. Tx 1 message gets frontrunned
4. Tx1 gets included without reverting
5. Tx 2 gets included without reverting (because nonce is outdated), thus the user is not aware that their authorization is not cancelled

True, can we really easily fix the frontrunning attack in fact?

[MathisGD]

Then the way to abort a signature is to submit it and batch a dis-authorization again.

In your example, if the authorized is malicious they can already rug you.

[julien-devatom]

Here's an edge case that is now possible and IMO unexpected:

1. User signs authorization, sends the tx
2. User changes mind, signs message to cancel authorization, sends tx 2
3. Tx 1 message gets frontrunned
4. Tx1 gets included without reverting
5. Tx 2 gets included without reverting (because the nonce is outdated). Thus the user is not aware that their authorization is not canceled

We thought about that on Thursday w @QGarchery & @MathisGD, and it was my take always to revert, even if this is an edge case (you have to sign the same authorizer w the same nonce, with both authorized = true & authorized = false).

My point is that if you always revert, you can try/catch on top. For example, in the bulker case, we want the bulker batch not to revert. My tx will revert if the signature is extracted and submitted before my batch. But, we can add some logic before using the authorized signature in the bulker to submit the signature only if the signature was not used before.

if (BLUE.nonce(user) == input.nonce && BLUE.isauthorized(user, address(this)) == input.isAuthorized) return; 
else {

  // call blue with the signature
}

So the contract is unopinionated on edge cases and always reverts, delegating the check to the upper layer.

[MerlinEgalite]

So should we close PRs?

[QGarchery]

Reopening this PR as I did not have the chance to give my opinion at the end of the week. Feel free to close this PR afterwards if you still have the same stance on this. I think that it's a good case where we can be a bit opinionated and fix the issue for most users by not reverting when no authorization change are asked.

First, the problem highlighted by @Rubilmax was not present in the original PR #230, because we had a check to revert when a change to the authorization is asked (and that the signature is not verified).

Also, to me the argument about being a primitive is not valid because we can also add a reverting case on top. So both options are as expressive. The decision is about which one is the "more standard" and most useful:

• EIP712 specifies that an interaction involving a signature should be idempotent, which suggests that the standard is to not revert when no changes is required to the authorization
• my view is that most integrations (including the bulker) will prefer not reverting on a transaction that has the intended behavior

Finally, as soon as someone sends a transaction containing a signature for the authorization it's possible for the authorized party to manage the account in whatever way he wants. So to me the case where there are multiple signatures lying around is already not supported, so it makes it less interesting to try to fix

[MathisGD]

We finally decided not to implement a "fix" because everyone does that and the spec of the EIP is not that clear about the topic. Also it's complete, like you can re-implement any behavior on top (not for free though).