ERC20Permit Manual Review Findings

ERP-01M: Insecure Elliptic Curve Recovery Mechanism

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).

Example:

Copy
35/**
36 * @dev See {IERC2612Permit-permit}.
37 *
38 */
39function permit(
40    address owner,
41    address spender,
42    uint256 amount,
43    uint256 deadline,
44    uint8 v,
45    bytes32 r,
46    bytes32 s
47) public virtual override {
48    require(block.timestamp <= deadline, "Permit: expired deadline");
49
50    bytes32 hashStruct =
51        keccak256(abi.encode(PERMIT_TYPEHASH, owner, spender, amount, _nonces[owner].current(), deadline));
52
53    bytes32 _hash = keccak256(abi.encodePacked(uint16(0x1901), DOMAIN_SEPARATOR, hashStruct));
54
55    address signer = ecrecover(_hash, v, r, s);
56    require(signer != address(0) && signer == owner, "ZeroSwapPermit: Invalid signature");
57
58    _nonces[owner].increment();
59    _approve(owner, spender, amount);
60}

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:

The code was adjusted to instead utilize the ECDSA library from OpenZeppelin directly that imposes the necessary sanitizations on the v and s arguments of the ecrecover function.

ERP-02M: Cross-Chain Signature Replay Attack Susceptibility

Description:

The DOMAIN_SEPARATOR used in conjunction with the EIP-712 standard is calculated once during the constructor of the contract. As this calculation involves the execution context's chainid, blockchain forks will allow signatures to be replayed across chains as the chainid is consequently not validated.

Example:

Copy
19constructor() {
20
21    uint256 chainID;
22    assembly {
23        chainID := chainid()
24    }
25
26    DOMAIN_SEPARATOR = keccak256(abi.encode(
27        keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"),
28        keccak256(bytes(name())),
29        keccak256(bytes("1")), // Version
30        chainID,
31        address(this)
32    ));
33}

Recommendation:

We advise a caching mechanism to be imposed here instead whereby the DOMAIN_SEPARATOR is stored to an immutable contract-level variable and is utilized only when the cached chainid (also stored in an immutable variable) matches the current execution context's chainid. A reference implementation of this paradigm can be observed at the draft-EIP712.sol implementation of OpenZeppelin.

Alleviation:

The code took inspiration from the draft-EIP712.sol implementation of OpenZeppelin and now uses a caching system that dynamically calculates the separator as needed depending on the evaluation of the current chainID, thereby alleviating this exhibit.