Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tooling to verify signatures with support for ERC1271 #2532

Merged
merged 9 commits into from
Apr 7, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -6,6 +6,7 @@
* `ERC777`: make reception acquirement optional in `_mint`. ([#2552](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2552))
* `ERC20Permit`: add a `_useNonce` to enable further usage of ERC712 signatures. ([#2565](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2565))
* `ERC20FlashMint`: add an implementation of the ERC3156 extension for flash-minting ERC20 tokens. ([#2543](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2543))
* `SignatureChecker`: add a signature verification library that supports both EOA and ERC1271 compliant contracts as signers. ([#2532](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2532))

## 4.0.0 (2021-03-23)

Expand Down
16 changes: 16 additions & 0 deletions contracts/interfaces/IERC1271.sol
@@ -0,0 +1,16 @@
// SPDX-License-Identifier: MIT

pragma solidity ^0.8.0;

/**
* @dev Interface of the ERC1271 standard signature validation method for
* contracts as defined in https://eips.ethereum.org/EIPS/eip-1271[ERC-1271].
*/
interface IERC1271 {
Amxx marked this conversation as resolved.
Show resolved Hide resolved
/**
* @dev Should return whether the signature provided is valid for the provided data
* @param hash Hash of the data to be signed
* @param signature Signature byte array associated with _data
*/
function isValidSignature(bytes32 hash, bytes memory signature) external view returns (bytes4 magicValue);
}
17 changes: 17 additions & 0 deletions contracts/mocks/ERC1271WalletMock.sol
@@ -0,0 +1,17 @@
// SPDX-License-Identifier: MIT

pragma solidity ^0.8.0;

import "../access/Ownable.sol";
import "../interfaces/IERC1271.sol";
import "../utils/cryptography/ECDSA.sol";

contract ERC1271WalletMock is Ownable, IERC1271 {
constructor(address originalOwner) {
transferOwnership(originalOwner);
}

function isValidSignature(bytes32 hash, bytes memory signature) public view override returns (bytes4 magicValue) {
return ECDSA.recover(hash, signature) == owner() ? this.isValidSignature.selector : bytes4(0);
}
}
13 changes: 13 additions & 0 deletions contracts/mocks/SignatureCheckerMock.sol
@@ -0,0 +1,13 @@
// SPDX-License-Identifier: MIT

pragma solidity ^0.8.0;

import "../utils/cryptography/SignatureChecker.sol";

contract SignatureCheckerMock {
using SignatureChecker for address;

function isValidSignatureNow(address signer, bytes32 hash, bytes memory signature) public view returns (bool) {
return signer.isValidSignatureNow(hash, signature);
}
}
2 changes: 2 additions & 0 deletions contracts/utils/README.adoc
Expand Up @@ -36,6 +36,8 @@ Finally, {Create2} contains all necessary utilities to safely use the https://bl

{{ECDSA}}

{{SignatureChecker}}

{{MerkleProof}}

{{EIP712}}
Expand Down
29 changes: 29 additions & 0 deletions contracts/utils/cryptography/SignatureChecker.sol
@@ -0,0 +1,29 @@
// SPDX-License-Identifier: MIT

pragma solidity ^0.8.0;

import "./ECDSA.sol";
import "../Address.sol";
import "../../interfaces/IERC1271.sol";

/**
* @dev Signature verification helper: Provide a single mechanism to verify both private-key (EOA) ECDSA signature and
* ERC1271 contract sigantures. Using this instead of ECDSA.recover in your contract will make them compatible with
* smart contract wallets such as Argent and Gnosis.
*
* Note: unlike ECDSA signatures, contract signature's are revocable, and the outcome of this function can thus change
* through time. It could return true at block N and false at block N+1 (or the opposite).
*/
library SignatureChecker {
Amxx marked this conversation as resolved.
Show resolved Hide resolved
function isValidSignatureNow(address signer, bytes32 hash, bytes memory signature) internal view returns (bool) {
if (Address.isContract(signer)) {
try IERC1271(signer).isValidSignature(hash, signature) returns (bytes4 magicValue) {
return magicValue == IERC1271(signer).isValidSignature.selector;
} catch {
return false;
}
} else {
return ECDSA.recover(hash, signature) == signer;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From our recommendation in ECDSA:

* IMPORTANT: `hash` _must_ be the result of a hash operation for the
* verification to be secure: it is possible to craft signatures that
* recover to arbitrary addresses for non-hashed data. A safe way to ensure
* this is by receiving a hash of the original message (which may otherwise
* be too long), and then calling {toEthSignedMessageHash} on it.

We should be providing some other link in the docs, but there is more info in this article.

So, is this function really safe? Isn't it vulnerable to crafting signature and hash values that will recover to the signer but which do not correspond to a real ECDSA signature?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, is this function really safe? Isn't it vulnerable to crafting signature and hash values that will recover to the signer but which do not correspond to a real ECDSA signature?

Our own ECDSA.recover doesn't enforce that. You can technically use it to recover a hash that was not properly formated. The same goes for a smart wallet 1271 implementation (the code in isValidSignature). In both cases, we cannot do anything about it (the hash might be 191, or 712, or anything not yet standardized. The best we can do is adding the warning you mentioned in the interface for 1271.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, for some reason I thought this API was more vulnerable to misuse, but now that I'm looking at it again it I can see that it's the same.

}
}
}
71 changes: 71 additions & 0 deletions test/utils/cryptography/SignatureChecker.test.js
@@ -0,0 +1,71 @@
const { toEthSignedMessageHash, fixSignature } = require('../../helpers/sign');

const { expect } = require('chai');

const SignatureCheckerMock = artifacts.require('SignatureCheckerMock');
const ERC1271WalletMock = artifacts.require('ERC1271WalletMock');

const TEST_MESSAGE = web3.utils.sha3('OpenZeppelin');
const WRONG_MESSAGE = web3.utils.sha3('Nope');

contract('SignatureChecker (ERC1271)', function (accounts) {
const [signer, other] = accounts;

before('deploying', async function () {
this.signaturechecker = await SignatureCheckerMock.new();
this.wallet = await ERC1271WalletMock.new(signer);
this.signature = fixSignature(await web3.eth.sign(TEST_MESSAGE, signer));
});

context('EOA account', function () {
it('with matching signer and signature', async function () {
expect(await this.signaturechecker.isValidSignatureNow(
signer,
toEthSignedMessageHash(TEST_MESSAGE),
this.signature,
)).to.equal(true);
});

it('with invalid signer', async function () {
expect(await this.signaturechecker.isValidSignatureNow(
other,
toEthSignedMessageHash(TEST_MESSAGE),
this.signature,
)).to.equal(false);
});

it('with invalid signature', async function () {
expect(await this.signaturechecker.isValidSignatureNow(
signer,
toEthSignedMessageHash(WRONG_MESSAGE),
this.signature,
)).to.equal(false);
});
});

context('ERC1271 wallet', function () {
it('with matching signer and signature', async function () {
expect(await this.signaturechecker.isValidSignatureNow(
this.wallet.address,
toEthSignedMessageHash(TEST_MESSAGE),
this.signature,
)).to.equal(true);
});

it('with invalid signer', async function () {
expect(await this.signaturechecker.isValidSignatureNow(
this.signaturechecker.address,
toEthSignedMessageHash(TEST_MESSAGE),
this.signature,
)).to.equal(false);
});

it('with invalid signature', async function () {
expect(await this.signaturechecker.isValidSignatureNow(
this.wallet.address,
toEthSignedMessageHash(WRONG_MESSAGE),
this.signature,
)).to.equal(false);
});
});
});