From 4f3422c4429bd862ce5d39744cf35ff1228ca44f Mon Sep 17 00:00:00 2001 From: Anton Bukov Date: Fri, 2 Dec 2022 17:43:24 +0100 Subject: [PATCH] Introduce SequentialOperations and ParallelOperations between EIP712 and signature-based operations like ERC20Permit, Votes, Governor, MinimalForwarder --- contracts/governance/Governor.sol | 27 +++----- contracts/governance/utils/Votes.sol | 15 ++--- contracts/metatx/MinimalForwarder.sol | 24 +++---- .../token/ERC20/extensions/ERC20Permit.sol | 27 ++++---- .../utils/cryptography/ParallelOperations.sol | 64 +++++++++++++++++++ .../cryptography/SequentialOperations.sol | 62 ++++++++++++++++++ test/governance/utils/Votes.behavior.js | 4 +- test/metatx/ERC2771Context.test.js | 4 +- test/metatx/MinimalForwarder.test.js | 20 +++--- .../token/ERC20/extensions/ERC20Votes.test.js | 4 +- .../ERC20/extensions/ERC20VotesComp.test.js | 4 +- .../extensions/draft-ERC20Permit.test.js | 2 +- 12 files changed, 188 insertions(+), 69 deletions(-) create mode 100644 contracts/utils/cryptography/ParallelOperations.sol create mode 100644 contracts/utils/cryptography/SequentialOperations.sol diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index 92e1ab44043..c772cd49ddd 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -6,7 +6,7 @@ pragma solidity ^0.8.0; import "../token/ERC721/IERC721Receiver.sol"; import "../token/ERC1155/IERC1155Receiver.sol"; import "../utils/cryptography/ECDSA.sol"; -import "../utils/cryptography/EIP712.sol"; +import "../utils/cryptography/ParallelOperations.sol"; import "../utils/introspection/ERC165.sol"; import "../utils/math/SafeCast.sol"; import "../utils/structs/DoubleEndedQueue.sol"; @@ -26,7 +26,7 @@ import "./IGovernor.sol"; * * _Available since v4.3._ */ -abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receiver, IERC1155Receiver { +abstract contract Governor is Context, ERC165, ParallelOperations, IGovernor, IERC721Receiver, IERC1155Receiver { using DoubleEndedQueue for DoubleEndedQueue.Bytes32Deque; using SafeCast for uint256; using Timers for Timers.BlockNumber; @@ -450,8 +450,10 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive bytes32 r, bytes32 s ) public virtual override returns (uint256) { - address voter = ECDSA.recover( - _hashTypedDataV4(keccak256(abi.encode(BALLOT_TYPEHASH, proposalId, support))), + address voter = _validateParallelOperation( + BALLOT_TYPEHASH, + keccak256(abi.encode(BALLOT_TYPEHASH, proposalId, support)), + proposalId, // use proposalId as nonce v, r, s @@ -471,23 +473,14 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive bytes32 r, bytes32 s ) public virtual override returns (uint256) { - address voter = ECDSA.recover( - _hashTypedDataV4( - keccak256( - abi.encode( - EXTENDED_BALLOT_TYPEHASH, - proposalId, - support, - keccak256(bytes(reason)), - keccak256(params) - ) - ) - ), + address voter = _validateParallelOperation( + EXTENDED_BALLOT_TYPEHASH, + keccak256(abi.encode(EXTENDED_BALLOT_TYPEHASH, proposalId, support, keccak256(bytes(reason)), keccak256(params))), + proposalId, // use proposalId as nonce v, r, s ); - return _castVote(proposalId, voter, support, reason, params); } diff --git a/contracts/governance/utils/Votes.sol b/contracts/governance/utils/Votes.sol index f91104354f7..a307474f855 100644 --- a/contracts/governance/utils/Votes.sol +++ b/contracts/governance/utils/Votes.sol @@ -5,7 +5,7 @@ pragma solidity ^0.8.0; import "../../utils/Context.sol"; import "../../utils/Nonces.sol"; import "../../utils/Checkpoints.sol"; -import "../../utils/cryptography/EIP712.sol"; +import "../../utils/cryptography/SequentialOperations.sol"; import "./IVotes.sol"; import "../../utils/math/SafeCast.sol"; @@ -29,9 +29,8 @@ import "../../utils/math/SafeCast.sol"; * * _Available since v4.5._ */ -abstract contract Votes is IVotes, Context, EIP712 { +abstract contract Votes is IVotes, Context, SequentialOperations { using Checkpoints for Checkpoints.History; - using Nonces for Nonces.Data; bytes32 private constant _DELEGATION_TYPEHASH = keccak256("Delegation(address delegatee,uint256 nonce,uint256 expiry)"); @@ -39,7 +38,6 @@ abstract contract Votes is IVotes, Context, EIP712 { mapping(address => address) private _delegation; mapping(address => Checkpoints.History) private _delegateCheckpoints; Checkpoints.History private _totalCheckpoints; - Nonces.Data private _nonces; /** * @dev Returns the current amount of votes that `account` has. @@ -86,7 +84,7 @@ abstract contract Votes is IVotes, Context, EIP712 { * @dev Returns the delegation nonce for `owner`. */ function delegationNonces(address owner) public view virtual override returns (uint256) { - return _nonces.nonces(owner); + return operationNonces(_DELEGATION_TYPEHASH, owner); } /** @@ -116,13 +114,14 @@ abstract contract Votes is IVotes, Context, EIP712 { bytes32 s ) public virtual override { require(block.timestamp <= expiry, "Votes: signature expired"); - address signer = ECDSA.recover( - _hashTypedDataV4(keccak256(abi.encode(_DELEGATION_TYPEHASH, delegatee, nonce, expiry))), + address signer = _validateSequentialOperation( + _DELEGATION_TYPEHASH, + keccak256(abi.encode(_DELEGATION_TYPEHASH, delegatee, nonce, expiry)), + nonce, v, r, s ); - require(nonce == _nonces.useNonce(signer), "Votes: invalid nonce"); _delegate(signer, delegatee); } diff --git a/contracts/metatx/MinimalForwarder.sol b/contracts/metatx/MinimalForwarder.sol index 910d5d28b8d..344025afcb0 100644 --- a/contracts/metatx/MinimalForwarder.sol +++ b/contracts/metatx/MinimalForwarder.sol @@ -4,7 +4,7 @@ pragma solidity ^0.8.0; import "../utils/cryptography/ECDSA.sol"; -import "../utils/cryptography/EIP712.sol"; +import "../utils/cryptography/SequentialOperations.sol"; /** * @dev Simple minimal forwarder to be used together with an ERC2771 compatible contract. See {ERC2771Context}. @@ -14,7 +14,7 @@ import "../utils/cryptography/EIP712.sol"; * functioning forwarding system with good properties requires more complexity. We suggest you look at other projects * such as the GSN which do have the goal of building a system like that. */ -contract MinimalForwarder is EIP712 { +contract MinimalForwarder is SequentialOperations { using ECDSA for bytes32; struct ForwardRequest { @@ -29,19 +29,20 @@ contract MinimalForwarder is EIP712 { bytes32 private constant _TYPEHASH = keccak256("ForwardRequest(address from,address to,uint256 value,uint256 gas,uint256 nonce,bytes data)"); - mapping(address => uint256) private _nonces; - constructor() EIP712("MinimalForwarder", "0.0.1") {} function getNonce(address from) public view returns (uint256) { - return _nonces[from]; + return operationNonces(_TYPEHASH, from); } - function verify(ForwardRequest calldata req, bytes calldata signature) public view returns (bool) { - address signer = _hashTypedDataV4( - keccak256(abi.encode(_TYPEHASH, req.from, req.to, req.value, req.gas, req.nonce, keccak256(req.data))) - ).recover(signature); - return _nonces[req.from] == req.nonce && signer == req.from; + function verify(ForwardRequest calldata req, bytes calldata signature) public returns (bool) { + address signer = _validateSequentialOperation( + _TYPEHASH, + keccak256(abi.encode(_TYPEHASH, req.from, req.to, req.value, req.gas, req.nonce, keccak256(req.data))), + req.nonce, + signature + ); + return signer == req.from; } function execute(ForwardRequest calldata req, bytes calldata signature) @@ -50,8 +51,7 @@ contract MinimalForwarder is EIP712 { returns (bool, bytes memory) { require(verify(req, signature), "MinimalForwarder: signature does not match request"); - _nonces[req.from] = req.nonce + 1; - + (bool success, bytes memory returndata) = req.to.call{gas: req.gas, value: req.value}( abi.encodePacked(req.data, req.from) ); diff --git a/contracts/token/ERC20/extensions/ERC20Permit.sol b/contracts/token/ERC20/extensions/ERC20Permit.sol index 513deb7a9a2..5694acd1051 100644 --- a/contracts/token/ERC20/extensions/ERC20Permit.sol +++ b/contracts/token/ERC20/extensions/ERC20Permit.sol @@ -6,7 +6,7 @@ pragma solidity ^0.8.0; import "./IERC20Permit.sol"; import "../ERC20.sol"; import "../../../utils/cryptography/ECDSA.sol"; -import "../../../utils/cryptography/EIP712.sol"; +import "../../../utils/cryptography/SequentialOperations.sol"; import "../../../utils/Nonces.sol"; /** @@ -19,9 +19,7 @@ import "../../../utils/Nonces.sol"; * * _Available since v3.4._ */ -abstract contract ERC20Permit is ERC20, IERC20Permit, EIP712 { - using Nonces for Nonces.Data; - +abstract contract ERC20Permit is ERC20, IERC20Permit, SequentialOperations { // solhint-disable-next-line var-name-mixedcase bytes32 private constant _PERMIT_TYPEHASH = keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)"); @@ -33,7 +31,6 @@ abstract contract ERC20Permit is ERC20, IERC20Permit, EIP712 { */ // solhint-disable-next-line var-name-mixedcase bytes32 private _PERMIT_TYPEHASH_DEPRECATED_SLOT; - Nonces.Data private _nonces; /** * @dev Initializes the {EIP712} domain separator using the `name` parameter, and setting `version` to `"1"`. @@ -55,14 +52,16 @@ abstract contract ERC20Permit is ERC20, IERC20Permit, EIP712 { bytes32 s ) public virtual override { require(block.timestamp <= deadline, "ERC20Permit: expired deadline"); - - bytes32 structHash = keccak256(abi.encode(_PERMIT_TYPEHASH, owner, spender, value, _nonces.useNonce(owner), deadline)); - - bytes32 hash = _hashTypedDataV4(structHash); - - address signer = ECDSA.recover(hash, v, r, s); - require(signer == owner, "ERC20Permit: invalid signature"); - + uint256 nonce = operationNonces(_PERMIT_TYPEHASH, owner); + address signer = _validateSequentialOperation( + _PERMIT_TYPEHASH, + keccak256(abi.encode(_PERMIT_TYPEHASH, owner, spender, value, nonce, deadline)), + nonce, + v, + r, + s + ); + require(owner == signer, "ERC20Permit: invalid signature"); _approve(owner, spender, value); } @@ -70,7 +69,7 @@ abstract contract ERC20Permit is ERC20, IERC20Permit, EIP712 { * @dev See {IERC20Permit-nonces}. */ function nonces(address owner) public view virtual override returns (uint256) { - return _nonces.nonces(owner); + return operationNonces(_PERMIT_TYPEHASH, owner); } /** diff --git a/contracts/utils/cryptography/ParallelOperations.sol b/contracts/utils/cryptography/ParallelOperations.sol new file mode 100644 index 00000000000..c998b44a8a5 --- /dev/null +++ b/contracts/utils/cryptography/ParallelOperations.sol @@ -0,0 +1,64 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +import "../Context.sol"; +import "../Nonces.sol"; +import "./EIP712.sol"; +import "./ECDSA.sol"; + +/** + * @dev Provides tracking nonces per address and operation. Operation ids should be unique. + */ +abstract contract ParallelOperations is Context, EIP712 { + mapping(bytes32 => mapping(address => mapping(uint256 => bool))) private _usedOperationIds; + + function isOperationIdAvailable(bytes32 operationTypehash, address owner, uint256 operationId) public view virtual returns (bool) { + return !_usedOperationIds[operationTypehash][owner][operationId]; + } + + function useOperationId(bytes32 operationTypehash, uint256 operationId) public virtual { + _useOperationId(operationTypehash, _msgSender(), operationId); + } + + function _validateParallelOperation( + bytes32 operationTypehash, + bytes32 operationHash, + uint256 operationId, + bytes memory signature + ) internal virtual returns(address signer) { + signer = ECDSA.recover(_hashTypedDataV4(operationHash), signature); + _useOperationId(operationTypehash, signer, operationId); + } + + function _validateParallelOperation( + bytes32 operationTypehash, + bytes32 operationHash, + uint256 operationId, + uint8 v, + bytes32 r, + bytes32 s + ) internal virtual returns(address signer) { + signer = ECDSA.recover(_hashTypedDataV4(operationHash), v, r, s); + _useOperationId(operationTypehash, signer, operationId); + } + + function _validateParallelOperation( + bytes32 operationTypehash, + bytes32 operationHash, + uint256 operationId, + bytes32 r, + bytes32 vs + ) internal virtual returns(address signer) { + signer = ECDSA.recover(_hashTypedDataV4(operationHash), r, vs); + _useOperationId(operationTypehash, signer, operationId); + } + + /// @dev Method made non-virtual to deny changing logic of parallel operations invalidation. + function _useOperationId(bytes32 operationTypehash, address owner, uint256 operationId) internal { + require( + !_usedOperationIds[operationTypehash][owner][operationId], + "ParallelOperations: invalid operation id" + ); + _usedOperationIds[operationTypehash][owner][operationId] = true; + } +} diff --git a/contracts/utils/cryptography/SequentialOperations.sol b/contracts/utils/cryptography/SequentialOperations.sol new file mode 100644 index 00000000000..f327c9316a1 --- /dev/null +++ b/contracts/utils/cryptography/SequentialOperations.sol @@ -0,0 +1,62 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +import "../Context.sol"; +import "../Nonces.sol"; +import "./EIP712.sol"; +import "./ECDSA.sol"; + +/** + * @dev Provides tracking nonces per address and operation. Nonces will only increment. + */ +abstract contract SequentialOperations is Context, EIP712 { + using Nonces for Nonces.Data; + + mapping(bytes32 => Nonces.Data) private _nonces; + + function operationNonces(bytes32 operationTypehash, address owner) public view virtual returns (uint256) { + return _nonces[operationTypehash].nonces(owner); + } + + function useOperationNonce(bytes32 operationTypehash, uint256 nonce) public virtual { + _useOperationNonce(operationTypehash, _msgSender(), nonce); + } + + function _validateSequentialOperation( + bytes32 operationTypehash, + bytes32 operationHash, + uint256 nonce, + bytes memory signature + ) internal virtual returns(address signer) { + signer = ECDSA.recover(_hashTypedDataV4(operationHash), signature); + _useOperationNonce(operationTypehash, signer, nonce); + } + + function _validateSequentialOperation( + bytes32 operationTypehash, + bytes32 operationHash, + uint256 nonce, + uint8 v, + bytes32 r, + bytes32 s + ) internal virtual returns(address signer) { + signer = ECDSA.recover(_hashTypedDataV4(operationHash), v, r, s); + _useOperationNonce(operationTypehash, signer, nonce); + } + + function _validateSequentialOperation( + bytes32 operationTypehash, + bytes32 operationHash, + uint256 nonce, + bytes32 r, + bytes32 vs + ) internal virtual returns(address signer) { + signer = ECDSA.recover(_hashTypedDataV4(operationHash), r, vs); + _useOperationNonce(operationTypehash, signer, nonce); + } + + /// @dev Method made non-virtual to deny changing logic of sequential operations invalidation. + function _useOperationNonce(bytes32 operationTypehash, address owner, uint256 nonce) internal { + require(nonce == _nonces[operationTypehash].useNonce(owner), "SequentialOperations: invalid nonce"); + } +} diff --git a/test/governance/utils/Votes.behavior.js b/test/governance/utils/Votes.behavior.js index 4b3810d07d9..37faed4e54f 100644 --- a/test/governance/utils/Votes.behavior.js +++ b/test/governance/utils/Votes.behavior.js @@ -95,7 +95,7 @@ function shouldBehaveLikeVotes () { await expectRevert( this.votes.delegateBySig(delegatorAddress, nonce, MAX_UINT256, v, r, s), - 'Votes: invalid nonce', + 'SequentialOperations: invalid nonce', ); }); @@ -127,7 +127,7 @@ function shouldBehaveLikeVotes () { )); await expectRevert( this.votes.delegateBySig(delegatorAddress, nonce + 1, MAX_UINT256, v, r, s), - 'Votes: invalid nonce', + 'SequentialOperations: invalid nonce', ); }); diff --git a/test/metatx/ERC2771Context.test.js b/test/metatx/ERC2771Context.test.js index 8db92ab83e3..36ac5a7dab3 100644 --- a/test/metatx/ERC2771Context.test.js +++ b/test/metatx/ERC2771Context.test.js @@ -76,7 +76,7 @@ contract('ERC2771Context', function (accounts) { }; const sign = ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), { data: { ...this.data, message: req } }); - expect(await this.forwarder.verify(req, sign)).to.equal(true); + expect(await this.forwarder.contract.methods.verify(req, sign).call()).to.equal(true); const { tx } = await this.forwarder.execute(req, sign); await expectEvent.inTransaction(tx, ERC2771ContextMock, 'Sender', { sender: this.sender }); @@ -99,7 +99,7 @@ contract('ERC2771Context', function (accounts) { }; const sign = ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), { data: { ...this.data, message: req } }); - expect(await this.forwarder.verify(req, sign)).to.equal(true); + expect(await this.forwarder.contract.methods.verify(req, sign).call()).to.equal(true); const { tx } = await this.forwarder.execute(req, sign); await expectEvent.inTransaction(tx, ERC2771ContextMock, 'Data', { data, integerValue, stringValue }); diff --git a/test/metatx/MinimalForwarder.test.js b/test/metatx/MinimalForwarder.test.js index b8984e431e3..4ae5ae20181 100644 --- a/test/metatx/MinimalForwarder.test.js +++ b/test/metatx/MinimalForwarder.test.js @@ -66,7 +66,7 @@ contract('MinimalForwarder', function (accounts) { }); it('success', async function () { - expect(await this.forwarder.verify(this.req, this.sign())).to.be.equal(true); + expect(await this.forwarder.contract.methods.verify(this.req, this.sign()).call()).to.be.equal(true); }); afterEach(async function () { @@ -77,29 +77,31 @@ contract('MinimalForwarder', function (accounts) { context('invalid signature', function () { it('tampered from', async function () { - expect(await this.forwarder.verify({ ...this.req, from: accounts[0] }, this.sign())) + expect(await this.forwarder.contract.methods.verify({ ...this.req, from: accounts[0] }, this.sign()).call()) .to.be.equal(false); }); it('tampered to', async function () { - expect(await this.forwarder.verify({ ...this.req, to: accounts[0] }, this.sign())) + expect(await this.forwarder.contract.methods.verify({ ...this.req, to: accounts[0] }, this.sign()).call()) .to.be.equal(false); }); it('tampered value', async function () { - expect(await this.forwarder.verify({ ...this.req, value: web3.utils.toWei('1') }, this.sign())) + expect(await this.forwarder.contract.methods.verify({ ...this.req, value: web3.utils.toWei('1') }, this.sign()).call()) .to.be.equal(false); }); it('tampered nonce', async function () { - expect(await this.forwarder.verify({ ...this.req, nonce: this.req.nonce + 1 }, this.sign())) - .to.be.equal(false); + await expectRevert( + this.forwarder.contract.methods.verify({ ...this.req, nonce: this.req.nonce + 1 }, this.sign()).call(), + 'SequentialOperations: invalid nonce' + ); }); it('tampered data', async function () { - expect(await this.forwarder.verify({ ...this.req, data: '0x1742' }, this.sign())) + expect(await this.forwarder.contract.methods.verify({ ...this.req, data: '0x1742' }, this.sign()).call()) .to.be.equal(false); }); it('tampered signature', async function () { const tamperedsign = web3.utils.hexToBytes(this.sign()); tamperedsign[42] ^= 0xff; - expect(await this.forwarder.verify(this.req, web3.utils.bytesToHex(tamperedsign))) + expect(await this.forwarder.contract.methods.verify(this.req, web3.utils.bytesToHex(tamperedsign)).call()) .to.be.equal(false); }); }); @@ -144,7 +146,7 @@ contract('MinimalForwarder', function (accounts) { it('tampered nonce', async function () { await expectRevert( this.forwarder.execute({ ...this.req, nonce: this.req.nonce + 1 }, this.sign()), - 'MinimalForwarder: signature does not match request', + 'SequentialOperations: invalid nonce', ); }); it('tampered data', async function () { diff --git a/test/token/ERC20/extensions/ERC20Votes.test.js b/test/token/ERC20/extensions/ERC20Votes.test.js index 6f9b2ecbd1d..52db72198b7 100644 --- a/test/token/ERC20/extensions/ERC20Votes.test.js +++ b/test/token/ERC20/extensions/ERC20Votes.test.js @@ -172,7 +172,7 @@ contract('ERC20Votes', function (accounts) { await expectRevert( this.token.delegateBySig(delegatorAddress, nonce, MAX_UINT256, v, r, s), - 'Votes: invalid nonce', + 'SequentialOperations: invalid nonce', ); }); @@ -204,7 +204,7 @@ contract('ERC20Votes', function (accounts) { )); await expectRevert( this.token.delegateBySig(delegatorAddress, nonce + 1, MAX_UINT256, v, r, s), - 'Votes: invalid nonce', + 'SequentialOperations: invalid nonce', ); }); diff --git a/test/token/ERC20/extensions/ERC20VotesComp.test.js b/test/token/ERC20/extensions/ERC20VotesComp.test.js index 411e27668ca..46f4c1df820 100644 --- a/test/token/ERC20/extensions/ERC20VotesComp.test.js +++ b/test/token/ERC20/extensions/ERC20VotesComp.test.js @@ -160,7 +160,7 @@ contract('ERC20VotesComp', function (accounts) { await expectRevert( this.token.delegateBySig(delegatorAddress, nonce, MAX_UINT256, v, r, s), - 'Votes: invalid nonce', + 'SequentialOperations: invalid nonce', ); }); @@ -192,7 +192,7 @@ contract('ERC20VotesComp', function (accounts) { )); await expectRevert( this.token.delegateBySig(delegatorAddress, nonce + 1, MAX_UINT256, v, r, s), - 'Votes: invalid nonce', + 'SequentialOperations: invalid nonce', ); }); diff --git a/test/token/ERC20/extensions/draft-ERC20Permit.test.js b/test/token/ERC20/extensions/draft-ERC20Permit.test.js index fb60a6cbe52..bf7f131de5c 100644 --- a/test/token/ERC20/extensions/draft-ERC20Permit.test.js +++ b/test/token/ERC20/extensions/draft-ERC20Permit.test.js @@ -77,7 +77,7 @@ contract('ERC20Permit', function (accounts) { await expectRevert( this.token.permit(owner, spender, value, maxDeadline, v, r, s), - 'ERC20Permit: invalid signature', + 'SequentialOperations: invalid nonce', ); });