Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(contracts): Frontrun Permit in GasSwap #30

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

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.

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.

1 participant