Can't get the MinimalForwarder Contract to successfully verify my EIP712 signature

ratio · February 20, 2023, 7:54pm

I just can't get the MinimalForwarder Contract to successfully verify my EIP712 signature.
Having thoroughly tested verifying the signature with tools like ethers or metamask eth-sig-utils works successfully however. (see code below). My last hope is that someone with clear eyes can help me resolve this issue. Thank you!

Code to reproduce

Here is the data I signed (both using Metamask, metamasks eth-sig-utils as well as ethers).
The resulting signature is the same.


const request = {
    "value":0,
    "gas":300000,
    "nonce":0,
    "from":"0x32a7914b011e1dd6b58980080cb525bc546bc164",
    "to":"0x46f4cd7c9c6aca27becf45cc5d836dddac204d32",
"data":"0x7a7f274ddbf166cf3a3757674f041fbd4ccc1cd6444a3ccebd526e196b6d24f1283aaf0000000000000000000000000000000000000000000000000000000000000000a01f5e482a2b11c7a811a5abe748024b24814656ca1d1f6c3d9a27d49ff75e54ff2cea6183837101e37145bacb145089ccd294e3ab1c1a5cdb98bcf5a6a675e59e000000000000000000000000000000000000000000000000000000000000001b0000000000000000000000000000000000000000000000000000000000000035697066733a2f2f516d6652376235706441435750515767534a6648726639467771677739726634474e505958394a794a5036566b380000000000000000000000"
}

const ForwardRequestType: MessageTypeProperty[] = [
    { name: 'from', type: 'address' },
    { name: 'to', type: 'address' },
    { name: 'value', type: 'uint256' },
    { name: 'gas', type: 'uint256' },
    { name: 'nonce', type: 'uint256' },
    { name: 'data', type: 'bytes' }
]

const typedData = {
    domain: {
        name: 'MinimalForwarder',
        version: '0.0.1',
        chainId: 80001,
        verifyingContract: '0x2db308bbecc75649a37e166680986044dbab483d',
    },
    primaryType: 'ForwardRequest',
    types: {
        ForwardRequest: ForwardRequestType
    },
    message: {
        request: request
    }
};

const signature = "0xa5cb69a356943e1ee9641e8d301def17726fe9c080371f22020599172387505d097a4c84d779193abc7a21dda67bdaddb8810e9771c055734f25ccdfc84756f21b";

the following successful test shows that the signature is indeed correct:

let recovered_address = ethers.utils.verifyTypedData(
            typedDataFromApp.domain as TypedDataDomain,
            { ForwardRequest: TypedData.types.ForwardRequest },
            request,
            signature
        );

expect(recovered_address1.toLowerCase()).to.equal(request.from);

(I also repeated this test with metamask eth-sig-utils, recovery also works here).

In contrast, this test FAILS (response from MinimalForwarder verify(): false):

const { fw } = await loadFixture(deployForwarder);
const res = await fw.verify(request, signature);
expect(res).to.equal(true);

Environment

OpenZeppelin MinimalForwarder contract 0.0.1
see code here

ernestognw · February 22, 2023, 9:11pm

Hey @ratio,

Taking a quick look it may be this:

const typedData = {
    ...
    message: {
        request: request
    }
};

When I think you should do:

const typedData = {
    ...
    message: request
};

Actually, just tested and it works, here's an example with Defender Autotask + Relay (is the same as an ethers signer):

const {
  DefenderRelayProvider,
  DefenderRelaySigner,
} = require("defender-relay-client/lib/ethers");
const { ethers } = require("ethers");

const request = {
  value: 0,
  gas: 300000,
  nonce: 0,
  from: "0x32a7914b011e1dd6b58980080cb525bc546bc164",
  to: "0x46f4cd7c9c6aca27becf45cc5d836dddac204d32",
  data: "0x7a7f274ddbf166cf3a3757674f041fbd4ccc1cd6444a3ccebd526e196b6d24f1283aaf0000000000000000000000000000000000000000000000000000000000000000a01f5e482a2b11c7a811a5abe748024b24814656ca1d1f6c3d9a27d49ff75e54ff2cea6183837101e37145bacb145089ccd294e3ab1c1a5cdb98bcf5a6a675e59e000000000000000000000000000000000000000000000000000000000000001b0000000000000000000000000000000000000000000000000000000000000035697066733a2f2f516d6652376235706441435750515767534a6648726639467771677739726634474e505958394a794a5036566b380000000000000000000000",
};

