EIP712 domain values not set in constructor

[hats-bug-reporter[bot]]

Github username: -- Twitter username: 97Sabit Submission hash (on-chain): 0x7ea83d68ba548c68309dd130c561748e67e1022e4b8e350f57ea32d35572a892 Severity: medium

Description: Description\ Here's what EIP712 says:

Protocol designers only need to include the fields that make sense for their signing domain. Unused fields are left out of the struct type.

string name the user readable name of signing domain, i.e. the name of the DApp or the protocol. string version the current major version of the signing domain. Signatures from different versions are not compatible.

https://eips.ethereum.org/EIPS/eip-712#definition-of-domainseparator[EIP-712]

And here's the comment on EIP712.sol:

• name: the user readable name of the signing domain, i.e. the name of the DApp or the protocol.
• • version: the current major version of the signing domain.

However, domainName and version has been included in EIP712Verifier.sol as parameters but were never set.

The EIP712Verifier constructor does not pass any concrete values for domainName and version to the EIP712 parent constructor. For example:

constructor(string memory domainName, string memory version, address signer) EIP712(domainName, version) {

} 

Because no actual values are passed, the domainName and version used in the parent contract will be empty strings by default.

For example, valid values could be:

• domainName = "MyDApp"
• version = "1.0"

But with the current constructor, these would be set to empty strings.

This is a problem because the EIP-712 domain separator is constructed using the domainName and version. If they are empty strings, it could allow for replay attacks across different DApps.

Recommendation:
Pass actual domainName and version values in the EIP712Verifier constructor

1. Proof of Concept (PoC) File

   https://github.com/hats-finance/Blast-Futures-Exchange-0x97895c329b950755566ddcdad3395caaea395074/blob/d9b402f5124f1470f979ed9a6d010d89988f6dee/foundry/src/EIP712Verifier.sol#L10

[bahurum]

The domainName and version arguments will have the values provided in the constructor of EIP712Verifier, so they won't be empty.

[alex-sumner]

Values for domainName and version are passed from the Bfx constructor to the EIP712Verifier constructor and on to to the EIP712 constructor. Specifically the values "BfxWithdrawal" and "1" are passed.

[ololade97]

As it is, Bfx and EIP712Verifier are two separate contracts that use EIP712.sol.

It's true Bfx passed "Withdrawal" and "1". What about EIP712Verifier contract?

[ololade97]

The domainName and version arguments will have the values provided in the constructor of EIP712Verifier, so they won't be empty.

domainName and version are not stored in any variable in EIP712Verifier.

[alex-sumner]

Bfx and EIP712Verifier are not two separate contracts, Bfx extends EIP712Verifier which extends EIP712. The values domainName and version are passed down the chain of constructors to EIP712.

The EIP712Verifier constructor receives them from the Bfx constructor and passes them on to the EIP712 constructor. It does not need to store them anywhere.

[ololade97]

EIP712Verifier isn't an abstract contract. It's a contract on its own. It's allowed for contracts to inherit each other, but that doesn't make them one same contract.

The samething arises here.

[ololade97]

In solidity, a parent contract doesn't inherit from a child contract. Inheritance in solidity only works in one direction - parent to child.