search

frangio / erc6909-extensions

ERC-6909 Usability & Security Extensions
11 stars 0 forks source link

Signatures can be burned by using them in nested temporary approvals #2

Open zobront opened 9 months ago

zobront commented 9 months ago

I'm not sure if this is a problem, but I'm auditing a protocol using ERC6909X and it crossed my mind as something that could cause an issue in certain specific circumstances, so figured I'd share...

As you mention in the EIP, the signatures allowing for approvals are publicly visible in the mempool. It is assumed that this visibility is safe because anyone using the signature should be forced to execute the exact callback specific in the signed data, and therefore will get the same results as were intended by the signer (with other protections you mention in the Security Considerations section).

However, there is one specific situation where this assumption may not hold: if an attacker nests an approval signature within a temporary approval call, its action will be undone when the temporary approval is unwound. This will use the nonce and burn the signature without getting the intended result.

Example

Let's imagine a situation where a user has approved a spender to spent 10e18 of their token. They need the send 11e18 of the token, and afterwards, they want to reset the approval to zero. How do they do it?

The most commonsense option using ERC6909X would be to issue two transactions:

1) temporaryApproveAndCallBySig() with an _amount of 11e18 (nonce X) 2) approveBySig() with an _amount of 0 (nonce X + 1)

Both transactions are sent to the mempool, with both signatures visible. When the temporaryApproveAndCallBySig() transaction is started, the prevAllowance of 10e18 is cached. We then make the onTemporaryApprove() call.

Within this call, the attacker regains control flow, and can use it (along with the signature from the mempool) to call approveBySig(). This will pass (nonce = X + 1) and set the approved amount to 0.

But after the call ends, the outer temporary function will reset the allowance to 10e18.

Both signatures will be "used up" (because their nonces will be used), but the result is that the spender will retain their approval.

In the event that signatures are difficult to obtain (or can't be obtained again), or that the spender has unlimited access to temporary transaction signatures (say, from a server that's been permissioned to give them in certain circumstances), this could block the user's ability to revoke access from the spender.

frangio commented 9 months ago

Do you have ideas about how this could be addressed?

I'm leaning towards considering this a non-issue on the basis that there is inherently some trust placed on the callback target. This also sounds very similar to ERC20's approve frontrunning risk, which nowadays seems to be considered a non-issue as well.

zobront commented 9 months ago

Yeah, after more thought, I think I'm with you. Like you said (and everyone agreed with ERC20 approve frontrunning), if someone malicious has an approval already, this doesn't really increase the risk surface area.

I might add a section to the Security Considerations section of the EIP mentioning it in case it happens to matter for a specific project, but agree that any fix in the code would be overkill.