From 0ec0371cd8d0eeeaee07811e3b3a24a241d6fbb4 Mon Sep 17 00:00:00 2001 From: Kirill Fedoseev Date: Mon, 19 Sep 2022 22:02:08 +0200 Subject: [PATCH] Secure handling of chainid in EIP712 domains --- foundry.toml | 1 + src/interfaces/IERC20Permit.sol | 19 +++- src/token/ERC20Permit.sol | 32 +++--- src/utils/EIP712.sol | 107 ++++++++++++++++++ src/zkbob/utils/ZkBobAccounting.sol | 5 +- .../verifiers/prodV1/TransferVerifier.sol | 4 +- .../verifiers/prodV1/TreeUpdateVerifier.sol | 4 +- .../verifiers/stageV1/TransferVerifier.sol | 4 +- .../verifiers/stageV1/TreeUpdateVerifier.sol | 4 +- test/BobToken.t.sol | 20 ++++ 10 files changed, 177 insertions(+), 23 deletions(-) create mode 100644 src/utils/EIP712.sol diff --git a/foundry.toml b/foundry.toml index 49c39d3..638ef76 100644 --- a/foundry.toml +++ b/foundry.toml @@ -6,5 +6,6 @@ fs_permissions = [{ access = "read", path = "./out"}] [fmt] line_length = 120 +multiline_func_header = 'all' # See more config options https://github.com/foundry-rs/foundry/tree/master/config diff --git a/src/interfaces/IERC20Permit.sol b/src/interfaces/IERC20Permit.sol index 4100abf..7fd4e42 100644 --- a/src/interfaces/IERC20Permit.sol +++ b/src/interfaces/IERC20Permit.sol @@ -3,7 +3,15 @@ pragma solidity 0.8.15; interface IERC20Permit { - function permit(address owner, address spender, uint256 value, uint256 deadline, uint8 v, bytes32 r, bytes32 s) + function permit( + address owner, + address spender, + uint256 value, + uint256 deadline, + uint8 v, + bytes32 r, + bytes32 s + ) external; function nonces(address owner) external view returns (uint256); @@ -14,7 +22,14 @@ interface IERC20Permit { function SALTED_PERMIT_TYPEHASH() external view returns (bytes32); - function receiveWithPermit(address _holder, uint256 _value, uint256 _deadline, uint8 _v, bytes32 _r, bytes32 _s) + function receiveWithPermit( + address _holder, + uint256 _value, + uint256 _deadline, + uint8 _v, + bytes32 _r, + bytes32 _s + ) external; function saltedPermit( diff --git a/src/token/ERC20Permit.sol b/src/token/ERC20Permit.sol index 1eda073..3cc0c12 100644 --- a/src/token/ERC20Permit.sol +++ b/src/token/ERC20Permit.sol @@ -5,13 +5,12 @@ pragma solidity 0.8.15; import "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; import "../interfaces/IERC20Permit.sol"; import "./BaseERC20.sol"; +import "../utils/EIP712.sol"; /** * @title ERC20Permit */ -abstract contract ERC20Permit is BaseERC20, IERC20Permit { - // EIP712 domain separator - bytes32 public immutable DOMAIN_SEPARATOR; +abstract contract ERC20Permit is IERC20Permit, BaseERC20, EIP712 { // EIP2612 permit typehash bytes32 public constant PERMIT_TYPEHASH = keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)"); @@ -23,16 +22,10 @@ abstract contract ERC20Permit is BaseERC20, IERC20Permit { mapping(address => uint256) public nonces; - constructor(address _self) { - DOMAIN_SEPARATOR = keccak256( - abi.encode( - keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"), - keccak256(bytes(name())), - keccak256("1"), - block.chainid, - _self - ) - ); + constructor(address _self) EIP712(_self, name(), "1") {} + + function DOMAIN_SEPARATOR() external view override returns (bytes32) { + return _domainSeparatorV4(); } /** @@ -67,7 +60,14 @@ abstract contract ERC20Permit is BaseERC20, IERC20Permit { /** * @dev Cheap shortcut for making sequential calls to permit() + transferFrom() functions. */ - function receiveWithPermit(address _holder, uint256 _value, uint256 _deadline, uint8 _v, bytes32 _r, bytes32 _s) + function receiveWithPermit( + address _holder, + uint256 _value, + uint256 _deadline, + uint8 _v, + bytes32 _r, + bytes32 _s + ) public virtual { @@ -140,7 +140,7 @@ abstract contract ERC20Permit is BaseERC20, IERC20Permit { uint256 nonce = nonces[_holder]++; bytes32 digest = ECDSA.toTypedDataHash( - DOMAIN_SEPARATOR, keccak256(abi.encode(PERMIT_TYPEHASH, _holder, _spender, _value, nonce, _deadline)) + _domainSeparatorV4(), keccak256(abi.encode(PERMIT_TYPEHASH, _holder, _spender, _value, nonce, _deadline)) ); require(_holder == ECDSA.recover(digest, _v, _r, _s), "ERC20Permit: invalid ERC2612 signature"); @@ -162,7 +162,7 @@ abstract contract ERC20Permit is BaseERC20, IERC20Permit { uint256 nonce = nonces[_holder]++; bytes32 digest = ECDSA.toTypedDataHash( - DOMAIN_SEPARATOR, + _domainSeparatorV4(), keccak256(abi.encode(SALTED_PERMIT_TYPEHASH, _holder, _spender, _value, nonce, _deadline, _salt)) ); diff --git a/src/utils/EIP712.sol b/src/utils/EIP712.sol new file mode 100644 index 0000000..b03415c --- /dev/null +++ b/src/utils/EIP712.sol @@ -0,0 +1,107 @@ +// SPDX-License-Identifier: MIT + +pragma solidity 0.8.15; + +import "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; + +/** + * @dev https://eips.ethereum.org/EIPS/eip-712[EIP 712] is a standard for hashing and signing of typed structured data. + * + * The encoding specified in the EIP is very generic, and such a generic implementation in Solidity is not feasible, + * thus this contract does not implement the encoding itself. Protocols need to implement the type-specific encoding + * they need in their contracts using a combination of `abi.encode` and `keccak256`. + * + * This contract implements the EIP 712 domain separator ({_domainSeparatorV4}) that is used as part of the encoding + * scheme, and the final step of the encoding to obtain the message digest that is then signed via ECDSA + * ({_hashTypedDataV4}). + * + * The implementation of the domain separator was designed to be as efficient as possible while still properly updating + * the chain id to protect against replay attacks on an eventual fork of the chain. + * + * NOTE: This contract implements the version of the encoding known as "v4", as implemented by the JSON RPC method + * https://docs.metamask.io/guide/signing-data.html[`eth_signTypedDataV4` in MetaMask]. + * + * Adapted from OpenZeppelin library to support address(this) overrides in proxy implementations. + */ +abstract contract EIP712 { + /* solhint-disable var-name-mixedcase */ + // Cache the domain separator as an immutable value, but also store the chain id that it corresponds to, in order to + // invalidate the cached domain separator if the chain id changes. + bytes32 private immutable _CACHED_DOMAIN_SEPARATOR; + uint256 private immutable _CACHED_CHAIN_ID; + address private immutable _CACHED_THIS; + + bytes32 private immutable _HASHED_NAME; + bytes32 private immutable _HASHED_VERSION; + bytes32 private immutable _TYPE_HASH; + + /* solhint-enable var-name-mixedcase */ + + /** + * @dev Initializes the domain separator and parameter caches. + * + * The meaning of `name` and `version` is specified in + * https://eips.ethereum.org/EIPS/eip-712#definition-of-domainseparator[EIP 712]: + * + * - `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. + * + * NOTE: These parameters cannot be changed except through a xref:learn::upgrading-smart-contracts.adoc[smart + * contract upgrade]. + */ + constructor(address self, string memory name, string memory version) { + bytes32 hashedName = keccak256(bytes(name)); + bytes32 hashedVersion = keccak256(bytes(version)); + bytes32 typeHash = + keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"); + _HASHED_NAME = hashedName; + _HASHED_VERSION = hashedVersion; + _CACHED_CHAIN_ID = block.chainid; + _CACHED_DOMAIN_SEPARATOR = _buildDomainSeparator(typeHash, hashedName, hashedVersion, self); + _CACHED_THIS = self; + _TYPE_HASH = typeHash; + } + + /** + * @dev Returns the domain separator for the current chain. + */ + function _domainSeparatorV4() internal view returns (bytes32) { + if (address(this) == _CACHED_THIS && block.chainid == _CACHED_CHAIN_ID) { + return _CACHED_DOMAIN_SEPARATOR; + } else { + return _buildDomainSeparator(_TYPE_HASH, _HASHED_NAME, _HASHED_VERSION, address(this)); + } + } + + function _buildDomainSeparator( + bytes32 typeHash, + bytes32 nameHash, + bytes32 versionHash, + address self + ) + private + view + returns (bytes32) + { + return keccak256(abi.encode(typeHash, nameHash, versionHash, block.chainid, self)); + } + + /** + * @dev Given an already https://eips.ethereum.org/EIPS/eip-712#definition-of-hashstruct[hashed struct], this + * function returns the hash of the fully encoded EIP712 message for this domain. + * + * This hash can be used together with {ECDSA-recover} to obtain the signer of a message. For example: + * + * ```solidity + * bytes32 digest = _hashTypedDataV4(keccak256(abi.encode( + * keccak256("Mail(address to,string contents)"), + * mailTo, + * keccak256(bytes(mailContents)) + * ))); + * address signer = ECDSA.recover(digest, signature); + * ``` + */ + function _hashTypedDataV4(bytes32 structHash) internal view virtual returns (bytes32) { + return ECDSA.toTypedDataHash(_domainSeparatorV4(), structHash); + } +} diff --git a/src/zkbob/utils/ZkBobAccounting.sol b/src/zkbob/utils/ZkBobAccounting.sol index 0262a11..3ec2d6f 100644 --- a/src/zkbob/utils/ZkBobAccounting.sol +++ b/src/zkbob/utils/ZkBobAccounting.sol @@ -120,7 +120,10 @@ contract ZkBobAccounting { }); } - function _recordOperation(address _user, int256 _txAmount) + function _recordOperation( + address _user, + int256 _txAmount + ) internal returns (uint56 maxWeeklyAvgTvl, uint32 maxWeeklyTxCount, uint256 txCount) { diff --git a/src/zkbob/verifiers/prodV1/TransferVerifier.sol b/src/zkbob/verifiers/prodV1/TransferVerifier.sol index 11bde4a..a9952c1 100644 --- a/src/zkbob/verifiers/prodV1/TransferVerifier.sol +++ b/src/zkbob/verifiers/prodV1/TransferVerifier.sol @@ -216,6 +216,8 @@ contract TransferVerifier { require(input[i] < SNARK_SCALAR_FIELD, "verifier-gte-snark-scalar-field"); vk_x = Pairing.plus(vk_x, Pairing.scalar_mul(vk.IC[i + 1], input[i])); } - return Pairing.pairing(Pairing.negate(_proof.A), _proof.B, vk.alfa1, vk.beta2, vk_x, vk.gamma2, _proof.C, vk.delta2); + return Pairing.pairing( + Pairing.negate(_proof.A), _proof.B, vk.alfa1, vk.beta2, vk_x, vk.gamma2, _proof.C, vk.delta2 + ); } } diff --git a/src/zkbob/verifiers/prodV1/TreeUpdateVerifier.sol b/src/zkbob/verifiers/prodV1/TreeUpdateVerifier.sol index ae5e663..2e6fd81 100644 --- a/src/zkbob/verifiers/prodV1/TreeUpdateVerifier.sol +++ b/src/zkbob/verifiers/prodV1/TreeUpdateVerifier.sol @@ -208,6 +208,8 @@ contract TreeUpdateVerifier { require(input[i] < SNARK_SCALAR_FIELD, "verifier-gte-snark-scalar-field"); vk_x = Pairing.plus(vk_x, Pairing.scalar_mul(vk.IC[i + 1], input[i])); } - return Pairing.pairing(Pairing.negate(_proof.A), _proof.B, vk.alfa1, vk.beta2, vk_x, vk.gamma2, _proof.C, vk.delta2); + return Pairing.pairing( + Pairing.negate(_proof.A), _proof.B, vk.alfa1, vk.beta2, vk_x, vk.gamma2, _proof.C, vk.delta2 + ); } } diff --git a/src/zkbob/verifiers/stageV1/TransferVerifier.sol b/src/zkbob/verifiers/stageV1/TransferVerifier.sol index fd3bfa9..a6b9788 100644 --- a/src/zkbob/verifiers/stageV1/TransferVerifier.sol +++ b/src/zkbob/verifiers/stageV1/TransferVerifier.sol @@ -216,6 +216,8 @@ contract TransferVerifier { require(input[i] < SNARK_SCALAR_FIELD, "verifier-gte-snark-scalar-field"); vk_x = Pairing.plus(vk_x, Pairing.scalar_mul(vk.IC[i + 1], input[i])); } - return Pairing.pairing(Pairing.negate(_proof.A), _proof.B, vk.alfa1, vk.beta2, vk_x, vk.gamma2, _proof.C, vk.delta2); + return Pairing.pairing( + Pairing.negate(_proof.A), _proof.B, vk.alfa1, vk.beta2, vk_x, vk.gamma2, _proof.C, vk.delta2 + ); } } diff --git a/src/zkbob/verifiers/stageV1/TreeUpdateVerifier.sol b/src/zkbob/verifiers/stageV1/TreeUpdateVerifier.sol index 380d6a0..d77c518 100644 --- a/src/zkbob/verifiers/stageV1/TreeUpdateVerifier.sol +++ b/src/zkbob/verifiers/stageV1/TreeUpdateVerifier.sol @@ -208,6 +208,8 @@ contract TreeUpdateVerifier { require(input[i] < SNARK_SCALAR_FIELD, "verifier-gte-snark-scalar-field"); vk_x = Pairing.plus(vk_x, Pairing.scalar_mul(vk.IC[i + 1], input[i])); } - return Pairing.pairing(Pairing.negate(_proof.A), _proof.B, vk.alfa1, vk.beta2, vk_x, vk.gamma2, _proof.C, vk.delta2); + return Pairing.pairing( + Pairing.negate(_proof.A), _proof.B, vk.alfa1, vk.beta2, vk_x, vk.gamma2, _proof.C, vk.delta2 + ); } } diff --git a/test/BobToken.t.sol b/test/BobToken.t.sol index 8297d2a..f07f893 100644 --- a/test/BobToken.t.sol +++ b/test/BobToken.t.sol @@ -144,6 +144,26 @@ contract BobTokenTest is Test, EIP2470Test { bob.permit(user1, user2, 1 ether, expiry, v, r, s); } + function testPermitFailsAfterHardFork() public { + vm.prank(user1); + bob.mint(user1, 1 ether); + + uint256 expiry = block.timestamp + 1 days; + (uint8 v, bytes32 r, bytes32 s) = _signPermit(pk1, user1, user2, 1 ether, 0, expiry); + + bytes32 sep = bob.DOMAIN_SEPARATOR(); + + vm.chainId(1234); + assertTrue(sep != bob.DOMAIN_SEPARATOR()); + vm.expectRevert("ERC20Permit: invalid ERC2612 signature"); + bob.permit(user1, user2, 1 ether, expiry, v, r, s); + + vm.chainId(31337); + assertTrue(sep == bob.DOMAIN_SEPARATOR()); + bob.permit(user1, user2, 1 ether, expiry, v, r, s); + assertEq(bob.allowance(user1, user2), 1 ether); + } + function testReceiveWithPermit() public { vm.prank(user1); bob.mint(user1, 1 ether);