Skip to content

Conversation

@tapakornl
Copy link

Purpose or design rationale of this PR

Describe your change. Make sure to answer these three questions: What does this PR do? Why does it do it? How does it do it?
This PR fix the following issue

In brief, users may face front-running vulnerabilities when invoking swap(...) in GasSwap.sol, as an attacker could execute permit in ERC20Permit.sol before user swap tx is mined, leading to the user's transaction reverting due to the signature being already used.

According to https://github.com/Amxx/openzeppelin-contracts/blob/487b3f9cdd0561e151d4965ca18590bba907209a/contracts/token/ERC20/extensions/IERC20Permit.sol#L16-L33, doesn't use safePermit doesn't mean users are protected from front-running attacks.

details:
In ERC20Permit.sol permit function: https://github.com/Amxx/openzeppelin-contracts/blob/487b3f9cdd0561e151d4965ca18590bba907209a/contracts/token/ERC20/extensions/ERC20Permit.sol#L44-L67

    function permit(
        address owner,
        address spender,
        uint256 value,
        uint256 deadline,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) public virtual {
        if (block.timestamp > deadline) {
            revert ERC2612ExpiredSignature(deadline);
        }

        bytes32 structHash = keccak256(abi.encode(PERMIT_TYPEHASH, owner, spender, value, _useNonce(owner), deadline));

        bytes32 hash = _hashTypedDataV4(structHash);

        address signer = ECDSA.recover(hash, v, r, s);
        if (signer != owner) {
            revert ERC2612InvalidSigner(signer, owner);
        }

        _approve(owner, spender, value);
    }

when user sign bytes32 hash, in bytes32 structHash, one of the signed data is _useNonce(owner)
so let's see what's _useNonce(address) do.

https://github.com/Amxx/openzeppelin-contracts/blob/487b3f9cdd0561e151d4965ca18590bba907209a/contracts/utils/Nonces.sol#L27-L34

function _useNonce(address owner) internal virtual returns (uint256) {
        // For each account, the nonce has an initial value of 0, can only be incremented by one, and cannot be
        // decremented or reset. This guarantees that the nonce never overflows.
        unchecked {
            // It is important to do x++ and not ++x here.
            return _nonces[owner]++;
        }
    }

so every time _useNonce(...) is triggered user nonce will increase by 1, what does this mean?
every time function permit(...) is called user nonce will increase by 1.
so here is the scenario,

  1. user A signed permit hash at _nonces[A] = 0 and call function swap(...) in gasSwap.sol
  2. attacker frontrun the signature and execute the function permit(...), after attacker execute permit _nonces[A] = 1
  3. step 1. get executed, user signature signed at _nonces[A] = 0, but now ERC20Permit.sol expect user to sign hash at _nonces[A] = 1, so the transaction will always revert.

@codecov-commenter
Copy link

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@Thegaram
Copy link
Contributor

Thegaram commented May 8, 2025

Thanks again for submitting this fix. We decided to close this PR since GasSwap is not used and will be removed from this repo.

@Thegaram Thegaram closed this May 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants