From aeca08c89c42c4bc4a560c038bba3baa6ed3af7e Mon Sep 17 00:00:00 2001 From: Eugene Mamin Date: Mon, 20 Feb 2023 14:17:22 +0300 Subject: [PATCH] feat: use embedded EIP712 for StETH The idea is to provide the signature for the proper StETH token address. --- contracts/0.4.24/StETHPermit.sol | 4 +- contracts/0.8.9/EIP712StETH.sol | 83 +++++++++++++++++++++---- contracts/common/interfaces/IEIP712.sol | 4 +- lib/abi/EIP712StETH.json | 2 +- test/0.4.24/stethpermit.test.js | 68 +++++++++++--------- 5 files changed, 114 insertions(+), 47 deletions(-) diff --git a/contracts/0.4.24/StETHPermit.sol b/contracts/0.4.24/StETHPermit.sol index 1c2ef1f40..fe05c818c 100644 --- a/contracts/0.4.24/StETHPermit.sol +++ b/contracts/0.4.24/StETHPermit.sol @@ -100,7 +100,7 @@ contract StETHPermit is IERC2612, StETH { abi.encode(PERMIT_TYPEHASH, _owner, _spender, _value, _useNonce(_owner), _deadline) ); - bytes32 hash = IEIP712(getEIP712StETH()).hashTypedDataV4(structHash); + bytes32 hash = IEIP712(getEIP712StETH()).hashTypedDataV4(address(this), structHash); address signer = ECDSA.recover(hash, _v, _r, _s); require(signer == _owner, "ERC20Permit: invalid signature"); @@ -124,7 +124,7 @@ contract StETHPermit is IERC2612, StETH { */ // solhint-disable-next-line func-name-mixedcase function DOMAIN_SEPARATOR() external view returns (bytes32) { - return IEIP712(getEIP712StETH()).domainSeparatorV4(); + return IEIP712(getEIP712StETH()).domainSeparatorV4(address(this)); } /** diff --git a/contracts/0.8.9/EIP712StETH.sol b/contracts/0.8.9/EIP712StETH.sol index 5686fd57c..f761047a2 100644 --- a/contracts/0.8.9/EIP712StETH.sol +++ b/contracts/0.8.9/EIP712StETH.sol @@ -1,27 +1,88 @@ -// SPDX-FileCopyrightText: 2023 Lido -// SPDX-License-Identifier: GPL-3.0 +// SPDX-FileCopyrightText: 2023 OpenZeppelin, Lido +// SPDX-License-Identifier: MIT /* See contracts/COMPILERS.md */ pragma solidity 0.8.9; -import {EIP712} from "@openzeppelin/contracts-v4.4/utils/cryptography/draft-EIP712.sol"; +import {ECDSA} from "@openzeppelin/contracts-v4.4/utils/cryptography/ECDSA.sol"; import {IEIP712} from "../common/interfaces/IEIP712.sol"; /** - * Helper contract exposes OpenZeppelin's EIP712 message utils implementation. + * NOTE: The code below is taken from "@openzeppelin/contracts-v4.4/utils/cryptography/draft-EIP712.sol" + * With a main difference to store the stETH contract address internally and use it for signing. */ -contract EIP712StETH is IEIP712, EIP712 { + +/** + * @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]. + * + */ +contract EIP712StETH is IEIP712 { + /* 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_STETH; + + bytes32 private immutable _HASHED_NAME; + bytes32 private immutable _HASHED_VERSION; + bytes32 private immutable _TYPE_HASH; + + error ZeroStETHAddress(); + /** * @dev Constructs specialized EIP712 instance for StETH token, version "2". */ - constructor() EIP712("Liquid staked Ether 2.0", "2") {} + constructor(address _stETH) { + if (_stETH == address(0)) { revert ZeroStETHAddress(); } + + bytes32 hashedName = keccak256(bytes("Liquid staked Ether 2.0")); + bytes32 hashedVersion = keccak256(bytes("2")); + 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, _stETH); + _CACHED_STETH = _stETH; + _TYPE_HASH = typeHash; + } /** * @dev Returns the domain separator for the current chain. */ - function domainSeparatorV4() external view override returns (bytes32) { - return _domainSeparatorV4(); + function domainSeparatorV4(address _stETH) public view override returns (bytes32) { + if (_stETH == _CACHED_STETH && block.chainid == _CACHED_CHAIN_ID) { + return _CACHED_DOMAIN_SEPARATOR; + } else { + return _buildDomainSeparator(_TYPE_HASH, _HASHED_NAME, _HASHED_VERSION, _stETH); + } + } + + function _buildDomainSeparator( + bytes32 _typeHash, + bytes32 _nameHash, + bytes32 _versionHash, + address _stETH + ) private view returns (bytes32) { + return keccak256(abi.encode(_typeHash, _nameHash, _versionHash, block.chainid, _stETH)); } /** @@ -31,7 +92,7 @@ contract EIP712StETH is IEIP712, EIP712 { * 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( + * bytes32 digest = _hashTypedDataV4(keccak256(abi.encode( * keccak256("Mail(address to,string contents)"), * mailTo, * keccak256(bytes(mailContents)) @@ -39,7 +100,7 @@ contract EIP712StETH is IEIP712, EIP712 { * address signer = ECDSA.recover(digest, signature); * ``` */ - function hashTypedDataV4(bytes32 _structHash) external view override returns (bytes32) { - return _hashTypedDataV4(_structHash); + function hashTypedDataV4(address _stETH, bytes32 _structHash) external view override returns (bytes32) { + return ECDSA.toTypedDataHash(domainSeparatorV4(_stETH), _structHash); } } diff --git a/contracts/common/interfaces/IEIP712.sol b/contracts/common/interfaces/IEIP712.sol index 2916c245d..1636432bc 100644 --- a/contracts/common/interfaces/IEIP712.sol +++ b/contracts/common/interfaces/IEIP712.sol @@ -15,7 +15,7 @@ interface IEIP712 { /** * @dev Returns the domain separator for the current chain. */ - function domainSeparatorV4() external view returns (bytes32); + function domainSeparatorV4(address _stETH) external view returns (bytes32); /** * @dev Given an already https://eips.ethereum.org/EIPS/eip-712#definition-of-hashstruct[hashed struct], this @@ -32,5 +32,5 @@ interface IEIP712 { * address signer = ECDSA.recover(digest, signature); * ``` */ - function hashTypedDataV4(bytes32 _structHash) external view returns (bytes32); + function hashTypedDataV4(address _stETH, bytes32 _structHash) external view returns (bytes32); } diff --git a/lib/abi/EIP712StETH.json b/lib/abi/EIP712StETH.json index b5fcf2fb2..bb6f561c4 100644 --- a/lib/abi/EIP712StETH.json +++ b/lib/abi/EIP712StETH.json @@ -1 +1 @@ -[{"inputs":[],"stateMutability":"nonpayable","type":"constructor"},{"inputs":[],"name":"domainSeparatorV4","outputs":[{"internalType":"bytes32","name":"","type":"bytes32"}],"stateMutability":"view","type":"function"},{"inputs":[{"internalType":"bytes32","name":"_structHash","type":"bytes32"}],"name":"hashTypedDataV4","outputs":[{"internalType":"bytes32","name":"","type":"bytes32"}],"stateMutability":"view","type":"function"}] \ No newline at end of file +[{"inputs":[{"internalType":"address","name":"_stETH","type":"address"}],"stateMutability":"nonpayable","type":"constructor"},{"inputs":[],"name":"ZeroStETHAddress","type":"error"},{"inputs":[{"internalType":"address","name":"_stETH","type":"address"}],"name":"domainSeparatorV4","outputs":[{"internalType":"bytes32","name":"","type":"bytes32"}],"stateMutability":"view","type":"function"},{"inputs":[{"internalType":"address","name":"_stETH","type":"address"},{"internalType":"bytes32","name":"_structHash","type":"bytes32"}],"name":"hashTypedDataV4","outputs":[{"internalType":"bytes32","name":"","type":"bytes32"}],"stateMutability":"view","type":"function"}] \ No newline at end of file diff --git a/test/0.4.24/stethpermit.test.js b/test/0.4.24/stethpermit.test.js index 53f7b0d86..acab0e364 100644 --- a/test/0.4.24/stethpermit.test.js +++ b/test/0.4.24/stethpermit.test.js @@ -1,8 +1,7 @@ const crypto = require('crypto') const { ACCOUNTS_AND_KEYS, MAX_UINT256, ZERO_ADDRESS } = require('./helpers/constants') const { bn } = require('@aragon/contract-helpers-test') -const { assertBn, assertEvent } = require('@aragon/contract-helpers-test/src/asserts') -const { assertRevert } = require('../helpers/assertThrow') +const { assert } = require('../helpers/assert') const { signPermit, signTransferAuthorization, makeDomainSeparator } = require('./helpers/permit_helpers') const { hexStringFromBuffer } = require('./helpers/sign_utils') const { ETH } = require('../helpers/utils') @@ -16,13 +15,13 @@ contract('StETHPermit', ([deployer, ...accounts]) => { const snapshot = new EvmSnapshot(hre.ethers.provider) before('deploy mock token', async () => { - eip712StETH = await EIP712StETH.new({ from: deployer }) stEthPermit = await StETHPermit.new({ from: deployer, value: ETH(1) }) + eip712StETH = await EIP712StETH.new(stEthPermit.address, { from: deployer }) await stEthPermit.initializeEIP712StETH(eip712StETH.address) chainId = await web3.eth.net.getId(); - domainSeparator = makeDomainSeparator('Liquid staked Ether 2.0', '2', chainId, eip712StETH.address) + domainSeparator = makeDomainSeparator('Liquid staked Ether 2.0', '2', chainId, stEthPermit.address) await snapshot.make() }) @@ -50,8 +49,15 @@ contract('StETHPermit', ([deployer, ...accounts]) => { await stEthPermit.mintShares(permitParams.owner, initialBalance, { from: deployer }) }) + it('EIP-712 signature helper reverts when zero stETH address passed', async () => { + await assert.revertsWithCustomError( + EIP712StETH.new(ZERO_ADDRESS, { from: deployer }), + `ZeroStETHAddress()` + ) + }) + it('EIP-712 signature helper contract matches the stored one', async () => { - assert.equal(await stEthPermit.getEIP712StETH(), eip712StETH.address) + assert.equals(await stEthPermit.getEIP712StETH(), eip712StETH.address) }) it('grants allowance when a valid permit is given', async () => { @@ -64,11 +70,11 @@ contract('StETHPermit', ([deployer, ...accounts]) => { let { v, r, s } = signPermit(owner, spender, value, nonce, deadline, domainSeparator, alice.key) // check that the allowance is initially zero - assertBn(await stEthPermit.allowance(owner, spender), bn(0)) + assert.equals(await stEthPermit.allowance(owner, spender), bn(0)) // check that the next nonce expected is zero - assertBn(await stEthPermit.nonces(owner), bn(0)) + assert.equals(await stEthPermit.nonces(owner), bn(0)) // check domain separator - assert.equal( + assert.equals( await stEthPermit.DOMAIN_SEPARATOR(), domainSeparator ) @@ -79,15 +85,15 @@ contract('StETHPermit', ([deployer, ...accounts]) => { ) // check that allowance is updated - assertBn(await stEthPermit.allowance(owner, spender), bn(value)) + assert.equals(await stEthPermit.allowance(owner, spender), bn(value)) - assertEvent( + assert.emits( receipt, 'Approval', - { expectedArgs: { owner: owner, spender: spender, value: bn(value) } } + { owner: owner, spender: spender, value: bn(value) } ) - assertBn(await stEthPermit.nonces(owner), bn(1)) + assert.equals(await stEthPermit.nonces(owner), bn(1)) // increment nonce nonce = 1 @@ -99,15 +105,15 @@ contract('StETHPermit', ([deployer, ...accounts]) => { const receipt2 = await stEthPermit.permit(owner, spender, value, deadline, v, r, s, { from: charlie }) // check that allowance is updated - assertBn(await stEthPermit.allowance(owner, spender), bn(value)) + assert.equals(await stEthPermit.allowance(owner, spender), bn(value)) - assertEvent( + assert.emits( receipt2, 'Approval', - { expectedArgs: { owner: owner, spender: spender, value: bn(value) } } + { owner: owner, spender: spender, value: bn(value) } ) - assertBn(await stEthPermit.nonces(owner), bn(2)) + assert.equals(await stEthPermit.nonces(owner), bn(2)) }) it('reverts if the signature does not match given parameters', async () => { @@ -116,7 +122,7 @@ contract('StETHPermit', ([deployer, ...accounts]) => { const { v, r, s } = signPermit(owner, spender, value, nonce, deadline, domainSeparator, alice.key) // try to cheat by claiming the approved amount + 1 - await assertRevert( + await assert.reverts( stEthPermit.permit( owner, spender, @@ -131,7 +137,7 @@ contract('StETHPermit', ([deployer, ...accounts]) => { ) // check that msg is incorrect even if claim the approved amount - 1 - await assertRevert( + await assert.reverts( stEthPermit.permit( owner, spender, @@ -154,7 +160,7 @@ contract('StETHPermit', ([deployer, ...accounts]) => { // try to cheat by submitting the permit that is signed by a // wrong person - await assertRevert( + await assert.reverts( stEthPermit.permit(owner, spender, value, deadline, v, r, s, { from: charlie }), @@ -166,7 +172,7 @@ contract('StETHPermit', ([deployer, ...accounts]) => { await web3.eth.sendTransaction({ to: bob.address, from: accounts[0], value: ETH(10) }) // even Bob himself can't call permit with the invalid sig - await assertRevert( + await assert.reverts( stEthPermit.permit(owner, spender, value, deadline, v, r, s, { from: bob.address }), @@ -181,7 +187,7 @@ contract('StETHPermit', ([deployer, ...accounts]) => { const { v, r, s } = signPermit(owner, spender, value, nonce, deadline, domainSeparator, alice.key) // try to submit the permit that is expired - await assertRevert( + await assert.reverts( stEthPermit.permit(owner, spender, value, deadline, v, r, s, { from: charlie }), @@ -194,11 +200,11 @@ contract('StETHPermit', ([deployer, ...accounts]) => { const { v, r, s } = signPermit(owner, spender, value, nonce, deadline1min, domainSeparator, alice.key) const receipt = await stEthPermit.permit(owner, spender, value, deadline1min, v, r, s, { from: charlie }) - assertBn(await stEthPermit.nonces(owner), bn(1)) - assertEvent( + assert.equals(await stEthPermit.nonces(owner), bn(1)) + assert.emits( receipt, 'Approval', - { expectedArgs: { owner: owner, spender: spender, value: bn(value) } } + { owner: owner, spender: spender, value: bn(value) } ) } }) @@ -209,10 +215,10 @@ contract('StETHPermit', ([deployer, ...accounts]) => { // create a signed permit const { v, r, s } = signPermit(owner, spender, value, nonce, deadline, domainSeparator, alice.key) // check that the next nonce expected is 0, not 1 - assertBn(await stEthPermit.nonces(owner), bn(0)) + assert.equals(await stEthPermit.nonces(owner), bn(0)) // try to submit the permit - await assertRevert( + await assert.reverts( stEthPermit.permit(owner, spender, value, deadline, v, r, s, { from: charlie }), @@ -229,7 +235,7 @@ contract('StETHPermit', ([deployer, ...accounts]) => { await stEthPermit.permit(owner, spender, value, deadline, v, r, s, { from: charlie }) // try to submit the permit again - await assertRevert( + await assert.reverts( stEthPermit.permit(owner, spender, value, deadline, v, r, s, { from: charlie }), @@ -241,7 +247,7 @@ contract('StETHPermit', ([deployer, ...accounts]) => { await web3.eth.sendTransaction({ to: alice.address, from: accounts[0], value: ETH(10) }) // try to submit the permit again from Alice herself - await assertRevert( + await assert.reverts( stEthPermit.permit(owner, spender, value, deadline, v, r, s, { from: alice.address }), @@ -262,7 +268,7 @@ contract('StETHPermit', ([deployer, ...accounts]) => { const permit2 = signPermit(owner, spender, 1e6, nonce, deadline, domainSeparator, alice.key) // try to submit the permit again - await assertRevert( + await assert.reverts( stEthPermit.permit(owner, spender, 1e6, deadline, permit2.v, permit2.r, permit2.s, { from: charlie }), 'ERC20Permit: invalid signature' ) @@ -276,7 +282,7 @@ contract('StETHPermit', ([deployer, ...accounts]) => { const { v, r, s } = signPermit(owner, spender, value, nonce, deadline, domainSeparator, alice.key) // try to submit the permit with invalid approval parameters - await assertRevert( + await assert.reverts( stEthPermit.permit(owner, spender, value, deadline, v, r, s, { from: charlie }), @@ -292,7 +298,7 @@ contract('StETHPermit', ([deployer, ...accounts]) => { const { v, r, s } = signTransferAuthorization(from, to, value, validAfter, validBefore, nonce, domainSeparator, alice.key) // try to submit the transfer permit - await assertRevert( + await assert.reverts( stEthPermit.permit(from, to, value, validBefore, v, r, s, { from: charlie }),