EIP712: _TYPE_HASH including salt

[pcaversaccio]

Since EIP-5267 got merged I wanted to raise a question. In EIP-712's definition of the domainSeparator a so-called salt is also listed:

bytes32 salt an disambiguating salt for the protocol. This can be used as a domain separator of last resort.

EIP-5267 also supports the salt parameters for retrieval via the fields parameter. Now, my question: shouldn't we adjust the EIP712 to incorporate this salt parameter?

I.e.:

- bytes32 private constant _TYPE_HASH = keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)");
+ bytes32 private constant _TYPE_HASH = keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract,bytes32 salt)");

and

function _buildDomainSeparator(bytes32 salt) private view returns (bytes32) {
    return keccak256(abi.encode(_TYPE_HASH, _hashedName, _hashedVersion, block.chainid, address(this), salt));
}

The above are illustrations and not final code suggestions.

[Amxx]

Hello @pcaversaccio

IMO most cases don't need the salt. In fact, the only use of salt I've seen in production is in some polygon contract that used chainId as the salt (and did not include chainId).

I wouldn't want to force that onto any user, particularly without a good documentation explaining when/why use the salt.

On the other hand, it would be better if our implementation could allow someone that want to include a salt to do so.

[pcaversaccio]

On the other hand, it would be better if our implementation could allow someone that want to include a salt to do so.

I think OpenZeppelin is optimising for security, correctness and completeness (to a certain extend). Thus, I definitely believe OZ as the most common Solidity smart contract library should allow for such a customisation if someone would like to include the salt.

[Amxx]

We could have a version that does

    function _buildDomainSeparator() private view returns (bytes32) {
        return _salt == bytes32(0)
            ? keccak256(abi.encode(_TYPE_HASH,           _hashedName, _hashedVersion, block.chainid, address(this)))
            : keccak256(abi.encode(_TYPE_HASH_WITH_SALT, _hashedName, _hashedVersion, block.chainid, address(this), _salt));
    }
    
    function eip712Domain()
        public
        view
        virtual
        override
        returns (
            bytes1 fields,
            string memory name,
            string memory version,
            uint256 chainId,
            address verifyingContract,
            bytes32 salt,
            uint256[] memory extensions
        )
    {
        return (
            _salt == bytes32(0) ? hex"0f" : hex"1f", // 01111 or 11111
            _name.toStringWithFallback(_nameFallback),
            _version.toStringWithFallback(_versionFallback),
            block.chainid,
            address(this),
            _salt,
            new uint256[](0)
        );
    }

If _salt is immutable, we would be breaking the constructor's argument.

We could declare a

function _salt() internal pure virtual returns (bytes32) { return bytes32(0); }

or

function _salt() internal pure virtual returns (bool enabled, bytes32 value) { return (false, bytes32(0)); }

that users can overload. I'm wondering if pure gives enough guarantees (it should be constant in time)

@frangio, what do you think ?

[Amxx]

One more thing we need to discuss is:
Are there any users that require this feature, or is the need purely theoretical?

If we don't see any real need for a salt in the EIP712 domain, we probably shouldn't add unnecessary complexity to the code.

[pcaversaccio]

absolutely - fwiw this is for instance what revoke.cash does (so apparently many use cases use the chainId as salt):

const salt = utils.hexZeroPad(utils.hexlify(chainId), 32);

// Given the potential fields of a domain, we try to find the one that matches the domain separator
const potentialDomains = [
  // Expected domain separators
  { name, version, chainId, verifyingContract },
  { name, version, verifyingContract, salt },
  { name: symbol, version, chainId, verifyingContract },
  { name: symbol, version, verifyingContract, salt },

  // Without version
  { name, chainId, verifyingContract },
  { name, verifyingContract, salt },
  { name: symbol, chainId, verifyingContract },
  { name: symbol, verifyingContract, salt },

  // Without name
  { version, chainId, verifyingContract },
  { version, verifyingContract, salt },

  // Without name or version
  { chainId, verifyingContract },
  { verifyingContract, salt },

  // With both chainId and salt
  { name, version, chainId, verifyingContract, salt },
  { name: symbol, version, chainId, verifyingContract, salt },
];

const domain = potentialDomains.find((domain) => utils._TypedDataEncoder.hashDomain(domain) === domainSeparator);

@rkalis sorry for tagging but you might have some input here about use cases you have seen with revoke.cash.

[Amxx]

I see no case when both chainId and salt are used ... and I would say that using the chainId as salt is bad design !

