getBaseEncoder to support "uint" #4541

charlie-kim · 2024-01-13T00:12:40Z

Ethers Version

6.9.2

Search Terms

hash

Describe the Problem

getBaseEncoder function crashes when type is uint. match[2] is an empty string and it is not equal to null.

https://github.com/ethers-io/ethers.js/blob/main/src.ts/hash/typed-data.ts#L132C2-L132C2

Updating to this might work.
(match[2] == null || match[2] == '' || match[2] === String(width))

Code Snippet

No response

Contract ABI

No response

Errors

No response

Environment

node.js (v12 or newer)

Environment (Other)

No response

The text was updated successfully, but these errors were encountered:

ricmoo · 2024-01-13T02:55:53Z

That does look wrong… I’ll get that fixed right away. Thanks!

charlie-kim · 2024-01-15T14:19:10Z

@ricmoo I would much appreciate it! I am trying to deploy my code with ethers V6 as soon as possible.

ricmoo · 2024-01-15T18:34:56Z

In the meantime, you can just use "uint256" instead (which is the same thing), but I will get it out today. :)

ricmoo · 2024-01-16T04:11:43Z

I've added a fix to the repo, but want to sleep on the solution over night before publishing.

Please feel free to check ou the change above though. And all dust files have been updated, so you can try it out by installing it (via npm) from the GitHub repo.

Would love some feedback.

Basically, I normalize the types in the constructor so there is no change to the getBaseEncoder, since the types are also required in other parts of the encoder to reflect what the baseEncoder would use. I also add a case allowing for a custom type of "uint" and "int", since technically according to the spec there is no type alias, so we make the type-alias only available in the absence of ambiguity.

There are test cases too, if curious what I mean. :)

charlie-kim · 2024-01-16T17:42:56Z

Thanks. I think it should support uint[] and int[] as well.

charlie-kim · 2024-01-16T18:07:04Z

Still getting error TypeError: invalid numeric width (argument="type", value="uint", code=INVALID_ARGUMENT, version=6.10.0) with the code below

await TypedDataEncoder.resolveNames({
      name: 'Rain Collateral',
      version: '1',
      chainId: '0x7a69',
      verifyingContract: '0xCf7Ed3AccA5a467e9e704C703E8D87F634fB0Fc9',
      salt: new Uint8Array([
        61, 148, 190,  20,  21, 86,  59, 231,
        65,  41, 246, 115, 158, 73, 184,  27,
        64, 115, 249, 177, 145, 65, 205, 148,
       181, 127,  98, 129,  44, 40, 143, 144
     ])
    }, {
      Withdraw: [
        { name: 'user', type: 'address' },
        { name: 'collateral', type: 'address' },
        { name: 'asset', type: 'address' },
        { name: 'amount', type: 'uint' },
        { name: 'recipient', type: 'address' },
        { name: 'nonce', type: 'uint' },
        { name: 'expiresAt', type: 'uint' }
      ]
    }, {
      user: '0x3C44CdDdB6a900fa2b585dd299e03d12FA4293BC',
      collateral: '0x7ab4C4804197531f7ed6A6bc0f0781f706ff7953',
      asset: '0xa513E6E4b8f2a923D98304ec87F64353C4D5C853',
      amount: 1000000n,
      recipient: '0xBB1dD05e4DaaaBC9A520D7d6863D7019Aac5710f',
      nonce: 0n,
      expiresAt: 1705430871
    },
    async (v: string) => {
      return v;
    })

ricmoo · 2024-01-16T18:14:22Z

Is that from GitHub? The error indicates you are using 6.10.0, but the fix is only in 6.10.1 (which is not on npm yet).

ricmoo · 2024-01-16T18:15:25Z

You’re right. I need to add the arrayification to that. I’m going to break that out into another function, like I should’ve in the first place.

charlie-kim · 2024-01-16T19:18:09Z

Is that from GitHub? The error indicates you are using 6.10.0, but the fix is only in 6.10.1 (which is not on npm yet).

I installed as this.

"ethers": "https://github.com/ethers-io/ethers.js#43fb9c233696aeaa80b1c2b0e5fafce90e0ad508"

by running command

yarn add ethers@https://github.com/ethers-io/ethers.js#43fb9c233696aeaa80b1c2b0e5fafce90e0ad508

ricmoo · 2024-01-16T22:48:45Z

@charlie-kim I've refactored the Array parsing code. I'd love an extra set of eyes to look it over. ;)

