From 226c6bd8f1350502c8984ee89c44e25d7618521c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Fri, 16 Aug 2019 15:49:13 -0300 Subject: [PATCH] Remove SignatureBouncer from drafts (#1879) * Remove SignatureBouncer * Update changelog entry * Fix coverage * Update CHANGELOG.md --- CHANGELOG.md | 3 +- contracts/mocks/SignatureBouncerMock.sol | 50 ----- test/cryptography/ECDSA.test.js | 12 ++ test/drafts/SignatureBouncer.test.js | 223 ----------------------- 4 files changed, 14 insertions(+), 274 deletions(-) delete mode 100644 contracts/mocks/SignatureBouncerMock.sol delete mode 100644 test/drafts/SignatureBouncer.test.js diff --git a/CHANGELOG.md b/CHANGELOG.md index de602d180..ac73a954e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,8 @@ * `Address.isContract`: switched from `extcodesize` to `extcodehash` for less gas usage. ([#1802](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1802)) * `SafeMath`: added custom error messages support for `sub`, `div` and `mod` functions. `ERC20` and `ERC777` updated to throw custom errors on subtraction overflows. ([#1828](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/1828)) -### Bugfixes +### Breaking changes in drafts: + * `SignatureBouncer` has been removed from the library, both to avoid confusions with the GSN Bouncers and `GSNBouncerSignature` and because the API was not very clear. ([#1879](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/1879)) ## 2.3.0 (2019-05-27) diff --git a/contracts/mocks/SignatureBouncerMock.sol b/contracts/mocks/SignatureBouncerMock.sol deleted file mode 100644 index 59c59b437..000000000 --- a/contracts/mocks/SignatureBouncerMock.sol +++ /dev/null @@ -1,50 +0,0 @@ -pragma solidity ^0.5.0; - -import "../drafts/SignatureBouncer.sol"; -import "./SignerRoleMock.sol"; - -contract SignatureBouncerMock is SignatureBouncer, SignerRoleMock { - function checkValidSignature(address account, bytes memory signature) - public view returns (bool) - { - return _isValidSignature(account, signature); - } - - function onlyWithValidSignature(bytes memory signature) - public onlyValidSignature(signature) view - { - // solhint-disable-previous-line no-empty-blocks - } - - function checkValidSignatureAndMethod(address account, bytes memory signature) - public view returns (bool) - { - return _isValidSignatureAndMethod(account, signature); - } - - function onlyWithValidSignatureAndMethod(bytes memory signature) - public onlyValidSignatureAndMethod(signature) view - { - // solhint-disable-previous-line no-empty-blocks - } - - function checkValidSignatureAndData(address account, bytes memory, uint, bytes memory signature) - public view returns (bool) - { - return _isValidSignatureAndData(account, signature); - } - - function onlyWithValidSignatureAndData(uint, bytes memory signature) - public onlyValidSignatureAndData(signature) view - { - // solhint-disable-previous-line no-empty-blocks - } - - function theWrongMethod(bytes memory) public pure { - // solhint-disable-previous-line no-empty-blocks - } - - function tooShortMsgData() public onlyValidSignatureAndData("") view { - // solhint-disable-previous-line no-empty-blocks - } -} diff --git a/test/cryptography/ECDSA.test.js b/test/cryptography/ECDSA.test.js index 8ddb73700..9f0a4bcb8 100644 --- a/test/cryptography/ECDSA.test.js +++ b/test/cryptography/ECDSA.test.js @@ -14,6 +14,18 @@ contract('ECDSA', function ([_, other]) { this.ecdsa = await ECDSAMock.new(); }); + context('recover with invalid signature', function () { + it('with short signature', async function () { + expect(await this.ecdsa.recover(TEST_MESSAGE, '0x1234')).to.equal(ZERO_ADDRESS); + }); + + it('with long signature', async function () { + // eslint-disable-next-line max-len + expect(await this.ecdsa.recover(TEST_MESSAGE, '0x01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789')) + .to.equal(ZERO_ADDRESS); + }); + }); + context('recover with valid signature', function () { context('with v0 signature', function () { // Signature generated outside ganache with method web3.eth.sign(signer, message) diff --git a/test/drafts/SignatureBouncer.test.js b/test/drafts/SignatureBouncer.test.js deleted file mode 100644 index a03401a25..000000000 --- a/test/drafts/SignatureBouncer.test.js +++ /dev/null @@ -1,223 +0,0 @@ -const { expectRevert } = require('openzeppelin-test-helpers'); -const { getSignFor } = require('../helpers/sign'); -const { shouldBehaveLikePublicRole } = require('../behaviors/access/roles/PublicRole.behavior'); - -const { expect } = require('chai'); - -const SignatureBouncerMock = artifacts.require('SignatureBouncerMock'); - -const UINT_VALUE = 23; -const BYTES_VALUE = web3.utils.toHex('test'); -const INVALID_SIGNATURE = '0xabcd'; - -contract('SignatureBouncer', function ([_, signer, otherSigner, other, authorizedUser, ...otherAccounts]) { - beforeEach(async function () { - this.sigBouncer = await SignatureBouncerMock.new({ from: signer }); - this.signFor = getSignFor(this.sigBouncer, signer); - }); - - describe('signer role', function () { - beforeEach(async function () { - this.contract = this.sigBouncer; - await this.contract.addSigner(otherSigner, { from: signer }); - }); - - shouldBehaveLikePublicRole(signer, otherSigner, otherAccounts, 'signer'); - }); - - describe('modifiers', function () { - context('plain signature', function () { - it('allows valid signature for sender', async function () { - await this.sigBouncer.onlyWithValidSignature(await this.signFor(authorizedUser), { from: authorizedUser }); - }); - - it('does not allow invalid signature for sender', async function () { - await expectRevert( - this.sigBouncer.onlyWithValidSignature(INVALID_SIGNATURE, { from: authorizedUser }), - 'SignatureBouncer: invalid signature for caller' - ); - }); - - it('does not allow valid signature for other sender', async function () { - await expectRevert( - this.sigBouncer.onlyWithValidSignature(await this.signFor(authorizedUser), { from: other }), - 'SignatureBouncer: invalid signature for caller' - ); - }); - - it('does not allow valid signature for method for sender', async function () { - await expectRevert( - this.sigBouncer.onlyWithValidSignature(await this.signFor(authorizedUser, 'onlyWithValidSignature'), - { from: authorizedUser }), 'SignatureBouncer: invalid signature for caller' - ); - }); - }); - - context('method signature', function () { - it('allows valid signature with correct method for sender', async function () { - await this.sigBouncer.onlyWithValidSignatureAndMethod( - await this.signFor(authorizedUser, 'onlyWithValidSignatureAndMethod'), { from: authorizedUser } - ); - }); - - it('does not allow invalid signature with correct method for sender', async function () { - await expectRevert( - this.sigBouncer.onlyWithValidSignatureAndMethod(INVALID_SIGNATURE, { from: authorizedUser }), - 'SignatureBouncer: invalid signature for caller and method' - ); - }); - - it('does not allow valid signature with correct method for other sender', async function () { - await expectRevert( - this.sigBouncer.onlyWithValidSignatureAndMethod( - await this.signFor(authorizedUser, 'onlyWithValidSignatureAndMethod'), { from: other } - ), - 'SignatureBouncer: invalid signature for caller and method' - ); - }); - - it('does not allow valid method signature with incorrect method for sender', async function () { - await expectRevert( - this.sigBouncer.onlyWithValidSignatureAndMethod(await this.signFor(authorizedUser, 'theWrongMethod'), - { from: authorizedUser }), 'SignatureBouncer: invalid signature for caller and method' - ); - }); - - it('does not allow valid non-method signature method for sender', async function () { - await expectRevert( - this.sigBouncer.onlyWithValidSignatureAndMethod(await this.signFor(authorizedUser), { from: authorizedUser }), - 'SignatureBouncer: invalid signature for caller and method' - ); - }); - }); - - context('method and data signature', function () { - it('allows valid signature with correct method and data for sender', async function () { - await this.sigBouncer.onlyWithValidSignatureAndData(UINT_VALUE, - await this.signFor(authorizedUser, 'onlyWithValidSignatureAndData', [UINT_VALUE]), { from: authorizedUser } - ); - }); - - it('does not allow invalid signature with correct method and data for sender', async function () { - await expectRevert( - this.sigBouncer.onlyWithValidSignatureAndData(UINT_VALUE, INVALID_SIGNATURE, { from: authorizedUser }), - 'SignatureBouncer: invalid signature for caller and data' - ); - }); - - it('does not allow valid signature with correct method and incorrect data for sender', async function () { - await expectRevert( - this.sigBouncer.onlyWithValidSignatureAndData(UINT_VALUE + 10, - await this.signFor(authorizedUser, 'onlyWithValidSignatureAndData', [UINT_VALUE]), - { from: authorizedUser } - ), 'SignatureBouncer: invalid signature for caller and data' - ); - }); - - it('does not allow valid signature with correct method and data for other sender', async function () { - await expectRevert( - this.sigBouncer.onlyWithValidSignatureAndData(UINT_VALUE, - await this.signFor(authorizedUser, 'onlyWithValidSignatureAndData', [UINT_VALUE]), - { from: other } - ), 'SignatureBouncer: invalid signature for caller and data' - ); - }); - - it('does not allow valid non-method signature for sender', async function () { - await expectRevert( - this.sigBouncer.onlyWithValidSignatureAndData(UINT_VALUE, - await this.signFor(authorizedUser), { from: authorizedUser } - ), 'SignatureBouncer: invalid signature for caller and data' - ); - }); - - it('does not allow msg.data shorter than SIGNATURE_SIZE', async function () { - await expectRevert( - this.sigBouncer.tooShortMsgData({ from: authorizedUser }), 'SignatureBouncer: data is too short' - ); - }); - }); - }); - - context('signature validation', function () { - context('plain signature', function () { - it('validates valid signature for valid user', async function () { - expect(await this.sigBouncer.checkValidSignature(authorizedUser, await this.signFor(authorizedUser))) - .to.equal(true); - }); - - it('does not validate invalid signature for valid user', async function () { - expect(await this.sigBouncer.checkValidSignature(authorizedUser, INVALID_SIGNATURE)).to.equal(false); - }); - - it('does not validate valid signature for anyone', async function () { - expect(await this.sigBouncer.checkValidSignature(other, await this.signFor(authorizedUser))).to.equal(false); - }); - - it('does not validate valid signature for method for valid user', async function () { - expect(await this.sigBouncer.checkValidSignature( - authorizedUser, await this.signFor(authorizedUser, 'checkValidSignature')) - ).to.equal(false); - }); - }); - - context('method signature', function () { - it('validates valid signature with correct method for valid user', async function () { - expect(await this.sigBouncer.checkValidSignatureAndMethod(authorizedUser, - await this.signFor(authorizedUser, 'checkValidSignatureAndMethod')) - ).to.equal(true); - }); - - it('does not validate invalid signature with correct method for valid user', async function () { - expect(await this.sigBouncer.checkValidSignatureAndMethod(authorizedUser, INVALID_SIGNATURE)).to.equal(false); - }); - - it('does not validate valid signature with correct method for anyone', async function () { - expect(await this.sigBouncer.checkValidSignatureAndMethod(other, - await this.signFor(authorizedUser, 'checkValidSignatureAndMethod')) - ).to.equal(false); - }); - - it('does not validate valid non-method signature with correct method for valid user', async function () { - expect(await this.sigBouncer.checkValidSignatureAndMethod(authorizedUser, await this.signFor(authorizedUser)) - ).to.equal(false); - }); - }); - - context('method and data signature', function () { - it('validates valid signature with correct method and data for valid user', async function () { - expect(await this.sigBouncer.checkValidSignatureAndData(authorizedUser, BYTES_VALUE, UINT_VALUE, - await this.signFor(authorizedUser, 'checkValidSignatureAndData', [authorizedUser, BYTES_VALUE, UINT_VALUE])) - ).to.equal(true); - }); - - it('does not validate invalid signature with correct method and data for valid user', async function () { - expect( - await this.sigBouncer.checkValidSignatureAndData(authorizedUser, BYTES_VALUE, UINT_VALUE, INVALID_SIGNATURE) - ).to.equal(false); - }); - - it('does not validate valid signature with correct method and incorrect data for valid user', - async function () { - expect(await this.sigBouncer.checkValidSignatureAndData(authorizedUser, BYTES_VALUE, UINT_VALUE + 10, - await this.signFor(authorizedUser, 'checkValidSignatureAndData', [authorizedUser, BYTES_VALUE, UINT_VALUE])) - ).to.equal(false); - } - ); - - it('does not validate valid signature with correct method and data for anyone', async function () { - expect(await this.sigBouncer.checkValidSignatureAndData(other, BYTES_VALUE, UINT_VALUE, - await this.signFor(authorizedUser, 'checkValidSignatureAndData', [authorizedUser, BYTES_VALUE, UINT_VALUE])) - ).that.equal(false); - }); - - it('does not validate valid non-method-data signature with correct method and data for valid user', - async function () { - expect(await this.sigBouncer.checkValidSignatureAndData(authorizedUser, BYTES_VALUE, UINT_VALUE, - await this.signFor(authorizedUser, 'checkValidSignatureAndData')) - ).to.equal(false); - } - ); - }); - }); -});