EIP712Lib Manual Review Findings

EIP-01M: Inexistent Validation of Meta-TX Sender Validity

Type	Severity	Location
Language Specific		EIP712Lib.sol:L102

Description:

The sender of a particular transaction should at all times not be equal to the zero-address as a standard throughout the ecosystem due to the said address' special purpose.

Example:

Copy
94/**
95 * @notice Returns the current sender address.
96 */
97function msgSender() internal view returns (address) {
98    bool isItAMetaTransaction = ProtocolLib.protocolMetaTxInfo().isMetaTransaction;
99
100    // Get sender from the storage if this is a meta transaction
101    if (isItAMetaTransaction) {
102        return getCurrentSenderAddress();
103    } else {
104        return msg.sender;
105    }
106}

Recommendation:

We advise such a check to be imposed here by mandating that getCurrentSenderAddress is non-zero as a case of isItAMetaTransaction and getCurrentSenderAddress being zero should be considered invalid.

Alleviation (44009967e4f68092941d841e9e0f5dd2bb31bf0b):

The result of getCurrentSenderAddress is now properly validated to be non-zero in conformance with the EVM ecosystem's standards.

EIP-02M: Improper Domain Separator Retrieval

Type	Severity	Location
Language Specific		EIP712Lib.sol:L67-L72

Description:

The EIP712Lib contract contains functions that dynamically construct the currently valid domain separator (i.e. domainSeparator) yet when getDomainSeparator is invoked the last value stored to domainSeparator is retrieved instead. As evidenced in the ConfigHandlerFacet contract that initializes this value, the domainSeparator is assigned to once thus permitting cross-chain replay attacks to occur, a trait especially relevant with the upcoming Ethereum hard fork.

Impact:

As the domainSeparator currently remains the same regardless of whether the blockchain the contract was deployed in forked, it is possible for a signed EIP-712 payload that is submitted on the original chain to be replayed on the forked chain improperly.

Example:

Copy
67/**
68 * @notice Get the domain separator.
69 */
70function getDomainSeparator() private view returns (bytes32) {
71    return ProtocolLib.protocolMetaTxInfo().domainSeparator;
72}

Recommendation:

We advise a caching system to be used instead similarly to the draft EIP-712 implementation by OpenZeppelin whereby the domain separator is re-calculated on a need-to basis by comparing the current block.chainid and the one that the original domainSeparator was calculated in, allowing the separator to be re-calculated in case the chain IDs do not match (i.e. due to the PoW chain of Ethereum being used instead of the PoS one).

Alleviation (44009967e4f68092941d841e9e0f5dd2bb31bf0b):

A proxy-compliant caching system is now utilized that builds the domain separator on a need-to basis depending on whether the chain ID identified during the deployment of the contract has changed in the current execution environment. As a result, we consider this exhibit alleviated in full.

EIP-03M: Insecure Elliptic Curve Recovery Mechanism

Type	Severity	Location
Language Specific		EIP712Lib.sol:L59, L60

Description:

The ecrecover function is a low-level cryptographic function that should be utilized after appropriate sanitizations have been enforced on its arguments, namely on the s and v values. This is due to the inherent trait of the curve to be symmetrical on the x-axis and thus permitting signatures to be replayed with the same x value (r) but a different y value (s).

Impact:

Should the payload being verified by the signature rely on differentiation based on the s or v arguments, it will be possible to replay the signature for the same data validly and acquire authorization twice.

Example:

Copy
55function verify(
56    address _user,
57    bytes32 _hashedMetaTx,
58    bytes32 _sigR,
59    bytes32 _sigS,
60    uint8 _sigV
61) internal view returns (bool) {
62    address signer = ecrecover(toTypedMessageHash(_hashedMetaTx), _sigV, _sigR, _sigS);
63    require(signer != address(0), INVALID_SIGNATURE);
64    return signer == _user;
65}

Recommendation:

We advise them to be sanitized by ensuring that v is equal to either 27 or 28 (v ∈ {27, 28}) and to ensure that s is existent in the lower half order of the elliptic curve (0 < s < secp256k1n ÷ 2 + 1) by ensuring it is less than 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A1. A reference implementation of those checks can be observed in the ECDSA library of OpenZeppelin and the rationale behind those restrictions exists within Appendix F of the Yellow Paper.

Alleviation (44009967e4f68092941d841e9e0f5dd2bb31bf0b):

The _sigS and _sigV values are now adequately validated within the verify function according to the EVM yellow-paper standards.