You should be able to install the main branch. I find sometimes the hash linked to by the change doesn't work... I just tried your above example locally and it seemed happy. :)

charlie-kim · 2024-01-16T22:51:05Z

Hey @ricmoo , I just found out I was running code in lib.commonjs and it didn't have the change you made. What is the process to have the change in the code here?

ricmoo · 2024-01-16T22:53:22Z

The lib.commonjs code should be updated along with the lib.esm. Ca you check the version getting imported with console.log(ethers.version)? It should be 6.10.1 if the npm install from GitHub worked properly.

charlie-kim · 2024-01-16T23:06:34Z

The current main branch seems to have the code built for lib.commonjs. I think the current main branch is working but let me test further to make sure.

charlie-kim · 2024-01-16T23:22:17Z

FYI, my contract has signature type defined with uint and it only works with signature created by ethers.js with uint, not with uint256. I was able to get the contract validate a signature made with uint with the change you made.

It looks good to me!

ricmoo · 2024-01-17T00:28:09Z

There shouldn’t be any difference between using uint and uint256. Can you include a quick example input and output?

ricmoo · 2024-01-17T04:14:29Z

I tried your above example, swapping out the uint and uint256 and both returned the same hash.

charlie-kim · 2024-01-17T16:49:18Z

My test called TypedDataEncoder.getPayload at some point.

domain = {
      name: 'Rain Collateral',
      version: '1',
      chainId: '0x7a69',
      verifyingContract: '0xCf7Ed3AccA5a467e9e704C703E8D87F634fB0Fc9',
      salt: new Uint8Array([
        157, 160,  69, 225, 246, 213,  92, 128,
        165, 255, 236, 208, 122, 173, 169,  52,
         24, 169,  88, 221, 214, 253, 244,  10,
         90,  74, 219,  85, 247, 228,  40,  21
      ])
    }
    types = {
      Withdraw: [
        { name: 'user', type: 'address' },
        { name: 'collateral', type: 'address' },
        { name: 'asset', type: 'address' },
        { name: 'amount', type: 'uint' },
        { name: 'recipient', type: 'address' },
        { name: 'nonce', type: 'uint' },
        { name: 'expiresAt', type: 'uint' }
      ]
    };
    data = {
      user: '0x3C44CdDdB6a900fa2b585dd299e03d12FA4293BC',
      collateral: '0x7ab4C4804197531f7ed6A6bc0f0781f706ff7953',
      asset: '0xa513E6E4b8f2a923D98304ec87F64353C4D5C853',
      amount: 1000000n,
      recipient: '0x0b4Ded768d5B7B975234c7e52053E714cCBEB5a6',
      nonce: 0n,
      expiresAt: 1705512338
    }
    console.log(JSON.stringify(await TypedDataEncoder.getPayload(
      domain,
      types,
      data,
    ), null, 4));

returns

{
    "types": {
        "Withdraw": [
            {
                "name": "user",
                "type": "address"
            },
            {
                "name": "collateral",
                "type": "address"
            },
            {
                "name": "asset",
                "type": "address"
            },
            {
                "name": "amount",
                "type": "uint"
            },
            {
                "name": "recipient",
                "type": "address"
            },
            {
                "name": "nonce",
                "type": "uint"
            },
            {
                "name": "expiresAt",
                "type": "uint"
            }
        ],
        "EIP712Domain": [
            {
                "name": "name",
                "type": "string"
            },
            {
                "name": "version",
                "type": "string"
            },
            {
                "name": "chainId",
                "type": "uint256"
            },
            {
                "name": "verifyingContract",
                "type": "address"
            },
            {
                "name": "salt",
                "type": "bytes32"
            }
        ]
    },
    "domain": {
        "name": "Rain Collateral",
        "version": "1",
        "chainId": "0x7a69",
        "verifyingContract": "0xcf7ed3acca5a467e9e704c703e8d87f634fb0fc9",
        "salt": "0x9da045e1f6d55c80a5ffecd07aada93418a958ddd6fdf40a5a4adb55f7e42815"
    },
    "primaryType": "Withdraw",
    "message": {
        "user": "0x3c44cdddb6a900fa2b585dd299e03d12fa4293bc",
        "collateral": "0x7ab4c4804197531f7ed6a6bc0f0781f706ff7953",
        "asset": "0xa513e6e4b8f2a923d98304ec87f64353c4d5c853",
        "amount": "1000000",
        "recipient": "0x0b4ded768d5b7b975234c7e52053e714ccbeb5a6",
        "nonce": "0",
        "expiresAt": "1705512338"
    }
}

and

domain = {
     name: 'Rain Collateral',
     version: '1',
     chainId: '0x7a69',
     verifyingContract: '0xCf7Ed3AccA5a467e9e704C703E8D87F634fB0Fc9',
     salt: new Uint8Array([
       157, 160,  69, 225, 246, 213,  92, 128,
       165, 255, 236, 208, 122, 173, 169,  52,
        24, 169,  88, 221, 214, 253, 244,  10,
        90,  74, 219,  85, 247, 228,  40,  21
     ])
   }
   types = {
     Withdraw: [
       { name: 'user', type: 'address' },
       { name: 'collateral', type: 'address' },
       { name: 'asset', type: 'address' },
       { name: 'amount', type: 'uint256' },
       { name: 'recipient', type: 'address' },
       { name: 'nonce', type: 'uint256' },
       { name: 'expiresAt', type: 'uint256' }
     ]
   };
   data = {
     user: '0x3C44CdDdB6a900fa2b585dd299e03d12FA4293BC',
     collateral: '0x7ab4C4804197531f7ed6A6bc0f0781f706ff7953',
     asset: '0xa513E6E4b8f2a923D98304ec87F64353C4D5C853',
     amount: 1000000n,
     recipient: '0x0b4Ded768d5B7B975234c7e52053E714cCBEB5a6',
     nonce: 0n,
     expiresAt: 1705512338
   }
   console.log(JSON.stringify(await TypedDataEncoder.getPayload(
     domain,
     types,
     data,
   ), null, 4));

returns

{
    "types": {
        "Withdraw": [
            {
                "name": "user",
                "type": "address"
            },
            {
                "name": "collateral",
                "type": "address"
            },
            {
                "name": "asset",
                "type": "address"
            },
            {
                "name": "amount",
                "type": "uint256"
            },
            {
                "name": "recipient",
                "type": "address"
            },
            {
                "name": "nonce",
                "type": "uint256"
            },
            {
                "name": "expiresAt",
                "type": "uint256"
            }
        ],
        "EIP712Domain": [
            {
                "name": "name",
                "type": "string"
            },
            {
                "name": "version",
                "type": "string"
            },
            {
                "name": "chainId",
                "type": "uint256"
            },
            {
                "name": "verifyingContract",
                "type": "address"
            },
            {
                "name": "salt",
                "type": "bytes32"
            }
        ]
    },
    "domain": {
        "name": "Rain Collateral",
        "version": "1",
        "chainId": "0x7a69",
        "verifyingContract": "0xcf7ed3acca5a467e9e704c703e8d87f634fb0fc9",
        "salt": "0x9da045e1f6d55c80a5ffecd07aada93418a958ddd6fdf40a5a4adb55f7e42815"
    },
    "primaryType": "Withdraw",
    "message": {
        "user": "0x3c44cdddb6a900fa2b585dd299e03d12fa4293bc",
        "collateral": "0x7ab4c4804197531f7ed6a6bc0f0781f706ff7953",
        "asset": "0xa513e6e4b8f2a923d98304ec87f64353c4d5c853",
        "amount": "1000000",
        "recipient": "0x0b4ded768d5b7b975234c7e52053e714ccbeb5a6",
        "nonce": "0",
        "expiresAt": "1705512338"
    }
}

And contract can only validate the signature from the first one.

My test uses hardhat-ethers and it does this to create signature.

this.provider.send("eth_signTypedData_v4", [
      this.address.toLowerCase(),
      JSON.stringify(
        TypedDataEncoder.getPayload(populated.domain, types, populated.value),
        (_k, v) => {
          if (typeof v === "bigint") {
            return v.toString();
          }

          return v;
        }
      ),
    ])

charlie-kim · 2024-01-17T22:37:36Z

I can successfully test signature by using pure ethers js. But using it with hardhat-ethers does not work with the new version.

This one works.

import { Wallet } from "ethers";
const wallet = Wallet.createRandom();
const signature = await wallet.signTypedData(domain, PaySignatureTypes, data);
const recovered = verifyTypedData(domain, PaySignatureTypes, data, signature);
expect(wallet.address).to.be.eq(recovered);

This one does not work.

import { ethers } from "hardhat";
const [signer] = await ethers.getSigners();
const signature = await signer.signTypedData(domain, PaySignatureTypes, data);
const recovered = verifyTypedData(domain, PaySignatureTypes, data, signature);
expect(signer.address).to.be.eq(recovered);

The work might be on them not you.

ricmoo · 2024-01-17T22:40:31Z

I think I spotted the problem earlier. I just need to prepare a test case for the before-and-after.

I think it’s a one-line fix when preparing the payload.

Just got back from a tooth extraction though, so I’ll get to it in the next couple hours. :)

charlie-kim · 2024-01-17T22:45:33Z

Oh dang! Are you still under influence of anesthesia?? Take it easy!

ricmoo · 2024-01-18T01:37:51Z

Oh no. They just froze my mouth. No crazy anesthesia. :)

I've pushed a new version up to GitHub, if you could test against the main branch again? :)

charlie-kim · 2024-01-18T16:59:14Z

It works fine now!

charlie-kim · 2024-01-18T17:01:43Z

Somehow, the change creates an error in another test when actually calling a contract function to validate the signature. I will let you know after looking into it.

charlie-kim · 2024-01-18T20:11:51Z

@ricmoo This is a tough case for me that the latest change seems to work within the tests here. But it breaks the test that used to work in hardhat.

I am calling makePayment function in RainCollateralContract with the signature. It works without the latest change but not with it.

You can find the contract here. https://etherscan.io/address/0xE5D3d7da4b24bc9D2FDA0e206680CD8A00C0FeBD#code

const ethersTransaction = await (RainCollateralController.connect(user1Signer) as Contract).makePayment(
      rainCollateralContract.proxyAddress,
      [await USDC.getAddress()],
      [oneDollarUsdc],
      expiresAt,
      blockchainSignature.salt,
      blockchainSignature.signature
    );

Please let me know if there's any information I can provide to help debugging. I will try to go deeper on the issue as well.

ricmoo · 2024-01-18T20:52:10Z

The contract has a bug in it. When you are computing the keccak256 of a signature you must still use uint256 (not uint).

I highly recommend you let Solidity compute those for you, as it will perform all the necessary normalizations (both types, white-space, etc). Using event.selector will save you. You should be able to do:

// Define the event if not already defined
event Pay(address user,address collateral,address[] assets,uint[] amounts,uint nonce,uint expiresAt);

// Get the selector, which for an Event is a bytes32
bytes32 public constant PAY_TYPE_HASH = Pay.selector;

Make sense?

charlie-kim · 2024-01-18T21:05:58Z

I see what is happening now.

Is there any chance we can still make uint work? We got the contract audited with ethers V5. And updating contract will require another audit that is hard for me to afford.

ricmoo · 2024-01-18T21:56:44Z

Without updating the contract? It unfortunately is not possible, as no wallet would be able to produce a valid hash for that (such as MetaMask); the hash is incorrect, so software like MetaMask will correctly reject it.

The v5 Typed Encoder should not have worked either though?

Also, you should talk to your auditing company, because that is definitely something that should have been caught...

charlie-kim · 2024-01-18T22:10:32Z

Actually, the contract has been working in production about a year with V5.

We are generating signature in server side and let user call makePayment function with it directly. For metamask, the signature is only a string passed into the function. So it does not matter to metamask.

Also, V5 typed encoder uses uint to create the signature when I pass in type uint.

ricmoo · 2024-01-18T22:13:10Z

Yikes. That is definitely a bug in v5 then. The hash is not to spec, so would not work with any other (non-buggy ;)) software.

It is bad though, since it means wallets cannot accurately represent EIP-712 data to the user. It basically forces "blind signing", when it shouldn't.

charlie-kim · 2024-01-24T21:54:49Z

I will have to use v5 for now until we can upgrade the contract 😭
But the fix looks good. I am closing the issue.

Thank you!

charlie-kim · 2024-01-25T23:19:55Z

I highly recommend you let Solidity compute those for you, as it will perform all the necessary normalizations (both types, white-space, etc). Using event.selector will save you. You should be able to do:
// Define the event if not already defined
event Pay(address user,address collateral,address[] assets,uint[] amounts,uint nonce,uint expiresAt);

// Get the selector, which for an Event is a bytes32
bytes32 public constant PAY_TYPE_HASH = Pay.selector;

@ricmoo I am trying to use event selector like you suggested. But the signature cannot be validated with it. Is there a working code sample? I used solidity 0.8.23.

