fix(contracts): Frontrun Permit in GasSwap

[tapakornl]

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

• Remove SafeERC20.safePermit OpenZeppelin/openzeppelin-contracts#4582
• https://www.trust-security.xyz/post/permission-denied.

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.

PR title

Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:

• fix: A bug fix

Deployment tag versioning

Has tag in common/version.go been updated or have you added bump-version label to this PR?

• No, this PR doesn't involve a new deployment, git tag, docker image tag
• Yes

Breaking change label

Does this PR have the breaking-change label?

• No, this PR is not a breaking change
• Yes

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 57.08%. Comparing base (5362e28) to head (2a96c8f).

❗ Current head 2a96c8f differs from pull request most recent head 2fdd7f4. Consider uploading reports for the commit 2fdd7f4 to get more accurate results

Files	Patch %	Lines
contracts/src/gas-swap/GasSwap.sol	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1184      +/-   ##
===========================================
+ Coverage    56.16%   57.08%   +0.91%     
===========================================
  Files          157      153       -4     
  Lines        12176    11322     -854     
===========================================
- Hits          6839     6463     -376     
+ Misses        4814     4387     -427     
+ Partials       523      472      -51     

Flag	Coverage Δ
contracts	57.53% <0.00%> (+2.28%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[bzarka]

Hi,

How does it do it?
The PR directly removes the SafeERC20.safePermit functionality from the codebase, thereby eliminating the potential vulnerability associated with it. By removing this functionality, the PR ensures that users are protected from front-running attacks when interacting with the GasSwap.sol contract.

Overall, the PR addresses a critical security concern and ensures the integrity and safety of the codebase by removing potentially exploitable functionality.

[tapakornl]

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.

add contract with normal permit(no try catch) + testing here:
#1189

[tapakornl]

Also, I just pushed a fix to the test. Could you give it another look? I'm new to Hardhat. Thanks!

[tapakornl]

@Thegaram hi! any update?

[spilehchiha]

hi @tapakornl we're reviewing this week, thank you very much for your patience. Update ASAP.

[zimpha]

please move to https://github.com/scroll-tech/scroll-contracts.