GPv2Signing Manual Review Findings

GPS-01M: Inexistent Cross-Chain Replay Attack Protection

Type	Severity	Location
Logical Fault		GPv2Signing.sol:L53, L72-L80

Description:

The GPv2Signing contract makes use of a domainSeparator that is calculated only once during the construction of the contract. As such, cross-chain replay attacks could become feasible if the contract is deployed across multiple chains at the same address.

Impact:

The severity of this exhibit will be adjusted depending on whether it is the Gnosis Guild team's intention to deploy the system across multiple chains.

Example:

Copy
48/// @dev The domain separator used for signing orders that gets mixed in
49/// making signatures for different domains incompatible. This domain
50/// separator is computed following the EIP-712 standard and has replay
51/// protection mixed in so that signed orders are only valid for specific
52/// GPv2 contracts.
53bytes32 public immutable domainSeparator;
54
55/// @dev Storage indicating whether or not an order has been signed by a
56/// particular address.
57mapping(bytes => uint256) public preSignature;
58
59/// @dev Event that is emitted when an account either pre-signs an order or
60/// revokes an existing pre-signature.
61event PreSignature(address indexed owner, bytes orderUid, bool signed);
62
63constructor() {
64    // NOTE: Currently, the only way to get the chain ID in solidity is
65    // using assembly.
66    uint256 chainId;
67    // solhint-disable-next-line no-inline-assembly
68    assembly {
69        chainId := chainid()
70    }
71
72    domainSeparator = keccak256(
73        abi.encode(
74            DOMAIN_TYPE_HASH,
75            DOMAIN_NAME,
76            DOMAIN_VERSION,
77            chainId,
78            address(this)
79        )
80    );
81}

Recommendation:

We advise the Gnosis Guild team to ensure that the contract will solely be deployed in the network it is intended to (Ethereum).

If the contract is expected to be deployed across multiple chains, we advise a caching system to be utilized for the domain separator similarly to OpenZeppelin's EIP712 implementation.

Alleviation (da3062f6b3ff452092a0b6daa6f226f0f3b696c6):

The Gnosis Guild team evaluated this exhibit and considered out-of-scope as it relates to the Cowswap codebase which the Gnosis Guild team wishes to leave as untouched as possible.

To this end, we consider this exhibit as safely acknowledged by the Gnosis Guild team given that they do not intend to deploy the contract directly but rather interact with it.