charlie-kim added investigate Under investigation and may be a bug. v6 Issues regarding v6 labels Jan 13, 2024

charlie-kim assigned ricmoo Jan 13, 2024

ricmoo added on-deck This Enhancement or Bug is currently being worked on. next-patch Issues scheduled for the next arch release. labels Jan 13, 2024

ricmoo added a commit that referenced this issue Jan 16, 2024

Fix EIP-712 type aliases for uint and int (#4541).

43fb9c2

ricmoo added a commit that referenced this issue Jan 16, 2024

Fixed normalization and abstracted EIP-712 Array parsing (#4541).

8f99601

ricmoo added a commit that referenced this issue Jan 18, 2024

Normalize EIP-712 types before computing the payload (#4541).

56c1361

charlie-kim closed this as completed Jan 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

getBaseEncoder to support "uint" #4541

getBaseEncoder to support "uint" #4541

charlie-kim commented Jan 13, 2024 •

edited

ricmoo commented Jan 13, 2024

charlie-kim commented Jan 15, 2024

ricmoo commented Jan 15, 2024

ricmoo commented Jan 16, 2024

charlie-kim commented Jan 16, 2024

charlie-kim commented Jan 16, 2024

ricmoo commented Jan 16, 2024

ricmoo commented Jan 16, 2024

charlie-kim commented Jan 16, 2024

ricmoo commented Jan 16, 2024

charlie-kim commented Jan 16, 2024

ricmoo commented Jan 16, 2024

charlie-kim commented Jan 16, 2024

charlie-kim commented Jan 16, 2024

ricmoo commented Jan 17, 2024

ricmoo commented Jan 17, 2024

charlie-kim commented Jan 17, 2024

charlie-kim commented Jan 17, 2024 •

edited

ricmoo commented Jan 17, 2024

charlie-kim commented Jan 17, 2024

ricmoo commented Jan 18, 2024

charlie-kim commented Jan 18, 2024 •

edited

charlie-kim commented Jan 18, 2024 •

edited

charlie-kim commented Jan 18, 2024 •

edited

ricmoo commented Jan 18, 2024

charlie-kim commented Jan 18, 2024

ricmoo commented Jan 18, 2024

charlie-kim commented Jan 18, 2024

ricmoo commented Jan 18, 2024

charlie-kim commented Jan 24, 2024

charlie-kim commented Jan 25, 2024

getBaseEncoder to support "uint" #4541

getBaseEncoder to support "uint" #4541

Comments

charlie-kim commented Jan 13, 2024 • edited

Ethers Version

Search Terms

Describe the Problem

Code Snippet

Contract ABI

Errors

Environment

Environment (Other)

ricmoo commented Jan 13, 2024

charlie-kim commented Jan 15, 2024

ricmoo commented Jan 15, 2024

ricmoo commented Jan 16, 2024

charlie-kim commented Jan 16, 2024

charlie-kim commented Jan 16, 2024

ricmoo commented Jan 16, 2024

ricmoo commented Jan 16, 2024

charlie-kim commented Jan 16, 2024

ricmoo commented Jan 16, 2024

charlie-kim commented Jan 16, 2024

ricmoo commented Jan 16, 2024

charlie-kim commented Jan 16, 2024

charlie-kim commented Jan 16, 2024

ricmoo commented Jan 17, 2024

ricmoo commented Jan 17, 2024

charlie-kim commented Jan 17, 2024

charlie-kim commented Jan 17, 2024 • edited

ricmoo commented Jan 17, 2024

charlie-kim commented Jan 17, 2024

ricmoo commented Jan 18, 2024

charlie-kim commented Jan 18, 2024 • edited

charlie-kim commented Jan 18, 2024 • edited

charlie-kim commented Jan 18, 2024 • edited

ricmoo commented Jan 18, 2024

charlie-kim commented Jan 18, 2024

ricmoo commented Jan 18, 2024

charlie-kim commented Jan 18, 2024

ricmoo commented Jan 18, 2024

charlie-kim commented Jan 24, 2024

charlie-kim commented Jan 25, 2024

charlie-kim commented Jan 13, 2024 •

edited

charlie-kim commented Jan 17, 2024 •

edited

charlie-kim commented Jan 18, 2024 •

edited

charlie-kim commented Jan 18, 2024 •

edited

charlie-kim commented Jan 18, 2024 •

edited