Wrong Implementation of EIP-712

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/libraries/TransactionHelper.sol#L81

Vulnerability details

Impact

The EIP-712 uses several parameters.

Those parameters are exactly:

 EIP712Domain {
 string name;
string version;
uint256 chainId;
address verifyingContract;
}

As you can see on the following Domain, ZkSync, is missing one parameter:

 bytes32 constant EIP712_DOMAIN_TYPEHASH = keccak256("EIP712Domain(string name,string 
  version,uint256 chainId)");

which is the verifyingContract.

missing the contract in the EIP-712, would cause to open an attack vector such has signature replay. As the contract does not hash the address(this) to generate the signature and recover from it, it is causing this potential signature replay across contracts from the same chainId.

That will also brick the _encodeHashEIP712Transaction function because the hash returned will not be the correct one:

 return keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash));

Proof of Concept

Tools Used

Manual

Recommended Mitigation Steps

Add the address(this) in the TYPEHASH to protect against replay attacks

bytes32 constant EIP712_DOMAIN_TYPEHASH = keccak256("EIP712Domain(string name,string version,uint256 chainId, address contract)");

[GalloDaSballo]

On one hand the parameters are optional

On the other hand it seems like this is valid

[c4-judge]

GalloDaSballo marked the issue as primary issue

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)

[c4-judge]

GalloDaSballo marked the issue as duplicate of #106

[c4-sponsor]

vladbochok marked the issue as sponsor disputed

[vladbochok]

EIP-712 does not require a verifyingContract parameter. It is optional.

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

Also, there is no room for the replay attack.

[c4-sponsor]

vladbochok requested judge review

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid