GraphToken permit() function is vulnerable to approval double spending :

[code423n4]

Lines of code

https://github.com/code-423n4/2022-10-thegraph/blob/309a188f7215fa42c745b136357702400f91b4ff/contracts/l2/token/GraphTokenUpgradeable.sol#L98

Vulnerability details

Description

GraphTokenUpgradeable exports a standard permit() function. If the signature checks out, it calls _approve(_owner, _spender, _value);This is vulnerable to a well known issue where spender of the permit can now spend previous allowance + _value, instead of just _value token. He does this by frontrunning the permit() function and spending the existing approved funds.

Impact

Spender of permit may double spend user's allowance.

Proof of Concept

Alice approves Bob access to 100 Graph tokens. After thinking about it, she wants to give Bob approval for 50 more tokens. She signs a permit for 150 Graph tokens. Seeing the Permit TX in the mempool, Bob frontruns it and spends his 100 Graph tokens. Therefore, he received access to 250 total tokens.

Tools Used

Manual audit

Recommended Mitigation Steps

Permit should only be allowed to toggle between zero and non zero allowance.

Note to judges: a very similar finding appeared in Olympus  where HIGH was awarded.

[0xean]

Downgrading to QA. After discussing this issue with a few judges have significantly revised how I think about this attack vector in general.

[pcarranzav]

Alice approves Bob access to 100 Graph tokens. After thinking about it, she wants to give Bob approval for 50 more tokens. She signs a permit for 150 Graph tokens. Seeing the Permit TX in the mempool, Bob frontruns it and spends his 100 Graph tokens. Therefore, he received access to 250 total tokens.

I would argue that if Alice wants this she should either:

a) Sign the permit for 150 GRT using the same nonce, invalidating the permit for 100, or
b) Sign a separate permit for 50 GRT

[pcarranzav]

^ To be clear, based on my argument above, I would recommend marking as invalid rather than QA; I believe the described attack vector would work for someone calling GraphToken.approve (as with any ERC20 token, and I think those are the attacks described in Olympus and code-423n4/org#51), but the described POC requires the approver to make an incorrect use of nonces or the permit feature. By design, a user can sign multiple permits with different amounts, and they will be valid if they are redeemed in the correct order based on their nonces, until the deadline.