OpenZeppelin / openzeppelin-contracts

OpenZeppelin Contracts is a library for secure smart contract development.
https://openzeppelin.com/contracts
MIT License
24.98k stars 11.8k forks source link

Permit2 Typehash Inconsistency #5292

Closed saibaneer closed 2 weeks ago

saibaneer commented 2 weeks ago

I have been trying to implement a meta-tx using permit2, and i noticed that I was getting different onchain and offchain digests given the same permit structure. When I dug slightly deeper, I saw that in the permit2 contract that lives on sepolia and polygon the domain type hash is given as:

bytes32 private constant _TYPE_HASH =
        keccak256("EIP712Domain(string name,uint256 chainId,address verifyingContract)")

Meanwhile, on the openzeppelin repo for EIP712 (which the permit2 contracts often relate with if you want to do an on-chain verification) the typehash is given as:

bytes32 private constant TYPE_HASH =
        keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)");

The difference? The EIP712 from openzeppelin has a version type, while the deployed permit2 does not. https://polygonscan.com/address/0x000000000022D473030F116dDEE9F6B43aC78BA3#code https://sepolia.etherscan.io/address/0x000000000022D473030F116dDEE9F6B43aC78BA3#code

From all indications, any attempt to do an onchain verification will faill using:

function depositERC20(
        IERC20 token,
        uint256 amount,
        uint256 nonce,
        uint256 deadline,
        bytes calldata signature
    ) external nonReentrant {

        bytes32 structHash = keccak256(abi.encode(
            TRANSFER_FROM_TYPEHASH,
            keccak256(abi.encode(TOKEN_PERMISSIONS_TYPEHASH, token, amount)),
            address(this),
            nonce,
            deadline
        ));
        bytes32 digest = _hashTypedDataV4(structHash);
        address user = ECDSA.recover(digest, signature);
        // Credit the caller.

        // Ensure the recovered address matches msg.sender
        require(user == msg.sender, "Invalid signer"); //keeps failing here
        tokenBalancesByUser[user][token] += amount;
        // Transfer tokens from the caller to ourselves.
        PERMIT2.permitTransferFrom(
            // The permit message. Spender will be inferred as the caller (us).
            IPermit2.PermitTransferFrom({
                permitted: IPermit2.TokenPermissions({
                    token: token,
                    amount: amount
                }),
                nonce: nonce,
                deadline: deadline
            }),
            // The transfer recipient and amount.
            IPermit2.SignatureTransferDetails({
                to: address(this),
                requestedAmount: amount
            }),
            // The owner of the tokens, which must also be
            // the signer of the message, otherwise this call
            // will fail.
            user,
            // The packed signature that was the result of signing
            // the EIP712 hash of `permit`.
            signature
        );

    }

}

code to reproduce te bug is here: https://github.com/saibaneer/permit2vault/blob/main/contracts/Vault.sol

cairoeth commented 2 weeks ago

EIP712 in this library follows the details described in the EIP, excluding salt. You can override _hashTypedDataV4 and adjust it for Uniswap's permit2 TYPE_HASH definition.

saibaneer commented 2 weeks ago

I might be misunderstanding you, but let me provide more context. The current address (0x000000000022D473030F116dDEE9F6B43aC78BA3) associated with Permit2 will only work with any other vault contract that mirrors the same


bytes32 private constant _TYPE_HASH =
        keccak256("EIP712Domain(string name,uint256 chainId,address verifyingContract)")

If a vault is created, and it inherits the EIP712 abstract contract, it will be required to provide both a name and a version. So are you suggesting that the way forward will be to modify the EIP712 abstract contract such that it no longer uses name & value in the constructor, but only name? I'm concerned doing this may make the implementation unsafe as it tinkers with an audited base eip712 contract.

What are your thoughts? Do you have an example of the implementation you suggested? Can you share it?

cairoeth commented 2 weeks ago

You can use https://github.com/Uniswap/permit2/blob/main/src/EIP712.sol and combine it with ours -- for other questions/support, please refer to the forum.