If that is the only real usecase, then I'd say we should stick with including the chainId for everyone and not including the salt.

[pcaversaccio]

A recent example where the block.prevrandao is used for the salt is the crvUSD Stablecoin:

It's written in Vyper but I simply wanted to highlight that there are projects out there using salt in different ways.

[Amxx]

I'm sure you can find example of projects using a salt ... but WHY are they doing that ?

I don't want an example of project that do it. I want a good reason for projects doing it. I want a rational for our users to clearly decide if they need it of not.

[frangio]

I believe the reason why people use salt instead of chainId is that they want to allow users to create signatures without switching networks in their wallet. If the domain separator contains the chainId parameter the wallet verifies that it is the network that they are connected to. By putting the chain id in salt instead, the signature remains non-replayable but works without switching networks. I recall someone mentioning this to me, but we should confirm. In any case, it sounds like a UX problem the wallets need to solve, that the contracts are understandably implementing workarounds for. We generally avoid implementing this kind of workaround though, I think it has the potential to keep the ecosystem behind instead of pushing towards solving these things where they should be.

Even if we add the salt in the way that @Amxx suggests, then people will want to remove chainId, or version or any other parameter. So this approach does not scale.

Most of what our EIP712 contract is doing is optimizing the way the values are cached, and making sure it behaves properly in a hard fork. I'd propose to keep the EIP712 contract as is, with the fields that we've been using so far, which seem to work for most users.

For supporting the more advanced use cases, we could have a set of utilities for implementing an alternative to our EIP712 contract but with different fields. In particular, we can have a function that converts the EIP-5267 descriptor into the domain separator hash. Since Solidity functions can't accept tuples as argument I think the best way would be:

function toDomainSeparator(
    function () view returns (bytes1, string memory, string memory, uint256, address, bytes32, uint256[] memory) eip712Domain
) internal returns (bytes32) {
    ...
}

In this way a user that wants a different set of EIP-712 fields can just implement the EIP-5267 eip712Domain function and pass that function pointer in to get the domain separator hash, then they can take care of caching it and we can provide instructions to handle a hard fork.

I would propose to consider this for 5.1, but it might make sense to look at one thing for 5.0: our ECDSA library currently contains toEthSignedMessageHash, toTypedDataHash, and toDataWithIntendedValidatorHash. We might want to move these out of the ECDSA library into their own SignatureUtils library. In this library we could add the toDomainSeparator function I described above.

---

I'm wondering if pure gives enough guarantees (it should be constant in time)

There is no reason why it should be constant in time AFAIK.

[pcaversaccio]

I believe the reason why people use salt instead of chainId is that they want to allow users to create signatures without switching networks in their wallet. If the domain separator contains the chainId parameter the wallet verifies that it is the network that they are connected to. By putting the chain id in salt instead, the signature remains non-replayable but works without switching networks. I recall someone mentioning this to me, but we should confirm. In any case, it sounds like a UX problem the wallets need to solve, that the contracts are understandably implementing workarounds for. We generally avoid implementing this kind of workaround though, I think it has the potential to keep the ecosystem behind instead of pushing towards solving these things where they should be.

@tayvano or @danfinlay could you confirm that MetaMask verifies the chainId parameter in the domain separator is the network that the user is connected to?

In this way a user that wants a different set of EIP-712 fields can just implement the EIP-5267 eip712Domain function and pass that function pointer in to get the domain separator hash, then they can take care of caching it and we can provide instructions to handle a hard fork.

I think that approach is indeed a good compromise and scales well. Another way would be by using a struct and passing this argument (am not yet sure about the gas implications of the function pointer approach):

struct Domain {
  bytes1 fields;
  string name;
  string version;
  uint256 chainId;
  address verifyingContract;
  bytes32 salt;
  uint256[] extensions;
}

function toDomainSeparator(Domain calldata domain) internal returns (bytes32) {
    ...
}

[ernestognw]

I believe the reason why people use salt instead of chainId is that they want to allow users to create signatures without switching networks in their wallet.

Another good related argument towards adding the salt is that some hardware wallet providers may restrict signing with certain chainId under paid plans. For example, a provider allowing to sign transactions/messages only for testnet, and enabling mainnet for a monthly fee subscription.

[pcaversaccio]

a provider allowing to sign transactions/messages only for testnet, and enabling mainnet for a monthly fee subscription.

Good point - tbh that sounds crazy and would mean it's not anymore fully non-custodial. If this ever happens, I will fire fiercely against these providers; that would be unacceptable behaviour.