const ForwardRequestType = [
  { name: "from", type: "address" },
  { name: "to", type: "address" },
  { name: "value", type: "uint256" },
  { name: "gas", type: "uint256" },
  { name: "nonce", type: "uint256" },
  { name: "data", type: "bytes" },
];

const typedData = {
  domain: {
    name: "MinimalForwarder",
    version: "0.0.1",
    chainId: 80001,
    verifyingContract: "0x2db308bbecc75649a37e166680986044dbab483d",
  },
  primaryType: "ForwardRequest",
  types: {
    ForwardRequest: ForwardRequestType,
  },
  message: request,
};

exports.handler = async (credentials) => {
  const provider = new DefenderRelayProvider(credentials);
  const signer = new DefenderRelaySigner(credentials, provider);
  const signerAddress = await signer.getAddress();

  const signature = await signer._signTypedData(
    typedData.domain,
    typedData.types,
    typedData.message
  );

  const verifiedAddress = ethers.utils.verifyTypedData(
    typedData.domain,
    typedData.types,
    typedData.message,
    signature
  );

  console.log(
    ethers.utils.getAddress(signerAddress) ===
      ethers.utils.getAddress(verifiedAddress)
  ); // true
};

Hope this helps

ratio · February 23, 2023, 6:21pm

Hey Ernesto,
thanks for taking the time to look into it.
The issue is not that I can not verify typedSignatures with ethers or metamask script (I can), but with the actual MinimalForwarder.sol.

I first generate the request and signature (I could do this with ethers, DefenderRelaySigner or metamask eth-sig-utils, it makes no difference really).

        const [writer] = await ethers.getSigners();
        const request = {
            "from":writer.address,
            "to":"0x46f4cd7c9c6aca27becf45cc5d836dddac204d32",
            "value":0,
            "gas": 300000,
            "nonce":1,
            "data":"0x7a7f274ddbf166cf3a3757674f041fbd4ccc1cd6444a3ccebd526e196b6d24f1283aaf0000000000000000000000000000000000000000000000000000000000000000a01f5e482a2b11c7a811a5abe748024b24814656ca1d1f6c3d9a27d49ff75e54ff2cea6183837101e37145bacb145089ccd294e3ab1c1a5cdb98bcf5a6a675e59e000000000000000000000000000000000000000000000000000000000000001b0000000000000000000000000000000000000000000000000000000000000035697066733a2f2f516d6652376235706441435750515767534a6648726639467771677739726634474e505958394a794a5036566b380000000000000000000000"
}

        const signature = await writer._signTypedData(
            TypedData.domain, 
            {ForwardRequest: TypedData.types.ForwardRequest}, 
            request
        );

now after generating this signature, I run 2 tests:
First, I try to verify with ethers, which works fine:

        let recovered_address = ethers.utils.verifyTypedData(
            TypedDataFromApp.domain as TypedDataDomain,
            { ForwardRequest: TypedData.types.ForwardRequest },
            request,
            signature
        );
        expect(recovered_address).to.equal(writer.address);

then, the part where I am using the MinimalForwarder contract to verify the signature and it always fails:

        const { fw } = await loadFixture(deployForwarder);
        const res = await fw.verify(request, signature);
        expect(res).to.equal(true);

I use the standard MinimalForwarder contract from OpenZeppelin without any changes. Or do I actually need to change anything in the contract to get it working?

github.com

OpenZeppelin/openzeppelin-contracts/blob/master/contracts/metatx/MinimalForwarder.sol

// SPDX-License-Identifier: MIT
// OpenZeppelin Contracts (last updated v4.8.0) (metatx/MinimalForwarder.sol)

pragma solidity ^0.8.0;

import "../utils/cryptography/ECDSA.sol";
import "../utils/cryptography/EIP712.sol";

/**
 * @dev Simple minimal forwarder to be used together with an ERC2771 compatible contract. See {ERC2771Context}.
 *
 * MinimalForwarder is mainly meant for testing, as it is missing features to be a good production-ready forwarder. This
 * contract does not intend to have all the properties that are needed for a sound forwarding system. A fully
 * functioning forwarding system with good properties requires more complexity. We suggest you look at other projects
 * such as the GSN which do have the goal of building a system like that.
 */
contract MinimalForwarder is EIP712 {
    using ECDSA for bytes32;

    struct ForwardRequest {

This file has been truncated. show original

ernestognw · February 23, 2023, 7:13pm

Thanks for providing further details, now I see that the recovery is not the problem itself, but making it match with the MiniminalForwarder recovery.

I see 2 problems:

Your signature is missing the type EIP712Domain, which is marked as required by the EIP-712, take for reference the MinimalForwarder tests
Your second example uses nonce: 1, which will be rejected by the MinimalForwarder when it's just been deployed

I think solving these two should be enough to get it working. Let me know if it's still failing.

Or do I actually need to change anything in the contract to get it working?

No, although it's minimal it works. We're currently also working on improvements but feel free to continue since it'll be backward compatible

github.com/OpenZeppelin/openzeppelin-contracts

Make `MinimalForwarder` production-ready

OpenZeppelin:master ← ernestognw:lib-643-production-ready-minimal-forwarder

opened 04:50AM - 22 Feb 23 UTC

ernestognw

+270 -167

Fixes #3884 Fixes #3664 Currently, `MinimalForwarder` is not considered prod…uction-ready since it was designed mainly for testing purposes in the library. However, it's not that far from a MinimalForwarder that's something we may recommend. The new MinimalForwarder should guarantee: - Backwards compatibility - Security (and thorough documentation about it) - A deadline for expiring transactions #### PR Checklist - [ ] Tests - [ ] Documentation - [ ] Changeset entry (run `npx changeset add`)

Best

ratio · February 24, 2023, 2:55pm

Thanks for pointing that out. Ethers generally does not need the EIP712Domain type as input.
To double check, I wrote another test like the one used by OZ for the MinimalForwarder and it still fails =(

 it("generate signature with metamask script and test with Minimal Forwarder contract", async function() {

const { fw } = await loadFixture(deployForwarder);

//exclude chainId to make it multi-chain compatible
const EIP712DomainType: MessageTypeProperty[] = [
    { name: 'name', type: 'string' },
    { name: 'version', type: 'string' },
    { name: 'verifyingContract', type: 'address' }
]

const ForwardRequestType: MessageTypeProperty[] = [
    { name: 'from', type: 'address' },
    { name: 'to', type: 'address' },
    { name: 'value', type: 'uint256' },
    { name: 'gas', type: 'uint256' },
    { name: 'nonce', type: 'uint256' },
    { name: 'data', type: 'bytes' }
]

const types: MessageTypes = {
    EIP712Domain: EIP712DomainType,
    ForwardRequest: ForwardRequestType
}

const version: SignTypedDataVersion = "V4" as SignTypedDataVersion;

const typedData: TypedMessage<MessageTypes> = {
            types,
            domain:{
                name:"MinimalForwarder",
                version:"0.0.1",
                verifyingContract:fw.address,
            },
            primaryType:"ForwardRequest",
            message:{
                from: oc_pubKey,
                to: "0x46f4cd7c9c6aca27becf45cc5d836dddac204d32",
                value: '0',
                gas: '300000',
                nonce: Number(await fw.getNonce(oc_pubKey)),
                data: '0x',
            }
};

const sig = signTypedData({
            privateKey: Buffer.from(`${process.env.PRIVATE_KEY}`, 'hex'),
            data: typedData,
            version
})

// recover using MinimalForwarder.sol
const res = await fw.verify(typedData.message, sig);
expect(res).to.equal(true);

});

EDIT:
Ah, seems if I add the correct CHAIN_ID of the hardhat test network, it actually works!
So I assume the way to go is to change the _buildDomainSeparator function in draft-EIP712.sol accordingly. (reasoning: UX. Otherwise, users need to add a new network to Metamask, switch to it and sign the typesMsg with the correct network selected.)

Thanks again for your help, @ernestognw

ernestognw · February 24, 2023, 10:15pm

I'm glad it finally worked @ratio. I completely skipped the chainId!

So I assume the way to go is to change the _buildDomainSeparator function in draft-EIP712.sol accordingly.

Although I agree with the UX issue, I'm not sure if I'd agree with that solution, in what way would you change the _buildDomainSeparator? I'm afraid making it able to receive the chainId as a parameter would make it easy to ask users to sign replayable requests.

My feeling is that the chainId should be always gotten via JSON-RPC in tests (or directly with block.chainId in Foundry), but this one may not seem that obvious at first.