From a3df995141f2416f13fd30b865e16d6f54317aa4 Mon Sep 17 00:00:00 2001 From: Matt Condon Date: Sun, 17 Jun 2018 15:31:10 -0700 Subject: [PATCH 01/10] fix: updates for SignatureBouncer tests and voucher construction --- contracts/access/SignatureBouncer.sol | 4 +-- contracts/ownership/rbac/RBACOwnable.sol | 41 ++++++++++++++++++++++++ test/access/SignatureBouncer.test.js | 8 ++--- test/ownership/rbac/RBACOwnable.test.js | 34 ++++++++++++++++++++ 4 files changed, 79 insertions(+), 8 deletions(-) create mode 100644 contracts/ownership/rbac/RBACOwnable.sol create mode 100644 test/ownership/rbac/RBACOwnable.test.js diff --git a/contracts/access/SignatureBouncer.sol b/contracts/access/SignatureBouncer.sol index d0f8f2a9247..7e8a069c6b3 100644 --- a/contracts/access/SignatureBouncer.sol +++ b/contracts/access/SignatureBouncer.sol @@ -1,6 +1,6 @@ pragma solidity ^0.4.24; -import "../ownership/Ownable.sol"; +import "../ownership/rbac/RBACOwnable.sol"; import "../ownership/rbac/RBAC.sol"; import "../ECRecovery.sol"; @@ -29,7 +29,7 @@ import "../ECRecovery.sol"; * Also non fixed sized parameters make constructing the data in the signature * much more complex. See https://ethereum.stackexchange.com/a/50616 for more details. */ -contract SignatureBouncer is Ownable, RBAC { +contract SignatureBouncer is RBACOwnable { using ECRecovery for bytes32; string public constant ROLE_BOUNCER = "bouncer"; diff --git a/contracts/ownership/rbac/RBACOwnable.sol b/contracts/ownership/rbac/RBACOwnable.sol new file mode 100644 index 00000000000..cb86ab16b27 --- /dev/null +++ b/contracts/ownership/rbac/RBACOwnable.sol @@ -0,0 +1,41 @@ +pragma solidity ^0.4.24; + +import "./RBAC.sol"; + + +/** + * @title RBACOwnable + * @author Matt Condon (@shrugs) + * @dev Ownable logic using RBAC. + * @dev Use RBACOwnable if you could have many different owners and you're ok with + * @dev the security profile of any owner being able to add another owner. + * @dev Only difference from Ownable.sol is that the owners are not stored as public variables. + */ +contract RBACOwnable is RBAC { + string public constant ROLE_OWNER = "owner"; + + constructor() + public + { + addRole(msg.sender, ROLE_OWNER); + } + + modifier onlyOwner() { + checkRole(msg.sender, ROLE_OWNER); + _; + } + + function addOwner(address _owner) + onlyOwner + public + { + addRole(_owner, ROLE_OWNER); + } + + function removeOwner(address _owner) + onlyOwner + public + { + removeRole(_owner, ROLE_OWNER); + } +} diff --git a/test/access/SignatureBouncer.test.js b/test/access/SignatureBouncer.test.js index 8e019701454..86b96789b59 100644 --- a/test/access/SignatureBouncer.test.js +++ b/test/access/SignatureBouncer.test.js @@ -1,7 +1,7 @@ const { assertRevert } = require('../helpers/assertRevert'); const { getBouncerSigner } = require('../helpers/sign'); -const Bouncer = artifacts.require('SignatureBouncerMock'); +const SignatureBouncer = artifacts.require('SignatureBouncerMock'); require('chai') .use(require('chai-as-promised')) @@ -13,15 +13,11 @@ const INVALID_SIGNATURE = '0xabcd'; contract('Bouncer', ([_, owner, anyone, bouncerAddress, authorizedUser]) => { beforeEach(async function () { - this.bouncer = await Bouncer.new({ from: owner }); + this.bouncer = await SignatureBouncer.new({ from: owner }); this.roleBouncer = await this.bouncer.ROLE_BOUNCER(); }); context('management', () => { - it('has a default owner of self', async function () { - (await this.bouncer.owner()).should.eq(owner); - }); - it('allows the owner to add a bouncer', async function () { await this.bouncer.addBouncer(bouncerAddress, { from: owner }); (await this.bouncer.hasRole(bouncerAddress, this.roleBouncer)).should.eq(true); diff --git a/test/ownership/rbac/RBACOwnable.test.js b/test/ownership/rbac/RBACOwnable.test.js new file mode 100644 index 00000000000..df067090c77 --- /dev/null +++ b/test/ownership/rbac/RBACOwnable.test.js @@ -0,0 +1,34 @@ +import assertRevert from '../../helpers/assertRevert'; + +const RBACOwnable = artifacts.require('RBACOwnable'); + +require('chai') + .use(require('chai-as-promised')) + .should(); + +contract('RBAC', function ([_, owner, newOwner, notOwner]) { + before(async function () { + this.ownable = await RBACOwnable.new({ from: owner }); + this.roleOwner = await this.ownable.ROLE_OWNER(); + }); + + it('should not allow notOwner to add or remove owners', async function () { + await assertRevert( + this.ownable.addOwner(newOwner, { from: notOwner }) + ); + + await assertRevert( + this.ownable.removeOwner(owner, { from: notOwner }) + ); + }); + + it('should allow existing owner to add and remove a newOwner', async function () { + await this.ownable.addOwner(newOwner, { from: owner }); + let hasRole = await this.ownable.hasRole(newOwner, this.roleOwner); + hasRole.should.eq(true); + + await this.ownable.removeOwner(newOwner, { from: owner }); + hasRole = await this.ownable.hasRole(newOwner, this.roleOwner); + hasRole.should.eq(false); + }); +}); From 40cd2a7a433a2a5e5689f6ac4d84188f7297025b Mon Sep 17 00:00:00 2001 From: Matt Condon Date: Wed, 11 Jul 2018 13:41:04 -0700 Subject: [PATCH 02/10] feat: implement isValidSignature and update language --- .../{SignatureBouncer.sol => Bouncer.sol} | 76 ++++++++++++------ contracts/access/BouncerDelegate.sol | 31 ++++++++ contracts/access/IBouncerDelegate.sol | 31 ++++++++ contracts/introspection/ERC165Checker.sol | 77 +++++++++++++++++++ contracts/mocks/BouncerDelegateImpl.sol | 40 ++++++++++ contracts/mocks/BouncerMock.sol | 4 +- .../mocks/ERC165/ERC165GenerallySupported.sol | 8 ++ .../mocks/ERC165/ERC165InterfaceSupported.sol | 12 +++ contracts/mocks/ERC165/ERC165NotSupported.sol | 6 ++ contracts/mocks/ERC165CheckerMock.sol | 16 ++++ ...gnatureBouncer.test.js => Bouncer.test.js} | 49 +++++++++++- test/helpers/assertRevert.js | 2 +- test/helpers/sign.js | 23 +++--- test/introspection/ERC165Checker.test.js | 51 ++++++++++++ .../ERC721/ERC721BasicToken.behaviour.js | 2 +- test/token/ERC721/ERC721BasicToken.test.js | 2 +- test/token/ERC721/ERC721Token.test.js | 2 +- 17 files changed, 391 insertions(+), 41 deletions(-) rename contracts/access/{SignatureBouncer.sol => Bouncer.sol} (56%) create mode 100644 contracts/access/BouncerDelegate.sol create mode 100644 contracts/access/IBouncerDelegate.sol create mode 100644 contracts/introspection/ERC165Checker.sol create mode 100644 contracts/mocks/BouncerDelegateImpl.sol create mode 100644 contracts/mocks/ERC165/ERC165GenerallySupported.sol create mode 100644 contracts/mocks/ERC165/ERC165InterfaceSupported.sol create mode 100644 contracts/mocks/ERC165/ERC165NotSupported.sol create mode 100644 contracts/mocks/ERC165CheckerMock.sol rename test/access/{SignatureBouncer.test.js => Bouncer.test.js} (85%) create mode 100644 test/introspection/ERC165Checker.test.js diff --git a/contracts/access/SignatureBouncer.sol b/contracts/access/Bouncer.sol similarity index 56% rename from contracts/access/SignatureBouncer.sol rename to contracts/access/Bouncer.sol index 7e8a069c6b3..0fb3d567604 100644 --- a/contracts/access/SignatureBouncer.sol +++ b/contracts/access/Bouncer.sol @@ -1,27 +1,35 @@ pragma solidity ^0.4.24; +import "../introspection/ERC165Checker.sol"; +import "./IBouncerDelegate.sol"; import "../ownership/rbac/RBACOwnable.sol"; import "../ownership/rbac/RBAC.sol"; import "../ECRecovery.sol"; /** - * @title SignatureBouncer + * @title Bouncer * @author PhABC, Shrugs and aflesher - * @dev Bouncer allows users to submit a signature as a permission to do an action. - * If the signature is from one of the authorized bouncer addresses, the signature - * is valid. The owner of the contract adds/removes bouncers. - * Bouncer addresses can be individual servers signing grants or different + * @dev Bouncer allows users to submit a `ticket` from a `delegate` as a permission to do an action. + * A ticket is a cryptographic signature generated via + * Ethereum ECDSA signing (web3.eth.personal_sign). + * Tickets must be a signature (with `Ethereum Signed Message:` prefix) of the hash of the verified + * information. See //test/helpers/sign.js for example ticket construction. + * + * If the ticket is from one of the authorized delegates, the ticket + * is valid. The owner of the contract adds/removes delegates. + * Delegates can be individual servers signing grants or different * users within a decentralized club that have permission to invite other members. + * Delegates can also be other contracts that implement `isValidSignature`, allowing you to write + * whatever access-control logic you want, like using alternative signature schemes. + * * This technique is useful for whitelists and airdrops; instead of putting all * valid addresses on-chain, simply sign a grant of the form - * keccak256(abi.encodePacked(`:contractAddress` + `:granteeAddress`)) using a valid bouncer address. - * Then restrict access to your crowdsale/whitelist/airdrop using the - * `onlyValidSignature` modifier (or implement your own using isValidSignature). - * In addition to `onlyValidSignature`, `onlyValidSignatureAndMethod` and - * `onlyValidSignatureAndData` can be used to restrict access to only a given method - * or a given method with given parameters respectively. - * See the tests Bouncer.test.js for specific usage examples. + * keccak256(abi.encodePacked(`:contractAddress` + `:granteeAddress`)) using a valid signer. + * + * Then restrict access to your crowdsale/whitelist/airdrop using the `onlyValidSignatureAndData` + * modifier, which allows users claiming your tokens to pay their own gas. + * * @notice A method that uses the `onlyValidSignatureAndData` modifier must make the _sig * parameter the "last" parameter. You cannot sign a message that has its own * signature in it so the last 128 bytes of msg.data (which represents the @@ -29,16 +37,20 @@ import "../ECRecovery.sol"; * Also non fixed sized parameters make constructing the data in the signature * much more complex. See https://ethereum.stackexchange.com/a/50616 for more details. */ -contract SignatureBouncer is RBACOwnable { +contract Bouncer is RBACOwnable { using ECRecovery for bytes32; + using ERC165Checker for address; - string public constant ROLE_BOUNCER = "bouncer"; + // @TODO - de-duplicate this line from IBouncerDelegate once sharing constants is possible + bytes4 internal constant InterfaceId_BouncerDelegate = 0x1626ba7e; + string public constant ROLE_DELEGATE = "delegate"; uint constant METHOD_ID_SIZE = 4; // signature size is 65 bytes (tightly packed v + r + s), but gets padded to 96 bytes uint constant SIGNATURE_SIZE = 96; /** * @dev requires that a valid signature of a bouncer was provided + * @notice does not validate method arguments */ modifier onlyValidSignature(bytes _sig) { @@ -48,6 +60,7 @@ contract SignatureBouncer is RBACOwnable { /** * @dev requires that a valid signature with a specifed method of a bouncer was provided + * @notice validates methodId, but not method arguments */ modifier onlyValidSignatureAndMethod(bytes _sig) { @@ -57,6 +70,7 @@ contract SignatureBouncer is RBACOwnable { /** * @dev requires that a valid signature with a specifed method and params of a bouncer was provided + * @notice verifies entire calldata */ modifier onlyValidSignatureAndData(bytes _sig) { @@ -65,25 +79,25 @@ contract SignatureBouncer is RBACOwnable { } /** - * @dev allows the owner to add additional bouncer addresses + * @dev allows the owner to add additional signer addresses */ - function addBouncer(address _bouncer) + function addDelegate(address _delegate) onlyOwner public { - require(_bouncer != address(0)); - addRole(_bouncer, ROLE_BOUNCER); + require(_delegate != address(0)); + addRole(_delegate, ROLE_DELEGATE); } /** - * @dev allows the owner to remove bouncer addresses + * @dev allows the owner to remove signer addresses */ - function removeBouncer(address _bouncer) + function removeDelegate(address _delegate) onlyOwner public { - require(_bouncer != address(0)); - removeRole(_bouncer, ROLE_BOUNCER); + require(_delegate != address(0)); + removeRole(_delegate, ROLE_DELEGATE); } /** @@ -146,14 +160,26 @@ contract SignatureBouncer is RBACOwnable { * and then recover the signature and check it against the bouncer role * @return bool */ - function isValidDataHash(bytes32 hash, bytes _sig) + function isValidDataHash(bytes32 _hash, bytes _sig) internal view returns (bool) { - address signer = hash + // if the sender is a delegate AND supports isValidSignature, we delegate validation to them + // this allows someone who wants custom validation logic (perhaps they check another contract's state) + // to code their own delegate. The users then submit actions to the delegate contract, which proxies + // them to this contract, which then pings-back to check if it's cool. + // This is also useful for contracts-as-identities: your identity contract implements isValidSignature + // and can then recover the signer and check it against the whitelisted ACTION keys. + if (hasRole(msg.sender, ROLE_DELEGATE) && msg.sender.supportsInterface(InterfaceId_BouncerDelegate)) { + return IBouncerDelegate(msg.sender).isValidSignature(_hash, _sig); + } + + // otherwise it's an ECDSA Ethereum Signed Message hash, + // so recover and check the signer's delegate status + address signer = _hash .toEthSignedMessageHash() .recover(_sig); - return hasRole(signer, ROLE_BOUNCER); + return hasRole(signer, ROLE_DELEGATE); } } diff --git a/contracts/access/BouncerDelegate.sol b/contracts/access/BouncerDelegate.sol new file mode 100644 index 00000000000..2131befddca --- /dev/null +++ b/contracts/access/BouncerDelegate.sol @@ -0,0 +1,31 @@ +pragma solidity 0.4.24; + +import "./IBouncerDelegate.sol"; +import "../introspection/SupportsInterfaceWithLookup.sol"; + +/** + * @title BouncerDelegate + * @dev Partial implementation of IBouncerDelegate that adds ERC165 support + * But you as the developer still have to add your signature validation logic. + * As a delegate, you must proxy calls from accounts you consider valid. + */ +contract BouncerDelegate is IBouncerDelegate, SupportsInterfaceWithLookup { + + constructor () + public + { + _registerInterface(InterfaceId_BouncerDelegate); + } + + /** + * @dev See IBouncerDelegate#isValidSignature for more info + * Override this function to supply implementation. + */ + function isValidSignature( + bytes32 _hash, + bytes _sig + ) + external + view + returns (bool); +} diff --git a/contracts/access/IBouncerDelegate.sol b/contracts/access/IBouncerDelegate.sol new file mode 100644 index 00000000000..fe5810a69b1 --- /dev/null +++ b/contracts/access/IBouncerDelegate.sol @@ -0,0 +1,31 @@ +pragma solidity 0.4.24; + +import "../introspection/ERC165.sol"; + +/** + * @title IBouncerDelegate + * @dev Implement this interface so that your contract can be a Bouncer's delegate. + * inspired by https://github.com/0xProject/0x-monorepo/blob/v2-prototype/packages/contracts/src/2.0.0/protocol/Exchange/interfaces/IWallet.sol + */ +contract IBouncerDelegate is ERC165 { + + bytes4 internal constant InterfaceId_BouncerDelegate = 0x1626ba7e; + /** + * 0x1626ba7e === + * bytes4(keccak256('isValidSignature(bytes32,bytes)')) + */ + + /** + * @dev verifies that a signature of a hash is valid + * @param _hash message hash that is signed + * @param _sig the provided signature + * @return bool validity of signature for the hash + */ + function isValidSignature( + bytes32 _hash, + bytes _sig + ) + external + view + returns (bool); +} diff --git a/contracts/introspection/ERC165Checker.sol b/contracts/introspection/ERC165Checker.sol new file mode 100644 index 00000000000..c500f9d388e --- /dev/null +++ b/contracts/introspection/ERC165Checker.sol @@ -0,0 +1,77 @@ +pragma solidity ^0.4.24; + + +/** + * @title ERC165Checker + * @dev Use `using ERC165Checker for address;` to include this library + */ +library ERC165Checker { + + bytes4 private constant InterfaceId_Invalid = 0xffffffff; + bytes4 private constant InterfaceId_ERC165 = 0x01ffc9a7; + + /** + * @notice Query if a contract implements an interface + * @param _interfaceId The interface identifier, as specified in ERC-165 + * @dev Interface identification is specified in ERC-165. + */ + function supportsInterface(address _address, bytes4 _interfaceId) + internal + view + returns (bool) + { + uint256 success; + uint256 result; + + // supports ERC165 (probably)? + (success, result) = noThrowCall(_address, InterfaceId_ERC165); + if ((success == 0) || (result == 0)) { + return false; + } + + // definitely supports ERC165? + (success, result) = noThrowCall(_address, InterfaceId_Invalid); + if ((success == 0) || (result != 0)) { + return false; + } + + // supports the _interfaceId we care about? + (success, result) = noThrowCall(_address, _interfaceId); + if ((success == 1) && (result == 1)) { + return true; + } + + return false; + } + + /** + * @dev noThrowCall via https://github.com/ethereum/EIPs/blob/master/EIPS/eip-165.md + */ + function noThrowCall( + address _address, + bytes4 _interfaceId + ) + internal + view + returns (uint256 success, uint256 result) + { + bytes4 erc165ID = InterfaceId_ERC165; + + // solium-disable-next-line security/no-inline-assembly + assembly { + let x := mload(0x40) // Find empty storage location using "free memory pointer" + mstore(x, erc165ID) // Place signature at begining of empty storage to call supportsInterface() + mstore(add(x, 0x04), _interfaceId) // Place first argument directly next to signature + + success := staticcall( + 30000, // 30k gas + _address, // To addr + x, // Inputs are stored at location x + 0x20, // Inputs are 32 bytes long + x, // Store output over input (saves space) + 0x20) // Outputs are 32 bytes long + + result := mload(x) // Load the result + } + } +} diff --git a/contracts/mocks/BouncerDelegateImpl.sol b/contracts/mocks/BouncerDelegateImpl.sol new file mode 100644 index 00000000000..5a009698b8c --- /dev/null +++ b/contracts/mocks/BouncerDelegateImpl.sol @@ -0,0 +1,40 @@ +pragma solidity 0.4.24; + +import "../access/BouncerDelegate.sol"; +import "./BouncerMock.sol"; + +contract BouncerDelegateImpl is BouncerDelegate { + + bool public shouldPass = false; + BouncerMock public bouncer; + + constructor (bool _shouldPass, BouncerMock _bouncer) + public + { + shouldPass = _shouldPass; + bouncer = _bouncer; + } + + function forward() + public + { + bouncer.onlyWithValidSignature(""); + } + + /** + * @dev verifies that a signature of a hash is valid + * @param _hash message hash that is signed + * @param _sig the provided signature + * @return bool validity of signature for the hash + */ + function isValidSignature( + bytes32 _hash, + bytes _sig + ) + external + view + returns (bool) + { + return shouldPass; + } +} diff --git a/contracts/mocks/BouncerMock.sol b/contracts/mocks/BouncerMock.sol index 65009b0b040..89a9f46a2e3 100644 --- a/contracts/mocks/BouncerMock.sol +++ b/contracts/mocks/BouncerMock.sol @@ -1,9 +1,9 @@ pragma solidity ^0.4.24; -import "../access/SignatureBouncer.sol"; +import "../access/Bouncer.sol"; -contract SignatureBouncerMock is SignatureBouncer { +contract BouncerMock is Bouncer { function checkValidSignature(address _address, bytes _sig) public view diff --git a/contracts/mocks/ERC165/ERC165GenerallySupported.sol b/contracts/mocks/ERC165/ERC165GenerallySupported.sol new file mode 100644 index 00000000000..64c63dacc28 --- /dev/null +++ b/contracts/mocks/ERC165/ERC165GenerallySupported.sol @@ -0,0 +1,8 @@ +pragma solidity ^0.4.24; + +import "../../introspection/SupportsInterfaceWithLookup.sol"; + + +contract ERC165GenerallySupported is SupportsInterfaceWithLookup { + +} diff --git a/contracts/mocks/ERC165/ERC165InterfaceSupported.sol b/contracts/mocks/ERC165/ERC165InterfaceSupported.sol new file mode 100644 index 00000000000..10d32965238 --- /dev/null +++ b/contracts/mocks/ERC165/ERC165InterfaceSupported.sol @@ -0,0 +1,12 @@ +pragma solidity ^0.4.24; + +import "../../introspection/SupportsInterfaceWithLookup.sol"; + + +contract ERC165InterfaceSupported is SupportsInterfaceWithLookup { + constructor (bytes4 _interfaceId) + public + { + _registerInterface(_interfaceId); + } +} diff --git a/contracts/mocks/ERC165/ERC165NotSupported.sol b/contracts/mocks/ERC165/ERC165NotSupported.sol new file mode 100644 index 00000000000..763f8fabadf --- /dev/null +++ b/contracts/mocks/ERC165/ERC165NotSupported.sol @@ -0,0 +1,6 @@ +pragma solidity ^0.4.24; + + +contract ERC165NotSupported { + +} diff --git a/contracts/mocks/ERC165CheckerMock.sol b/contracts/mocks/ERC165CheckerMock.sol new file mode 100644 index 00000000000..249cbc785d2 --- /dev/null +++ b/contracts/mocks/ERC165CheckerMock.sol @@ -0,0 +1,16 @@ +pragma solidity ^0.4.24; + +import "../introspection/ERC165Checker.sol"; + + +contract ERC165CheckerMock { + using ERC165Checker for address; + + function supportsInterface(address _address, bytes4 _interfaceId) + public + view + returns (bool) + { + return _address.supportsInterface(_interfaceId); + } +} diff --git a/test/access/SignatureBouncer.test.js b/test/access/Bouncer.test.js similarity index 85% rename from test/access/SignatureBouncer.test.js rename to test/access/Bouncer.test.js index 86b96789b59..fae7f542e19 100644 --- a/test/access/SignatureBouncer.test.js +++ b/test/access/Bouncer.test.js @@ -1,7 +1,9 @@ const { assertRevert } = require('../helpers/assertRevert'); const { getBouncerSigner } = require('../helpers/sign'); +const makeInterfaceId = require('../helpers/makeInterfaceId'); -const SignatureBouncer = artifacts.require('SignatureBouncerMock'); +const Bouncer = artifacts.require('BouncerMock'); +const BouncerDelegateImpl = artifacts.require('BouncerDelegateImpl'); require('chai') .use(require('chai-as-promised')) @@ -238,4 +240,49 @@ contract('Bouncer', ([_, owner, anyone, bouncerAddress, authorizedUser]) => { }); }); }); + + context('contract delegate', () => { + context('not a delegate', () => { + beforeEach(async function () { + this.delegateContract = await BouncerDelegateImpl.new(true, this.bouncer.address, { from: owner }); + }); + + it('should fail', async function () { + await assertRevert( + this.delegateContract.forward({ from: anyone }) + ); + }); + }); + + context('invalid delegate', () => { + beforeEach(async function () { + this.delegateContract = await BouncerDelegateImpl.new(false, this.bouncer.address, { from: owner }); + await this.bouncer.addDelegate(this.delegateContract.address, { from: owner }); + }); + + it('should be invalid', async function () { + await assertRevert( + this.delegateContract.forward({ from: anyone }) + ); + }); + }); + + context('valid delegate', () => { + beforeEach(async function () { + this.delegateContract = await BouncerDelegateImpl.new(true, this.bouncer.address, { from: owner }); + await this.bouncer.addDelegate(this.delegateContract.address, { from: owner }); + }); + + it('should support isValidSignature', async function () { + const supported = await this.delegateContract.supportsInterface(makeInterfaceId([ + 'isValidSignature(bytes32,bytes)', + ])); + supported.should.eq(true); + }); + + it('should be valid', async function () { + await this.delegateContract.forward({ from: anyone }); + }); + }); + }); }); diff --git a/test/helpers/assertRevert.js b/test/helpers/assertRevert.js index b82e0f195b5..0c4ed094cdf 100644 --- a/test/helpers/assertRevert.js +++ b/test/helpers/assertRevert.js @@ -1,7 +1,7 @@ async function assertRevert (promise) { try { await promise; - assert.fail('Expected revert not received'); + throw new Error('Transaction succeeded when it shouldn\'t have'); } catch (error) { const revertFound = error.message.search('revert') >= 0; assert(revertFound, `Expected "revert", got ${error} instead`); diff --git a/test/helpers/sign.js b/test/helpers/sign.js index a991d10a378..a732d22d634 100644 --- a/test/helpers/sign.js +++ b/test/helpers/sign.js @@ -3,11 +3,10 @@ const { soliditySha3 } = require('web3-utils'); const REAL_SIGNATURE_SIZE = 2 * 65; // 65 bytes in hexadecimal string legnth const PADDED_SIGNATURE_SIZE = 2 * 96; // 96 bytes in hexadecimal string length - -const DUMMY_SIGNATURE = `0x${web3.padLeft('', REAL_SIGNATURE_SIZE)}`; +const DUMMY_SIGNATURE = `0x${web3.padLeft('', REAL_SIGNATURE_SIZE)}`; // '0x' plus 130 '0's /** - * Hash and add same prefix to the hash that ganache use. + * Hash and add same prefix to the hash that ganache uses. * @param {string} message the plaintext/ascii/original message * @return {string} the hash of the message, prefixed, and then hashed again */ @@ -26,6 +25,7 @@ const signMessage = (signer, message = '') => { // @TODO - remove this when we migrate to web3-1.0.0 const transformToFullName = function (json) { if (json.name.indexOf('(') !== -1) { + console.log(json); return json.name; } @@ -53,22 +53,27 @@ const getBouncerSigner = (contract, signer) => (redeemer, methodName, methodArgs // if we have a method, add it to the parts that we're signing if (methodName) { if (methodArgs.length > 0) { + // construct the transaction data, but sub out the signature part + // (necessary to correctly calculate the offsets for any variable-length arguments parts.push( - contract.contract[methodName].getData(...methodArgs.concat([DUMMY_SIGNATURE])).slice( - 0, - -1 * PADDED_SIGNATURE_SIZE - ) + contract.contract[methodName] + .getData(...methodArgs.concat([DUMMY_SIGNATURE])) + .slice( + 0, + -1 * PADDED_SIGNATURE_SIZE + ) ); } else { const abi = contract.abi.find(abi => abi.name === methodName); const name = transformToFullName(abi); - const signature = web3.sha3(name).slice(0, 10); + const signature = web3.sha3(name).slice(0, 10); // 0xabcd parts.push(signature); } } - // ^ substr to remove `0x` because in solidity the address is a set of byes, not a string `0xabcd` + // hash the method data const hashOfMessage = soliditySha3(...parts); + // then sign that hash return signMessage(signer, hashOfMessage); }; diff --git a/test/introspection/ERC165Checker.test.js b/test/introspection/ERC165Checker.test.js new file mode 100644 index 00000000000..50ef5c0de35 --- /dev/null +++ b/test/introspection/ERC165Checker.test.js @@ -0,0 +1,51 @@ +import assertRevert from '../helpers/assertRevert'; + +const ERC165Checker = artifacts.require('ERC165CheckerMock'); +const ERC165NotSupported = artifacts.require('ERC165NotSupported'); +const ERC165GenerallySupported = artifacts.require('ERC165GenerallySupported'); +const ERC165InterfaceSupported = artifacts.require('ERC165InterfaceSupported'); + +const DUMMY_ID = '0xdeadbeef'; + +require('chai') + .use(require('chai-as-promised')) + .should(); + +contract('ERC165Checker', function (accounts) { + before(async function () { + this.mock = await ERC165Checker.new(); + }); + + context('not supported', () => { + beforeEach(async function () { + this.target = await ERC165NotSupported.new(); + }); + + it('fails', async function () { + const supported = await this.mock.supportsInterface(this.target.address, DUMMY_ID); + supported.should.eq(false); + }); + }); + + context('generally supported', () => { + beforeEach(async function () { + this.target = await ERC165GenerallySupported.new(); + }); + + it('fails', async function () { + const supported = await this.mock.supportsInterface(this.target.address, DUMMY_ID); + supported.should.eq(false); + }); + }); + + context('interface supported', () => { + beforeEach(async function () { + this.target = await ERC165InterfaceSupported.new(DUMMY_ID); + }); + + it('succeeds', async function () { + const supported = await this.mock.supportsInterface(this.target.address, DUMMY_ID); + supported.should.eq(true); + }); + }); +}); diff --git a/test/token/ERC721/ERC721BasicToken.behaviour.js b/test/token/ERC721/ERC721BasicToken.behaviour.js index bf14a573a78..9b688df8d90 100644 --- a/test/token/ERC721/ERC721BasicToken.behaviour.js +++ b/test/token/ERC721/ERC721BasicToken.behaviour.js @@ -4,7 +4,7 @@ const { decodeLogs } = require('../../helpers/decodeLogs'); const { sendTransaction } = require('../../helpers/sendTransaction'); const _ = require('lodash'); -const ERC721Receiver = artifacts.require('ERC721ReceiverMock.sol'); +const ERC721Receiver = artifacts.require('ERC721ReceiverMock'); const BigNumber = web3.BigNumber; require('chai') diff --git a/test/token/ERC721/ERC721BasicToken.test.js b/test/token/ERC721/ERC721BasicToken.test.js index 65aeb12d5a0..a84af6f13ae 100644 --- a/test/token/ERC721/ERC721BasicToken.test.js +++ b/test/token/ERC721/ERC721BasicToken.test.js @@ -2,7 +2,7 @@ const { shouldBehaveLikeERC721BasicToken } = require('./ERC721BasicToken.behavio const { shouldBehaveLikeMintAndBurnERC721Token } = require('./ERC721MintBurn.behaviour'); const BigNumber = web3.BigNumber; -const ERC721BasicToken = artifacts.require('ERC721BasicTokenMock.sol'); +const ERC721BasicToken = artifacts.require('ERC721BasicTokenMock'); require('chai') .use(require('chai-as-promised')) diff --git a/test/token/ERC721/ERC721Token.test.js b/test/token/ERC721/ERC721Token.test.js index 1cd9240a72b..65167453902 100644 --- a/test/token/ERC721/ERC721Token.test.js +++ b/test/token/ERC721/ERC721Token.test.js @@ -5,7 +5,7 @@ const { shouldSupportInterfaces } = require('../../introspection/SupportsInterfa const _ = require('lodash'); const BigNumber = web3.BigNumber; -const ERC721Token = artifacts.require('ERC721TokenMock.sol'); +const ERC721Token = artifacts.require('ERC721TokenMock'); require('chai') .use(require('chai-as-promised')) From 84c63f5a5a083b7e8ec3aa6249ab7ca888453655 Mon Sep 17 00:00:00 2001 From: Matt Condon Date: Wed, 11 Jul 2018 16:09:43 -0700 Subject: [PATCH 03/10] fix: linting errors --- contracts/access/Bouncer.sol | 3 ++- contracts/access/BouncerDelegate.sol | 1 + contracts/mocks/BouncerDelegateImpl.sol | 14 ++++++++------ test/introspection/ERC165Checker.test.js | 2 -- 4 files changed, 11 insertions(+), 9 deletions(-) diff --git a/contracts/access/Bouncer.sol b/contracts/access/Bouncer.sol index 0fb3d567604..022bfcd4241 100644 --- a/contracts/access/Bouncer.sol +++ b/contracts/access/Bouncer.sol @@ -171,7 +171,8 @@ contract Bouncer is RBACOwnable { // them to this contract, which then pings-back to check if it's cool. // This is also useful for contracts-as-identities: your identity contract implements isValidSignature // and can then recover the signer and check it against the whitelisted ACTION keys. - if (hasRole(msg.sender, ROLE_DELEGATE) && msg.sender.supportsInterface(InterfaceId_BouncerDelegate)) { + if (hasRole(msg.sender, ROLE_DELEGATE) && + msg.sender.supportsInterface(InterfaceId_BouncerDelegate)) { return IBouncerDelegate(msg.sender).isValidSignature(_hash, _sig); } diff --git a/contracts/access/BouncerDelegate.sol b/contracts/access/BouncerDelegate.sol index 2131befddca..73aa7b1ab4a 100644 --- a/contracts/access/BouncerDelegate.sol +++ b/contracts/access/BouncerDelegate.sol @@ -3,6 +3,7 @@ pragma solidity 0.4.24; import "./IBouncerDelegate.sol"; import "../introspection/SupportsInterfaceWithLookup.sol"; + /** * @title BouncerDelegate * @dev Partial implementation of IBouncerDelegate that adds ERC165 support diff --git a/contracts/mocks/BouncerDelegateImpl.sol b/contracts/mocks/BouncerDelegateImpl.sol index 5a009698b8c..6631e489d4e 100644 --- a/contracts/mocks/BouncerDelegateImpl.sol +++ b/contracts/mocks/BouncerDelegateImpl.sol @@ -3,6 +3,7 @@ pragma solidity 0.4.24; import "../access/BouncerDelegate.sol"; import "./BouncerMock.sol"; + contract BouncerDelegateImpl is BouncerDelegate { bool public shouldPass = false; @@ -15,12 +16,6 @@ contract BouncerDelegateImpl is BouncerDelegate { bouncer = _bouncer; } - function forward() - public - { - bouncer.onlyWithValidSignature(""); - } - /** * @dev verifies that a signature of a hash is valid * @param _hash message hash that is signed @@ -37,4 +32,11 @@ contract BouncerDelegateImpl is BouncerDelegate { { return shouldPass; } + + function forward() + public + { + bouncer.onlyWithValidSignature(""); + } + } diff --git a/test/introspection/ERC165Checker.test.js b/test/introspection/ERC165Checker.test.js index 50ef5c0de35..698a7095059 100644 --- a/test/introspection/ERC165Checker.test.js +++ b/test/introspection/ERC165Checker.test.js @@ -1,5 +1,3 @@ -import assertRevert from '../helpers/assertRevert'; - const ERC165Checker = artifacts.require('ERC165CheckerMock'); const ERC165NotSupported = artifacts.require('ERC165NotSupported'); const ERC165GenerallySupported = artifacts.require('ERC165GenerallySupported'); From a07eca71c01742226009d33d897ecce530afd374 Mon Sep 17 00:00:00 2001 From: Matt Condon Date: Tue, 17 Jul 2018 15:10:21 -0400 Subject: [PATCH 04/10] fix: change RBACOwnable#onlyOwner to onlyOwners --- contracts/access/Bouncer.sol | 4 ++-- contracts/ownership/rbac/RBACOwnable.sol | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/contracts/access/Bouncer.sol b/contracts/access/Bouncer.sol index 022bfcd4241..1c07599501a 100644 --- a/contracts/access/Bouncer.sol +++ b/contracts/access/Bouncer.sol @@ -82,7 +82,7 @@ contract Bouncer is RBACOwnable { * @dev allows the owner to add additional signer addresses */ function addDelegate(address _delegate) - onlyOwner + onlyOwners public { require(_delegate != address(0)); @@ -93,7 +93,7 @@ contract Bouncer is RBACOwnable { * @dev allows the owner to remove signer addresses */ function removeDelegate(address _delegate) - onlyOwner + onlyOwners public { require(_delegate != address(0)); diff --git a/contracts/ownership/rbac/RBACOwnable.sol b/contracts/ownership/rbac/RBACOwnable.sol index cb86ab16b27..e6b1087c2f9 100644 --- a/contracts/ownership/rbac/RBACOwnable.sol +++ b/contracts/ownership/rbac/RBACOwnable.sol @@ -20,20 +20,20 @@ contract RBACOwnable is RBAC { addRole(msg.sender, ROLE_OWNER); } - modifier onlyOwner() { + modifier onlyOwners() { checkRole(msg.sender, ROLE_OWNER); _; } function addOwner(address _owner) - onlyOwner + onlyOwners public { addRole(_owner, ROLE_OWNER); } function removeOwner(address _owner) - onlyOwner + onlyOwners public { removeRole(_owner, ROLE_OWNER); From f11aa5c2d65ecb7ed278d8db75433644a99e2c14 Mon Sep 17 00:00:00 2001 From: Matt Condon Date: Mon, 16 Jul 2018 18:42:18 -0400 Subject: [PATCH 05/10] checkpoint: refactor out delegate contant --- contracts/access/Bouncer.sol | 166 +++++++++++++++--------- contracts/access/BouncerDelegate.sol | 2 +- contracts/access/IBouncerDelegate.sol | 2 +- contracts/mocks/BouncerDelegateImpl.sol | 2 +- contracts/mocks/BouncerMock.sol | 24 ++-- test/access/Bouncer.test.js | 131 ++++++++++++++++--- 6 files changed, 235 insertions(+), 92 deletions(-) diff --git a/contracts/access/Bouncer.sol b/contracts/access/Bouncer.sol index 1c07599501a..735d5b81129 100644 --- a/contracts/access/Bouncer.sol +++ b/contracts/access/Bouncer.sol @@ -2,8 +2,8 @@ pragma solidity ^0.4.24; import "../introspection/ERC165Checker.sol"; import "./IBouncerDelegate.sol"; +import "./BouncerDelegate.sol"; import "../ownership/rbac/RBACOwnable.sol"; -import "../ownership/rbac/RBAC.sol"; import "../ECRecovery.sol"; @@ -11,39 +11,39 @@ import "../ECRecovery.sol"; * @title Bouncer * @author PhABC, Shrugs and aflesher * @dev Bouncer allows users to submit a `ticket` from a `delegate` as a permission to do an action. - * A ticket is a cryptographic signature generated via - * Ethereum ECDSA signing (web3.eth.personal_sign). - * Tickets must be a signature (with `Ethereum Signed Message:` prefix) of the hash of the verified - * information. See //test/helpers/sign.js for example ticket construction. + * By default a `ticket` is a cryptographic signature + * generated via Ethereum ECDSA signing (web3.eth.personal_sign). + * See //test/helpers/sign.js for example ticket construction. + * + * If the ticket is from one of the authorized delegates, the ticket is valid. + The owner of the contract adds/removes delegates. * - * If the ticket is from one of the authorized delegates, the ticket - * is valid. The owner of the contract adds/removes delegates. * Delegates can be individual servers signing grants or different * users within a decentralized club that have permission to invite other members. + * * Delegates can also be other contracts that implement `isValidSignature`, allowing you to write - * whatever access-control logic you want, like using alternative signature schemes. + * whatever access-control logic you want, like using alternative signature schemes or + * contract-as-identity patterns. * - * This technique is useful for whitelists and airdrops; instead of putting all + * This Bouncer technique is useful for whitelists and airdrops; instead of putting all * valid addresses on-chain, simply sign a grant of the form - * keccak256(abi.encodePacked(`:contractAddress` + `:granteeAddress`)) using a valid signer. + * keccak256(abi.encodePacked(`:delegate` + `:sender`)) using a valid signer. * - * Then restrict access to your crowdsale/whitelist/airdrop using the `onlyValidSignatureAndData` - * modifier, which allows users claiming your tokens to pay their own gas. + * Then restrict access to your crowdsale/whitelist/airdrop using one of the only* modifiers, + * which allows users claiming your tokens to pay their own gas. * - * @notice A method that uses the `onlyValidSignatureAndData` modifier must make the _sig + * @notice A method that uses the `onlyValidTicketForData` modifier must make the _sig * parameter the "last" parameter. You cannot sign a message that has its own - * signature in it so the last 128 bytes of msg.data (which represents the - * length of the _sig data and the _sig data itself) is ignored when validating. - * Also non fixed sized parameters make constructing the data in the signature - * much more complex. See https://ethereum.stackexchange.com/a/50616 for more details. + * signature in it so the last 96 bytes of msg.data (which represents the _sig data itself) + * is ignored when constructing the data hash that is validated. */ -contract Bouncer is RBACOwnable { +contract Bouncer is RBACOwnable, BouncerDelegate { using ECRecovery for bytes32; using ERC165Checker for address; - // @TODO - de-duplicate this line from IBouncerDelegate once sharing constants is possible - bytes4 internal constant InterfaceId_BouncerDelegate = 0x1626ba7e; string public constant ROLE_DELEGATE = "delegate"; + + // method ids are 4 bytes long uint constant METHOD_ID_SIZE = 4; // signature size is 65 bytes (tightly packed v + r + s), but gets padded to 96 bytes uint constant SIGNATURE_SIZE = 96; @@ -52,9 +52,9 @@ contract Bouncer is RBACOwnable { * @dev requires that a valid signature of a bouncer was provided * @notice does not validate method arguments */ - modifier onlyValidSignature(bytes _sig) + modifier onlyValidTicket(address _delegate, bytes _sig) { - require(isValidSignature(msg.sender, _sig)); + require(isValidTicket(_delegate, msg.sender, _sig)); _; } @@ -62,22 +62,29 @@ contract Bouncer is RBACOwnable { * @dev requires that a valid signature with a specifed method of a bouncer was provided * @notice validates methodId, but not method arguments */ - modifier onlyValidSignatureAndMethod(bytes _sig) + modifier onlyValidTicketForMethod(address _delegate, bytes _sig) { - require(isValidSignatureAndMethod(msg.sender, _sig)); + require(isValidTicketAndMethod(_delegate, msg.sender, _sig)); _; } /** * @dev requires that a valid signature with a specifed method and params of a bouncer was provided - * @notice verifies entire calldata + * @notice verifies entire calldata, sans final signature */ - modifier onlyValidSignatureAndData(bytes _sig) + modifier onlyValidTicketForData(address _delegate, bytes _sig) { - require(isValidSignatureAndData(msg.sender, _sig)); + require(isValidTicketAndData(_delegate, msg.sender, _sig)); _; } + constructor() + public + { + // this contract implements IBouncerDelegate for all of its EOA delegate accounts. + addRole(address(this), ROLE_DELEGATE); + } + /** * @dev allows the owner to add additional signer addresses */ @@ -104,13 +111,14 @@ contract Bouncer is RBACOwnable { * @dev is the signature of `this + sender` from a bouncer? * @return bool */ - function isValidSignature(address _address, bytes _sig) + function isValidTicket(address _delegate, address _sender, bytes _sig) internal view returns (bool) { - return isValidDataHash( - keccak256(abi.encodePacked(address(this), _address)), + return isDelegatedSignatureValidForHash( + _delegate, + keccak256(abi.encodePacked(_delegate, _sender)), _sig ); } @@ -119,17 +127,14 @@ contract Bouncer is RBACOwnable { * @dev is the signature of `this + sender + methodId` from a bouncer? * @return bool */ - function isValidSignatureAndMethod(address _address, bytes _sig) + function isValidTicketAndMethod(address _delegate, address _sender, bytes _sig) internal view returns (bool) { - bytes memory data = new bytes(METHOD_ID_SIZE); - for (uint i = 0; i < data.length; i++) { - data[i] = msg.data[i]; - } - return isValidDataHash( - keccak256(abi.encodePacked(address(this), _address, data)), + return isDelegatedSignatureValidForHash( + _delegate, + keccak256(abi.encodePacked(_delegate, _sender, getMethodId())), _sig ); } @@ -139,48 +144,89 @@ contract Bouncer is RBACOwnable { * @notice the _sig parameter of the method being validated must be the "last" parameter * @return bool */ - function isValidSignatureAndData(address _address, bytes _sig) + function isValidTicketAndData(address _delegate, address _sender, bytes _sig) internal view returns (bool) { - require(msg.data.length > SIGNATURE_SIZE); - bytes memory data = new bytes(msg.data.length - SIGNATURE_SIZE); - for (uint i = 0; i < data.length; i++) { - data[i] = msg.data[i]; - } - return isValidDataHash( - keccak256(abi.encodePacked(address(this), _address, data)), + return isDelegatedSignatureValidForHash( + _delegate, + keccak256(abi.encodePacked(_delegate, _sender, getMessageData())), _sig ); } /** - * @dev internal function to convert a hash to an eth signed message - * and then recover the signature and check it against the bouncer role - * @return bool + * @dev tests to see if the signature is valid according to the delegate + * The delegate address must either be this contract or have the delegate role AND support isValidSignature + * This allows someone who wants custom validation logic (perhaps they check another contract's state) + * to code their own delegate. The users then submit signatures to the Bouncer, + * which then pings back to the delegate to check if it's cool. + * This is also useful for contracts-as-identities: your identity contract implements + * isValidTicket and can then recover the signer and check it against the whitelisted ACTION keys. + * @return bool is the ticket valid */ - function isValidDataHash(bytes32 _hash, bytes _sig) + function isDelegatedSignatureValidForHash(address _delegate, bytes32 _hash, bytes _sig) internal view returns (bool) { - // if the sender is a delegate AND supports isValidSignature, we delegate validation to them - // this allows someone who wants custom validation logic (perhaps they check another contract's state) - // to code their own delegate. The users then submit actions to the delegate contract, which proxies - // them to this contract, which then pings-back to check if it's cool. - // This is also useful for contracts-as-identities: your identity contract implements isValidSignature - // and can then recover the signer and check it against the whitelisted ACTION keys. - if (hasRole(msg.sender, ROLE_DELEGATE) && - msg.sender.supportsInterface(InterfaceId_BouncerDelegate)) { - return IBouncerDelegate(msg.sender).isValidSignature(_hash, _sig); + bool isDelegate = hasRole(_delegate, ROLE_DELEGATE); + bool hasInterface = msg.sender.supportsInterface(InterfaceId_BouncerDelegate); + + if (isDelegate && hasInterface) { + return IBouncerDelegate(_delegate).isValidSignature(_hash, _sig); } + } - // otherwise it's an ECDSA Ethereum Signed Message hash, - // so recover and check the signer's delegate status + /** + * @dev implement `isValidSignature` of IBouncerDelegate for EOA accounts that have the delegate role + */ + function isValidSignature( + bytes32 _hash, + bytes _sig + ) + public + view + returns (bool) + { address signer = _hash .toEthSignedMessageHash() .recover(_sig); return hasRole(signer, ROLE_DELEGATE); } + + /** + * @dev Returns the first METHOD_ID_SIZE bytes of msg.data, which is the method signature + */ + function getMethodId() + internal + pure + returns (bytes) + { + bytes memory data = new bytes(METHOD_ID_SIZE); + for (uint i = 0; i < data.length; i++) { + data[i] = msg.data[i]; + } + + return data; + } + + /** + * @dev returns msg.data, sans the last SIGNATURE_SIZE bytes + */ + function getMessageData() + internal + pure + returns (bytes) + { + require(msg.data.length > SIGNATURE_SIZE); + + bytes memory data = new bytes(msg.data.length - SIGNATURE_SIZE); + for (uint i = 0; i < data.length; i++) { + data[i] = msg.data[i]; + } + + return data; + } } diff --git a/contracts/access/BouncerDelegate.sol b/contracts/access/BouncerDelegate.sol index 73aa7b1ab4a..4938e678966 100644 --- a/contracts/access/BouncerDelegate.sol +++ b/contracts/access/BouncerDelegate.sol @@ -26,7 +26,7 @@ contract BouncerDelegate is IBouncerDelegate, SupportsInterfaceWithLookup { bytes32 _hash, bytes _sig ) - external + public view returns (bool); } diff --git a/contracts/access/IBouncerDelegate.sol b/contracts/access/IBouncerDelegate.sol index fe5810a69b1..4794d76868b 100644 --- a/contracts/access/IBouncerDelegate.sol +++ b/contracts/access/IBouncerDelegate.sol @@ -25,7 +25,7 @@ contract IBouncerDelegate is ERC165 { bytes32 _hash, bytes _sig ) - external + public view returns (bool); } diff --git a/contracts/mocks/BouncerDelegateImpl.sol b/contracts/mocks/BouncerDelegateImpl.sol index 6631e489d4e..73f8cf4d3ee 100644 --- a/contracts/mocks/BouncerDelegateImpl.sol +++ b/contracts/mocks/BouncerDelegateImpl.sol @@ -36,7 +36,7 @@ contract BouncerDelegateImpl is BouncerDelegate { function forward() public { - bouncer.onlyWithValidSignature(""); + bouncer.onlyWithValidTicket(""); } } diff --git a/contracts/mocks/BouncerMock.sol b/contracts/mocks/BouncerMock.sol index 89a9f46a2e3..ce18481bd12 100644 --- a/contracts/mocks/BouncerMock.sol +++ b/contracts/mocks/BouncerMock.sol @@ -4,39 +4,39 @@ import "../access/Bouncer.sol"; contract BouncerMock is Bouncer { - function checkValidSignature(address _address, bytes _sig) + function checkValidTicket(address _address, bytes _sig) public view returns (bool) { - return isValidSignature(_address, _sig); + return isValidTicket(address(this), _address, _sig); } - function onlyWithValidSignature(bytes _sig) - onlyValidSignature(_sig) + function onlyWithValidTicket(bytes _sig) + onlyValidTicket(address(this), _sig) public view { } - function checkValidSignatureAndMethod(address _address, bytes _sig) + function checkValidTicketAndMethod(address _address, bytes _sig) public view returns (bool) { - return isValidSignatureAndMethod(_address, _sig); + return isValidTicketAndMethod(address(this), _address, _sig); } - function onlyWithValidSignatureAndMethod(bytes _sig) - onlyValidSignatureAndMethod(_sig) + function onlyWithValidTicketAndMethod(bytes _sig) + onlyValidTicketForMethod(address(this), _sig) public view { } - function checkValidSignatureAndData( + function checkValidTicketAndData( address _address, bytes, uint, @@ -46,11 +46,11 @@ contract BouncerMock is Bouncer { view returns (bool) { - return isValidSignatureAndData(_address, _sig); + return isValidTicketAndData(address(this), _address, _sig); } - function onlyWithValidSignatureAndData(uint, bytes _sig) - onlyValidSignatureAndData(_sig) + function onlyWithValidTicketAndData(uint, bytes _sig) + onlyValidTicketForData(address(this), _sig) public view { diff --git a/test/access/Bouncer.test.js b/test/access/Bouncer.test.js index fae7f542e19..d25c3f7a3d5 100644 --- a/test/access/Bouncer.test.js +++ b/test/access/Bouncer.test.js @@ -19,28 +19,48 @@ contract('Bouncer', ([_, owner, anyone, bouncerAddress, authorizedUser]) => { this.roleBouncer = await this.bouncer.ROLE_BOUNCER(); }); - context('management', () => { - it('allows the owner to add a bouncer', async function () { - await this.bouncer.addBouncer(bouncerAddress, { from: owner }); - (await this.bouncer.hasRole(bouncerAddress, this.roleBouncer)).should.eq(true); + it('should not allow anyone to add a delegate', async function () { + await assertRevert( + this.bouncer.addDelegate(delegate, { from: anyone }) + ); + }); + + context('modifiers', () => { + it('should allow valid signature for sender', async function () { + await this.bouncer.onlyWithValidTicket( + this.signFor(authorizedUser), + { from: authorizedUser } + ); }); it('does not allow adding an invalid address', async function () { await assertRevert( - this.bouncer.addBouncer('0x0', { from: owner }) + this.bouncer.onlyWithValidTicket( + INVALID_SIGNATURE, + { from: authorizedUser } + ) ); }); - - it('allows the owner to remove a bouncer', async function () { - await this.bouncer.addBouncer(bouncerAddress, { from: owner }); - - await this.bouncer.removeBouncer(bouncerAddress, { from: owner }); - (await this.bouncer.hasRole(bouncerAddress, this.roleBouncer)).should.eq(false); + it('should allow valid signature with a valid method for sender', async function () { + await this.bouncer.onlyWithValidTicketAndMethod( + this.signFor(authorizedUser, 'onlyWithValidTicketAndMethod'), + { from: authorizedUser } + ); }); it('does not allow anyone to add a bouncer', async function () { await assertRevert( - this.bouncer.addBouncer(bouncerAddress, { from: anyone }) + this.bouncer.onlyWithValidTicketAndMethod( + INVALID_SIGNATURE, + { from: authorizedUser } + ) + ); + }); + it('should allow valid signature with a valid data for sender', async function () { + await this.bouncer.onlyWithValidTicketAndData( + UINT_VALUE, + this.signFor(authorizedUser, 'onlyWithValidTicketAndData', [UINT_VALUE]), + { from: authorizedUser } ); }); @@ -48,16 +68,93 @@ contract('Bouncer', ([_, owner, anyone, bouncerAddress, authorizedUser]) => { await this.bouncer.addBouncer(bouncerAddress, { from: owner }); await assertRevert( - this.bouncer.removeBouncer(bouncerAddress, { from: anyone }) + this.bouncer.onlyWithValidTicketAndData( + UINT_VALUE, + INVALID_SIGNATURE, + { from: authorizedUser } + ) ); }); }); - context('with bouncer address', () => { - beforeEach(async function () { - await this.bouncer.addBouncer(bouncerAddress, { from: owner }); - this.signFor = getBouncerSigner(this.bouncer, bouncerAddress); + context('signatures', () => { + it('should accept valid message for valid user', async function () { + const isValid = await this.bouncer.checkValidTicket( + authorizedUser, + this.signFor(authorizedUser) + ); + isValid.should.eq(true); + }); + it('should not accept invalid message for valid user', async function () { + const isValid = await this.bouncer.checkValidTicket( + authorizedUser, + this.signFor(anyone) + ); + isValid.should.eq(false); + }); + it('should not accept invalid message for invalid user', async function () { + const isValid = await this.bouncer.checkValidTicket( + anyone, + 'abcd' + ); + isValid.should.eq(false); }); + it('should not accept valid message for invalid user', async function () { + const isValid = await this.bouncer.checkValidTicket( + anyone, + this.signFor(authorizedUser) + ); + isValid.should.eq(false); + }); + it('should accept valid message with valid method for valid user', async function () { + const isValid = await this.bouncer.checkValidTicketAndMethod( + authorizedUser, + this.signFor(authorizedUser, 'checkValidTicketAndMethod') + ); + isValid.should.eq(true); + }); + it('should not accept valid message with an invalid method for valid user', async function () { + const isValid = await this.bouncer.checkValidTicketAndMethod( + authorizedUser, + this.signFor(authorizedUser, 'theWrongMethod') + ); + isValid.should.eq(false); + }); + it('should not accept valid message with a valid method for an invalid user', async function () { + const isValid = await this.bouncer.checkValidTicketAndMethod( + anyone, + this.signFor(authorizedUser, 'checkValidTicketAndMethod') + ); + isValid.should.eq(false); + }); + it('should accept valid method with valid params for valid user', async function () { + const isValid = await this.bouncer.checkValidTicketAndData( + authorizedUser, + BYTES_VALUE, + UINT_VALUE, + this.signFor(authorizedUser, 'checkValidTicketAndData', [authorizedUser, BYTES_VALUE, UINT_VALUE]) + ); + isValid.should.eq(true); + }); + it('should not accept valid method with invalid params for valid user', async function () { + const isValid = await this.bouncer.checkValidTicketAndData( + authorizedUser, + BYTES_VALUE, + 500, + this.signFor(authorizedUser, 'checkValidTicketAndData', [authorizedUser, BYTES_VALUE, UINT_VALUE]) + ); + isValid.should.eq(false); + }); + it('should not accept valid method with valid params for invalid user', async function () { + const isValid = await this.bouncer.checkValidTicketAndData( + anyone, + BYTES_VALUE, + UINT_VALUE, + this.signFor(authorizedUser, 'checkValidTicketAndData', [authorizedUser, BYTES_VALUE, UINT_VALUE]) + ); + isValid.should.eq(false); + }); + }); describe('modifiers', () => { context('plain signature', () => { From 5257a81dbc57d0d5642d001eb47c9a2b0cf1d884 Mon Sep 17 00:00:00 2001 From: Matt Condon Date: Wed, 18 Jul 2018 15:16:17 -0700 Subject: [PATCH 06/10] fix: split logic into BouncerUtils and abstract awway sender, delegate --- contracts/ECRecovery.sol | 24 +- contracts/access/Bouncer.sol | 80 ++--- contracts/access/BouncerDelegate.sol | 13 - contracts/access/BouncerUtils.sol | 151 ++++++++ contracts/mocks/BouncerDelegateImpl.sol | 5 +- contracts/mocks/BouncerMock.sol | 12 +- test/access/Bouncer.test.js | 441 ++++++++---------------- test/helpers/sign.js | 9 +- 8 files changed, 342 insertions(+), 393 deletions(-) create mode 100644 contracts/access/BouncerUtils.sol diff --git a/contracts/ECRecovery.sol b/contracts/ECRecovery.sol index aacc7df0cfb..a03a85acc88 100644 --- a/contracts/ECRecovery.sol +++ b/contracts/ECRecovery.sol @@ -12,10 +12,10 @@ library ECRecovery { /** * @dev Recover signer address from a message by using their signature - * @param hash bytes32 message, the hash is the signed message. What is recovered is the signer address. - * @param sig bytes signature, the signature is generated using web3.eth.sign() + * @param _hash bytes32 message, the hash is the signed message. What is recovered is the signer address. + * @param _sig bytes signature, the signature is generated using web3.eth.sign() */ - function recover(bytes32 hash, bytes sig) + function recover(bytes32 _hash, bytes _sig) internal pure returns (address) @@ -25,7 +25,7 @@ library ECRecovery { uint8 v; // Check the signature length - if (sig.length != 65) { + if (_sig.length != 65) { return (address(0)); } @@ -34,9 +34,9 @@ library ECRecovery { // currently is to use assembly. // solium-disable-next-line security/no-inline-assembly assembly { - r := mload(add(sig, 32)) - s := mload(add(sig, 64)) - v := byte(0, mload(add(sig, 96))) + r := mload(add(_sig, 32)) + s := mload(add(_sig, 64)) + v := byte(0, mload(add(_sig, 96))) } // Version of signature should be 27 or 28, but 0 and 1 are also possible versions @@ -49,16 +49,16 @@ library ECRecovery { return (address(0)); } else { // solium-disable-next-line arg-overflow - return ecrecover(hash, v, r, s); + return ecrecover(_hash, v, r, s); } } /** * toEthSignedMessageHash - * @dev prefix a bytes32 value with "\x19Ethereum Signed Message:" - * and hash the result + * @dev prefix a bytes32 value with "\x19Ethereum Signed Message:\n32" and hash the result + * @param _hash the original hash */ - function toEthSignedMessageHash(bytes32 hash) + function toEthSignedMessageHash(bytes32 _hash) internal pure returns (bytes32) @@ -66,7 +66,7 @@ library ECRecovery { // 32 is the length in bytes of hash, // enforced by the type signature above return keccak256( - abi.encodePacked("\x19Ethereum Signed Message:\n32", hash) + abi.encodePacked("\x19Ethereum Signed Message:\n32", _hash) ); } } diff --git a/contracts/access/Bouncer.sol b/contracts/access/Bouncer.sol index 735d5b81129..b1a71d2e6a9 100644 --- a/contracts/access/Bouncer.sol +++ b/contracts/access/Bouncer.sol @@ -2,6 +2,7 @@ pragma solidity ^0.4.24; import "../introspection/ERC165Checker.sol"; import "./IBouncerDelegate.sol"; +import "./BouncerUtils.sol"; import "./BouncerDelegate.sol"; import "../ownership/rbac/RBACOwnable.sol"; import "../ECRecovery.sol"; @@ -9,7 +10,7 @@ import "../ECRecovery.sol"; /** * @title Bouncer - * @author PhABC, Shrugs and aflesher + * @author PhABC, Shrugs, and aflesher * @dev Bouncer allows users to submit a `ticket` from a `delegate` as a permission to do an action. * By default a `ticket` is a cryptographic signature * generated via Ethereum ECDSA signing (web3.eth.personal_sign). @@ -40,21 +41,17 @@ import "../ECRecovery.sol"; contract Bouncer is RBACOwnable, BouncerDelegate { using ECRecovery for bytes32; using ERC165Checker for address; + using BouncerUtils for bytes32; string public constant ROLE_DELEGATE = "delegate"; - // method ids are 4 bytes long - uint constant METHOD_ID_SIZE = 4; - // signature size is 65 bytes (tightly packed v + r + s), but gets padded to 96 bytes - uint constant SIGNATURE_SIZE = 96; - /** * @dev requires that a valid signature of a bouncer was provided * @notice does not validate method arguments */ modifier onlyValidTicket(address _delegate, bytes _sig) { - require(isValidTicket(_delegate, msg.sender, _sig)); + require(isValidTicket(_delegate, _sig)); _; } @@ -64,7 +61,7 @@ contract Bouncer is RBACOwnable, BouncerDelegate { */ modifier onlyValidTicketForMethod(address _delegate, bytes _sig) { - require(isValidTicketAndMethod(_delegate, msg.sender, _sig)); + require(isValidTicketAndMethod(_delegate, _sig)); _; } @@ -74,7 +71,7 @@ contract Bouncer is RBACOwnable, BouncerDelegate { */ modifier onlyValidTicketForData(address _delegate, bytes _sig) { - require(isValidTicketAndData(_delegate, msg.sender, _sig)); + require(isValidTicketAndData(_delegate, _sig)); _; } @@ -108,63 +105,63 @@ contract Bouncer is RBACOwnable, BouncerDelegate { } /** - * @dev is the signature of `this + sender` from a bouncer? + * @dev is the signature of `delegate + sender` from a bouncer? * @return bool */ - function isValidTicket(address _delegate, address _sender, bytes _sig) + function isValidTicket(address _delegate, bytes _sig) internal view returns (bool) { return isDelegatedSignatureValidForHash( _delegate, - keccak256(abi.encodePacked(_delegate, _sender)), + keccak256(abi.encodePacked(_delegate)), _sig ); } /** - * @dev is the signature of `this + sender + methodId` from a bouncer? + * @dev is the signature of `delegate + sender + methodId` from a bouncer? * @return bool */ - function isValidTicketAndMethod(address _delegate, address _sender, bytes _sig) + function isValidTicketAndMethod(address _delegate, bytes _sig) internal view returns (bool) { return isDelegatedSignatureValidForHash( _delegate, - keccak256(abi.encodePacked(_delegate, _sender, getMethodId())), + keccak256(abi.encodePacked(_delegate, BouncerUtils.getMethodId())), _sig ); } /** - * @dev is the signature of `this + sender + methodId + params(s)` from a bouncer? + * @dev is the signature of `delegate + sender + msg.data` from a bouncer? * @notice the _sig parameter of the method being validated must be the "last" parameter * @return bool */ - function isValidTicketAndData(address _delegate, address _sender, bytes _sig) + function isValidTicketAndData(address _delegate, bytes _sig) internal view returns (bool) { return isDelegatedSignatureValidForHash( _delegate, - keccak256(abi.encodePacked(_delegate, _sender, getMessageData())), + keccak256(abi.encodePacked(_delegate, BouncerUtils.getMessageData())), _sig ); } /** * @dev tests to see if the signature is valid according to the delegate - * The delegate address must either be this contract or have the delegate role AND support isValidSignature + * The delegate address must have the delegate role AND support isValidSignature * This allows someone who wants custom validation logic (perhaps they check another contract's state) * to code their own delegate. The users then submit signatures to the Bouncer, * which then pings back to the delegate to check if it's cool. * This is also useful for contracts-as-identities: your identity contract implements * isValidTicket and can then recover the signer and check it against the whitelisted ACTION keys. - * @return bool is the ticket valid + * @return bool validity of the signature */ function isDelegatedSignatureValidForHash(address _delegate, bytes32 _hash, bytes _sig) internal @@ -172,11 +169,14 @@ contract Bouncer is RBACOwnable, BouncerDelegate { returns (bool) { bool isDelegate = hasRole(_delegate, ROLE_DELEGATE); - bool hasInterface = msg.sender.supportsInterface(InterfaceId_BouncerDelegate); + bool hasInterface = _delegate.supportsInterface(InterfaceId_BouncerDelegate); if (isDelegate && hasInterface) { return IBouncerDelegate(_delegate).isValidSignature(_hash, _sig); } + + // delegate is invalid, so signature is invalid as well + return false; } /** @@ -190,43 +190,7 @@ contract Bouncer is RBACOwnable, BouncerDelegate { view returns (bool) { - address signer = _hash - .toEthSignedMessageHash() - .recover(_sig); + address signer = _hash.signerWithSignature(_sig); return hasRole(signer, ROLE_DELEGATE); } - - /** - * @dev Returns the first METHOD_ID_SIZE bytes of msg.data, which is the method signature - */ - function getMethodId() - internal - pure - returns (bytes) - { - bytes memory data = new bytes(METHOD_ID_SIZE); - for (uint i = 0; i < data.length; i++) { - data[i] = msg.data[i]; - } - - return data; - } - - /** - * @dev returns msg.data, sans the last SIGNATURE_SIZE bytes - */ - function getMessageData() - internal - pure - returns (bytes) - { - require(msg.data.length > SIGNATURE_SIZE); - - bytes memory data = new bytes(msg.data.length - SIGNATURE_SIZE); - for (uint i = 0; i < data.length; i++) { - data[i] = msg.data[i]; - } - - return data; - } } diff --git a/contracts/access/BouncerDelegate.sol b/contracts/access/BouncerDelegate.sol index 4938e678966..6f6513cdf4a 100644 --- a/contracts/access/BouncerDelegate.sol +++ b/contracts/access/BouncerDelegate.sol @@ -11,22 +11,9 @@ import "../introspection/SupportsInterfaceWithLookup.sol"; * As a delegate, you must proxy calls from accounts you consider valid. */ contract BouncerDelegate is IBouncerDelegate, SupportsInterfaceWithLookup { - constructor () public { _registerInterface(InterfaceId_BouncerDelegate); } - - /** - * @dev See IBouncerDelegate#isValidSignature for more info - * Override this function to supply implementation. - */ - function isValidSignature( - bytes32 _hash, - bytes _sig - ) - public - view - returns (bool); } diff --git a/contracts/access/BouncerUtils.sol b/contracts/access/BouncerUtils.sol new file mode 100644 index 00000000000..3763a4bd033 --- /dev/null +++ b/contracts/access/BouncerUtils.sol @@ -0,0 +1,151 @@ +pragma solidity 0.4.24; + +import "../ECRecovery.sol"; + + +/** + * @title BouncerUtils + * @author Matt Condon (@Shrugs) + * @dev Provides helpful logic for verifying method parameters. + * Use this contract if you want to implement your own Bouncer access-control logic that doesn't + * require delegated signature validity (i.e., if you need a multisig wallet or identity contract to + * "sign" for things, you need delegated signature validity.) + * + * ```solidity + * using BouncerUtils for bytes32; + * + * + * // require that the `owner` EOA account signs the submitted msg.data + * // include your custom arguments here if necessary + * modifier onlyValidMint(address _to, uint256 _amount, bytes _sig) { + * require( + * keccak256(abi.encodePacked( + * address(this), + * _to, + * _amount + * )).signerWithSignature(_sig) == owner, + * "invalid signature" + * ); + * _; + * } + * + * function mint(address _to, uint256 _amount, bytes _sig) + * onlyValidMint(_to, _amount, _sig) + * public + * { + * // trust arguments + * MyToken.mint(_to, _amount); + * } + * + * // or, simpler, if you want to verify all arguments + * + * modifier onlyValidTransaction() { + * require( + * BouncerUtils.signerOfMessageData(address(this)) == owner, + * "invalid signature" + * ); + * } + * + * function mint(address _to, uint256 _amount, bytes _sig) + * onlyValidTransaction() + * public + * { + * // trust arguments + * MyToken.mint(_to, _amount); + * } + * ``` + */ +library BouncerUtils { + using ECRecovery for bytes32; + + // method ids are 4 bytes long + uint constant METHOD_ID_SIZE = 4; + // signature size is 65 bytes (tightly packed v + r + s), but gets padded to 96 bytes + uint constant SIGNATURE_SIZE = 96; + + function signerWithSignature(bytes32 _hash, bytes _sig) + internal + pure + returns (address) + { + return _hash + .toEthSignedMessageHash() + .recover(_sig); + } + + /** + * @dev recover the signer of the msg.data, assuming all arguments are checked and the last + * argument is a compact vrs signature, padded to 96 bytes. + * @return address + */ + function signerOfMessageData(address _delegate) + internal + view + returns (address) + { + bytes32 hashOfMessageData = keccak256( + abi.encodePacked( + _delegate, + getMessageData() + ) + ); + + return signerWithSignature( + hashOfMessageData, + getSignatureArgument() + ); + } + + + /** + * @dev Returns the first METHOD_ID_SIZE bytes of msg.data, which is the method signature + */ + function getMethodId() + internal + pure + returns (bytes) + { + bytes memory data = new bytes(METHOD_ID_SIZE); + for (uint i = 0; i < data.length; i++) { + data[i] = msg.data[i]; + } + + return data; + } + + /** + * @dev returns msg.data, sans the last SIGNATURE_SIZE bytes + */ + function getMessageData() + internal + pure + returns (bytes) + { + require(msg.data.length > SIGNATURE_SIZE); + + bytes memory data = new bytes(msg.data.length - SIGNATURE_SIZE); + for (uint i = 0; i < data.length; i++) { + data[i] = msg.data[i]; + } + + return data; + } + + /** + * @dev returns the last 96 bytes of msg.data, the compacted vrs signature + */ + function getSignatureArgument() + internal + pure + returns (bytes) + { + require(msg.data.length > SIGNATURE_SIZE); + + bytes memory data = new bytes(SIGNATURE_SIZE); + for (uint i = msg.data.length - 1; i > data.length; i++) { + data[i] = msg.data[i]; + } + + return data; + } +} diff --git a/contracts/mocks/BouncerDelegateImpl.sol b/contracts/mocks/BouncerDelegateImpl.sol index 73f8cf4d3ee..709bd480899 100644 --- a/contracts/mocks/BouncerDelegateImpl.sol +++ b/contracts/mocks/BouncerDelegateImpl.sol @@ -26,7 +26,7 @@ contract BouncerDelegateImpl is BouncerDelegate { bytes32 _hash, bytes _sig ) - external + public view returns (bool) { @@ -34,9 +34,10 @@ contract BouncerDelegateImpl is BouncerDelegate { } function forward() + view public { - bouncer.onlyWithValidTicket(""); + require(bouncer.checkValidTicket(address(this), "")); } } diff --git a/contracts/mocks/BouncerMock.sol b/contracts/mocks/BouncerMock.sol index ce18481bd12..6e5515e579e 100644 --- a/contracts/mocks/BouncerMock.sol +++ b/contracts/mocks/BouncerMock.sol @@ -4,12 +4,12 @@ import "../access/Bouncer.sol"; contract BouncerMock is Bouncer { - function checkValidTicket(address _address, bytes _sig) + function checkValidTicket(address _delegate, bytes _sig) public view returns (bool) { - return isValidTicket(address(this), _address, _sig); + return isValidTicket(_delegate, _sig); } function onlyWithValidTicket(bytes _sig) @@ -20,12 +20,12 @@ contract BouncerMock is Bouncer { } - function checkValidTicketAndMethod(address _address, bytes _sig) + function checkValidTicketAndMethod(address _delegate, bytes _sig) public view returns (bool) { - return isValidTicketAndMethod(address(this), _address, _sig); + return isValidTicketAndMethod(_delegate, _sig); } function onlyWithValidTicketAndMethod(bytes _sig) @@ -37,7 +37,7 @@ contract BouncerMock is Bouncer { } function checkValidTicketAndData( - address _address, + address _delegate, bytes, uint, bytes _sig @@ -46,7 +46,7 @@ contract BouncerMock is Bouncer { view returns (bool) { - return isValidTicketAndData(address(this), _address, _sig); + return isValidTicketAndData(_delegate, _sig); } function onlyWithValidTicketAndData(uint, bytes _sig) diff --git a/test/access/Bouncer.test.js b/test/access/Bouncer.test.js index d25c3f7a3d5..8b458fdb6f2 100644 --- a/test/access/Bouncer.test.js +++ b/test/access/Bouncer.test.js @@ -1,5 +1,6 @@ + const { assertRevert } = require('../helpers/assertRevert'); -const { getBouncerSigner } = require('../helpers/sign'); +const { getBouncerTicketGenerator } = require('../helpers/sign'); const makeInterfaceId = require('../helpers/makeInterfaceId'); const Bouncer = artifacts.require('BouncerMock'); @@ -13,10 +14,23 @@ const UINT_VALUE = 23; const BYTES_VALUE = web3.toHex('test'); const INVALID_SIGNATURE = '0xabcd'; -contract('Bouncer', ([_, owner, anyone, bouncerAddress, authorizedUser]) => { +contract('Bouncer', ([_, owner, anyone, delegate, newDelegate]) => { beforeEach(async function () { - this.bouncer = await SignatureBouncer.new({ from: owner }); - this.roleBouncer = await this.bouncer.ROLE_BOUNCER(); + this.bouncer = await Bouncer.new({ from: owner }); + this.roleDelegate = await this.bouncer.ROLE_DELEGATE(); + this.roleOwner = await this.bouncer.ROLE_OWNER(); + this.generateTicket = getBouncerTicketGenerator(this.bouncer, delegate); + }); + + it('should have a default owner', async function () { + const hasRole = await this.bouncer.hasRole(owner, this.roleOwner); + hasRole.should.eq(true); + }); + + it('should allow owner to add a delegate', async function () { + await this.bouncer.addDelegate(delegate, { from: owner }); + const hasRole = await this.bouncer.hasRole(delegate, this.roleDelegate); + hasRole.should.eq(true); }); it('should not allow anyone to add a delegate', async function () { @@ -25,316 +39,151 @@ contract('Bouncer', ([_, owner, anyone, bouncerAddress, authorizedUser]) => { ); }); - context('modifiers', () => { - it('should allow valid signature for sender', async function () { - await this.bouncer.onlyWithValidTicket( - this.signFor(authorizedUser), - { from: authorizedUser } - ); - }); - - it('does not allow adding an invalid address', async function () { - await assertRevert( - this.bouncer.onlyWithValidTicket( - INVALID_SIGNATURE, - { from: authorizedUser } - ) - ); - }); - it('should allow valid signature with a valid method for sender', async function () { - await this.bouncer.onlyWithValidTicketAndMethod( - this.signFor(authorizedUser, 'onlyWithValidTicketAndMethod'), - { from: authorizedUser } - ); - }); - - it('does not allow anyone to add a bouncer', async function () { - await assertRevert( - this.bouncer.onlyWithValidTicketAndMethod( - INVALID_SIGNATURE, - { from: authorizedUser } - ) - ); - }); - it('should allow valid signature with a valid data for sender', async function () { - await this.bouncer.onlyWithValidTicketAndData( - UINT_VALUE, - this.signFor(authorizedUser, 'onlyWithValidTicketAndData', [UINT_VALUE]), - { from: authorizedUser } - ); + context('EOA delegate', function () { + beforeEach(async function () { + await this.bouncer.addDelegate(delegate, { from: owner }); }); - it('does not allow anyone to remove a bouncer', async function () { - await this.bouncer.addBouncer(bouncerAddress, { from: owner }); - - await assertRevert( - this.bouncer.onlyWithValidTicketAndData( + context('modifiers', () => { + it('should allow valid signature', async function () { + await this.bouncer.onlyWithValidTicket( + this.generateTicket(), + { from: anyone } + ); + }); + it('should not allow invalid signature', async function () { + await assertRevert( + this.bouncer.onlyWithValidTicket( + INVALID_SIGNATURE, + { from: anyone } + ) + ); + }); + it('should allow valid signature with a valid method', async function () { + await this.bouncer.onlyWithValidTicketAndMethod( + this.generateTicket('onlyWithValidTicketAndMethod'), + { from: anyone } + ); + }); + it('should not allow invalid signature with method', async function () { + await assertRevert( + this.bouncer.onlyWithValidTicketAndMethod( + INVALID_SIGNATURE, + { from: anyone } + ) + ); + }); + it('should allow valid signature with a valid data', async function () { + await this.bouncer.onlyWithValidTicketAndData( UINT_VALUE, - INVALID_SIGNATURE, - { from: authorizedUser } - ) - ); - }); - }); - - context('signatures', () => { - it('should accept valid message for valid user', async function () { - const isValid = await this.bouncer.checkValidTicket( - authorizedUser, - this.signFor(authorizedUser) - ); - isValid.should.eq(true); - }); - it('should not accept invalid message for valid user', async function () { - const isValid = await this.bouncer.checkValidTicket( - authorizedUser, - this.signFor(anyone) - ); - isValid.should.eq(false); - }); - it('should not accept invalid message for invalid user', async function () { - const isValid = await this.bouncer.checkValidTicket( - anyone, - 'abcd' - ); - isValid.should.eq(false); - }); - it('should not accept valid message for invalid user', async function () { - const isValid = await this.bouncer.checkValidTicket( - anyone, - this.signFor(authorizedUser) - ); - isValid.should.eq(false); - }); - it('should accept valid message with valid method for valid user', async function () { - const isValid = await this.bouncer.checkValidTicketAndMethod( - authorizedUser, - this.signFor(authorizedUser, 'checkValidTicketAndMethod') - ); - isValid.should.eq(true); - }); - it('should not accept valid message with an invalid method for valid user', async function () { - const isValid = await this.bouncer.checkValidTicketAndMethod( - authorizedUser, - this.signFor(authorizedUser, 'theWrongMethod') - ); - isValid.should.eq(false); - }); - it('should not accept valid message with a valid method for an invalid user', async function () { - const isValid = await this.bouncer.checkValidTicketAndMethod( - anyone, - this.signFor(authorizedUser, 'checkValidTicketAndMethod') - ); - isValid.should.eq(false); - }); - it('should accept valid method with valid params for valid user', async function () { - const isValid = await this.bouncer.checkValidTicketAndData( - authorizedUser, - BYTES_VALUE, - UINT_VALUE, - this.signFor(authorizedUser, 'checkValidTicketAndData', [authorizedUser, BYTES_VALUE, UINT_VALUE]) - ); - isValid.should.eq(true); - }); - it('should not accept valid method with invalid params for valid user', async function () { - const isValid = await this.bouncer.checkValidTicketAndData( - authorizedUser, - BYTES_VALUE, - 500, - this.signFor(authorizedUser, 'checkValidTicketAndData', [authorizedUser, BYTES_VALUE, UINT_VALUE]) - ); - isValid.should.eq(false); - }); - it('should not accept valid method with valid params for invalid user', async function () { - const isValid = await this.bouncer.checkValidTicketAndData( - anyone, - BYTES_VALUE, - UINT_VALUE, - this.signFor(authorizedUser, 'checkValidTicketAndData', [authorizedUser, BYTES_VALUE, UINT_VALUE]) - ); - isValid.should.eq(false); + this.generateTicket('onlyWithValidTicketAndData', [UINT_VALUE]), + { from: anyone } + ); + }); + it('should not allow invalid signature with data', async function () { + await assertRevert( + this.bouncer.onlyWithValidTicketAndData( + UINT_VALUE, + INVALID_SIGNATURE, + { from: anyone } + ) + ); + }); }); - }); - - describe('modifiers', () => { - context('plain signature', () => { - it('allows valid signature for sender', async function () { - await this.bouncer.onlyWithValidSignature(this.signFor(authorizedUser), { from: authorizedUser }); - }); - - it('does not allow invalid signature for sender', async function () { - await assertRevert( - this.bouncer.onlyWithValidSignature(INVALID_SIGNATURE, { from: authorizedUser }) - ); - }); - it('does not allow valid signature for other sender', async function () { - await assertRevert( - this.bouncer.onlyWithValidSignature(this.signFor(authorizedUser), { from: anyone }) - ); - }); - - it('does not allow valid signature for method for sender', async function () { - await assertRevert( - this.bouncer.onlyWithValidSignature(this.signFor(authorizedUser, 'onlyWithValidSignature'), - { from: authorizedUser }) - ); - }); + context('signatures', () => { + it('should accept valid ticket for valid delegate', async function () { + const isValid = await this.bouncer.checkValidTicket( + this.bouncer.address, + this.generateTicket() + ); + isValid.should.eq(true); }); - - context('method signature', () => { - it('allows valid signature with correct method for sender', async function () { - await this.bouncer.onlyWithValidSignatureAndMethod( - this.signFor(authorizedUser, 'onlyWithValidSignatureAndMethod'), { from: authorizedUser } - ); - }); - - it('does not allow invalid signature with correct method for sender', async function () { - await assertRevert( - this.bouncer.onlyWithValidSignatureAndMethod(INVALID_SIGNATURE, { from: authorizedUser }) - ); - }); - - it('does not allow valid signature with correct method for other sender', async function () { - await assertRevert( - this.bouncer.onlyWithValidSignatureAndMethod( - this.signFor(authorizedUser, 'onlyWithValidSignatureAndMethod'), { from: anyone } - ) - ); - }); - - it('does not allow valid method signature with incorrect method for sender', async function () { - await assertRevert( - this.bouncer.onlyWithValidSignatureAndMethod(this.signFor(authorizedUser, 'theWrongMethod'), - { from: authorizedUser }) - ); - }); - - it('does not allow valid non-method signature method for sender', async function () { - await assertRevert( - this.bouncer.onlyWithValidSignatureAndMethod(this.signFor(authorizedUser), { from: authorizedUser }) - ); - }); + it('should not accept invalid ticket for valid delegate', async function () { + const isValid = await this.bouncer.checkValidTicket( + this.bouncer.address, + 'abcd' + ); + isValid.should.eq(false); }); - - context('method and data signature', () => { - it('allows valid signature with correct method and data for sender', async function () { - await this.bouncer.onlyWithValidSignatureAndData(UINT_VALUE, - this.signFor(authorizedUser, 'onlyWithValidSignatureAndData', [UINT_VALUE]), { from: authorizedUser } - ); - }); - - it('does not allow invalid signature with correct method and data for sender', async function () { - await assertRevert( - this.bouncer.onlyWithValidSignatureAndData(UINT_VALUE, INVALID_SIGNATURE, { from: authorizedUser }) - ); - }); - - it('does not allow valid signature with correct method and incorrect data for sender', async function () { - await assertRevert( - this.bouncer.onlyWithValidSignatureAndData(UINT_VALUE + 10, - this.signFor(authorizedUser, 'onlyWithValidSignatureAndData', [UINT_VALUE]), - { from: authorizedUser } - ) - ); - }); - - it('does not allow valid signature with correct method and data for other sender', async function () { - await assertRevert( - this.bouncer.onlyWithValidSignatureAndData(UINT_VALUE, - this.signFor(authorizedUser, 'onlyWithValidSignatureAndData', [UINT_VALUE]), - { from: anyone } - ) - ); - }); - - it('does not allow valid non-method signature for sender', async function () { - await assertRevert( - this.bouncer.onlyWithValidSignatureAndData(UINT_VALUE, - this.signFor(authorizedUser), { from: authorizedUser } - ) - ); - }); + it('should accept valid ticket with valid method', async function () { + const isValid = await this.bouncer.checkValidTicketAndMethod( + this.bouncer.address, + this.generateTicket('checkValidTicketAndMethod') + ); + isValid.should.eq(true); + }); + it('should not accept valid message with an invalid method', async function () { + const isValid = await this.bouncer.checkValidTicketAndMethod( + this.bouncer.address, + this.generateTicket('theWrongMethod') + ); + isValid.should.eq(false); + }); + it('should not accept valid message with a valid method for an invalid user', async function () { + const isValid = await this.bouncer.checkValidTicketAndMethod( + anyone, + this.generateTicket('checkValidTicketAndMethod') + ); + isValid.should.eq(false); + }); + it('should accept valid method with valid params', async function () { + const isValid = await this.bouncer.checkValidTicketAndData( + this.bouncer.address, + BYTES_VALUE, + UINT_VALUE, + this.generateTicket('checkValidTicketAndData', [this.bouncer.address, BYTES_VALUE, UINT_VALUE]) + ); + isValid.should.eq(true); + }); + it('should not accept valid method with invalid params', async function () { + const isValid = await this.bouncer.checkValidTicketAndData( + this.bouncer.address, + BYTES_VALUE, + 500, + this.generateTicket('checkValidTicketAndData', [this.bouncer.address, BYTES_VALUE, UINT_VALUE]) + ); + isValid.should.eq(false); + }); + it('should not accept valid method with valid params for invalid delegate', async function () { + const isValid = await this.bouncer.checkValidTicketAndData( + anyone, + BYTES_VALUE, + UINT_VALUE, + this.generateTicket('checkValidTicketAndData', [anyone, BYTES_VALUE, UINT_VALUE]) + ); + isValid.should.eq(false); }); }); - context('signature validation', () => { - context('plain signature', () => { - it('validates valid signature for valid user', async function () { - (await this.bouncer.checkValidSignature(authorizedUser, this.signFor(authorizedUser))).should.eq(true); - }); - - it('does not validate invalid signature for valid user', async function () { - (await this.bouncer.checkValidSignature(authorizedUser, INVALID_SIGNATURE)).should.eq(false); - }); - - it('does not validate valid signature for anyone', async function () { - (await this.bouncer.checkValidSignature(anyone, this.signFor(authorizedUser))).should.eq(false); - }); - - it('does not validate valid signature for method for valid user', async function () { - (await this.bouncer.checkValidSignature(authorizedUser, this.signFor(authorizedUser, 'checkValidSignature')) - ).should.eq(false); - }); + context('management', () => { + it('should not allow anyone to add delegates', async function () { + await assertRevert( + this.bouncer.addDelegate(newDelegate, { from: anyone }) + ); }); - context('method signature', () => { - it('validates valid signature with correct method for valid user', async function () { - (await this.bouncer.checkValidSignatureAndMethod(authorizedUser, - this.signFor(authorizedUser, 'checkValidSignatureAndMethod')) - ).should.eq(true); - }); - - it('does not validate invalid signature with correct method for valid user', async function () { - (await this.bouncer.checkValidSignatureAndMethod(authorizedUser, INVALID_SIGNATURE)).should.eq(false); - }); - - it('does not validate valid signature with correct method for anyone', async function () { - (await this.bouncer.checkValidSignatureAndMethod(anyone, - this.signFor(authorizedUser, 'checkValidSignatureAndMethod')) - ).should.eq(false); - }); - - it('does not validate valid non-method signature with correct method for valid user', async function () { - (await this.bouncer.checkValidSignatureAndMethod(authorizedUser, this.signFor(authorizedUser)) - ).should.eq(false); - }); + it('should be able to add delegate', async function () { + await this.bouncer.addDelegate(newDelegate, { from: owner }) + .should.be.fulfilled; }); - context('method and data signature', () => { - it('validates valid signature with correct method and data for valid user', async function () { - (await this.bouncer.checkValidSignatureAndData(authorizedUser, BYTES_VALUE, UINT_VALUE, - this.signFor(authorizedUser, 'checkValidSignatureAndData', [authorizedUser, BYTES_VALUE, UINT_VALUE])) - ).should.eq(true); - }); - - it('does not validate invalid signature with correct method and data for valid user', async function () { - (await this.bouncer.checkValidSignatureAndData(authorizedUser, BYTES_VALUE, UINT_VALUE, INVALID_SIGNATURE) - ).should.eq(false); - }); - - it('does not validate valid signature with correct method and incorrect data for valid user', - async function () { - (await this.bouncer.checkValidSignatureAndData(authorizedUser, BYTES_VALUE, UINT_VALUE + 10, - this.signFor(authorizedUser, 'checkValidSignatureAndData', [authorizedUser, BYTES_VALUE, UINT_VALUE])) - ).should.eq(false); - } + it('should not allow adding invalid address', async function () { + await assertRevert( + this.bouncer.addDelegate('0x0', { from: owner }) ); + }); - it('does not validate valid signature with correct method and data for anyone', async function () { - (await this.bouncer.checkValidSignatureAndData(anyone, BYTES_VALUE, UINT_VALUE, - this.signFor(authorizedUser, 'checkValidSignatureAndData', [authorizedUser, BYTES_VALUE, UINT_VALUE])) - ).should.eq(false); - }); - - it('does not validate valid non-method-data signature with correct method and data for valid user', - async function () { - (await this.bouncer.checkValidSignatureAndData(authorizedUser, BYTES_VALUE, UINT_VALUE, - this.signFor(authorizedUser, 'checkValidSignatureAndData')) - ).should.eq(false); - } + it('should not allow anyone to remove delegate', async function () { + await assertRevert( + this.bouncer.removeDelegate(newDelegate, { from: anyone }) ); }); + + it('should be able to remove delegates', async function () { + await this.bouncer.removeDelegate(newDelegate, { from: owner }) + .should.be.fulfilled; + }); }); }); diff --git a/test/helpers/sign.js b/test/helpers/sign.js index a732d22d634..d2bea54dafb 100644 --- a/test/helpers/sign.js +++ b/test/helpers/sign.js @@ -3,7 +3,7 @@ const { soliditySha3 } = require('web3-utils'); const REAL_SIGNATURE_SIZE = 2 * 65; // 65 bytes in hexadecimal string legnth const PADDED_SIGNATURE_SIZE = 2 * 96; // 96 bytes in hexadecimal string length -const DUMMY_SIGNATURE = `0x${web3.padLeft('', REAL_SIGNATURE_SIZE)}`; // '0x' plus 130 '0's +const DUMMY_SIGNATURE = `0x${web3.padLeft('', REAL_SIGNATURE_SIZE)}`; // '0x' plus 130 '0's because hex /** * Hash and add same prefix to the hash that ganache uses. @@ -25,7 +25,6 @@ const signMessage = (signer, message = '') => { // @TODO - remove this when we migrate to web3-1.0.0 const transformToFullName = function (json) { if (json.name.indexOf('(') !== -1) { - console.log(json); return json.name; } @@ -40,14 +39,12 @@ const transformToFullName = function (json) { * fetcher because the method on the contract isn't actually the SolidityFunction object ಠ_ಠ * @param contract TruffleContract * @param signer address - * @param redeemer address * @param methodName string * @param methodArgs any[] */ -const getBouncerSigner = (contract, signer) => (redeemer, methodName, methodArgs = []) => { +const getBouncerTicketGenerator = (contract, signer) => (methodName, methodArgs = []) => { const parts = [ contract.address, - redeemer, ]; // if we have a method, add it to the parts that we're signing @@ -80,5 +77,5 @@ const getBouncerSigner = (contract, signer) => (redeemer, methodName, methodArgs module.exports = { hashMessage, signMessage, - getBouncerSigner, + getBouncerTicketGenerator, }; From b9326126829e8ce99a2d5a6bf16544744632bb70 Mon Sep 17 00:00:00 2001 From: Matt Condon Date: Wed, 18 Jul 2018 15:57:37 -0700 Subject: [PATCH 07/10] fix: update imports for other contracts --- test/access/Bouncer.test.js | 2 +- test/ownership/rbac/RBACOwnable.test.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/access/Bouncer.test.js b/test/access/Bouncer.test.js index 8b458fdb6f2..0e9db1cb426 100644 --- a/test/access/Bouncer.test.js +++ b/test/access/Bouncer.test.js @@ -1,7 +1,7 @@ const { assertRevert } = require('../helpers/assertRevert'); const { getBouncerTicketGenerator } = require('../helpers/sign'); -const makeInterfaceId = require('../helpers/makeInterfaceId'); +const { makeInterfaceId } = require('../helpers/makeInterfaceId'); const Bouncer = artifacts.require('BouncerMock'); const BouncerDelegateImpl = artifacts.require('BouncerDelegateImpl'); diff --git a/test/ownership/rbac/RBACOwnable.test.js b/test/ownership/rbac/RBACOwnable.test.js index df067090c77..442f6b100a0 100644 --- a/test/ownership/rbac/RBACOwnable.test.js +++ b/test/ownership/rbac/RBACOwnable.test.js @@ -1,4 +1,4 @@ -import assertRevert from '../../helpers/assertRevert'; +const { assertRevert } = require('../../helpers/assertRevert'); const RBACOwnable = artifacts.require('RBACOwnable'); From ca23fa7acdfa17e677efd57b3c5548b6755f6062 Mon Sep 17 00:00:00 2001 From: Matt Condon Date: Wed, 25 Jul 2018 16:24:06 +0800 Subject: [PATCH 08/10] feat: refactor into SignatureChecker --- contracts/access/Bouncer.sol | 82 +----------------- contracts/access/BouncerUtils.sol | 23 ++--- ...gateImpl.sol => SignatureDelegateImpl.sol} | 4 +- .../ISignatureDelegate.sol} | 8 +- contracts/signatures/SignatureChecker.sol | 85 +++++++++++++++++++ .../SignatureDelegate.sol} | 10 +-- test/access/Bouncer.test.js | 8 +- 7 files changed, 110 insertions(+), 110 deletions(-) rename contracts/mocks/{BouncerDelegateImpl.sol => SignatureDelegateImpl.sol} (87%) rename contracts/{access/IBouncerDelegate.sol => signatures/ISignatureDelegate.sol} (73%) create mode 100644 contracts/signatures/SignatureChecker.sol rename contracts/{access/BouncerDelegate.sol => signatures/SignatureDelegate.sol} (50%) diff --git a/contracts/access/Bouncer.sol b/contracts/access/Bouncer.sol index b1a71d2e6a9..c7ac5b0f676 100644 --- a/contracts/access/Bouncer.sol +++ b/contracts/access/Bouncer.sol @@ -1,11 +1,8 @@ pragma solidity ^0.4.24; -import "../introspection/ERC165Checker.sol"; -import "./IBouncerDelegate.sol"; -import "./BouncerUtils.sol"; -import "./BouncerDelegate.sol"; -import "../ownership/rbac/RBACOwnable.sol"; import "../ECRecovery.sol"; +import "./BouncerUtils.sol"; +import "../signatures/SignatureChecker.sol"; /** @@ -38,13 +35,10 @@ import "../ECRecovery.sol"; * signature in it so the last 96 bytes of msg.data (which represents the _sig data itself) * is ignored when constructing the data hash that is validated. */ -contract Bouncer is RBACOwnable, BouncerDelegate { +contract Bouncer is SignatureChecker { using ECRecovery for bytes32; - using ERC165Checker for address; using BouncerUtils for bytes32; - string public constant ROLE_DELEGATE = "delegate"; - /** * @dev requires that a valid signature of a bouncer was provided * @notice does not validate method arguments @@ -75,35 +69,6 @@ contract Bouncer is RBACOwnable, BouncerDelegate { _; } - constructor() - public - { - // this contract implements IBouncerDelegate for all of its EOA delegate accounts. - addRole(address(this), ROLE_DELEGATE); - } - - /** - * @dev allows the owner to add additional signer addresses - */ - function addDelegate(address _delegate) - onlyOwners - public - { - require(_delegate != address(0)); - addRole(_delegate, ROLE_DELEGATE); - } - - /** - * @dev allows the owner to remove signer addresses - */ - function removeDelegate(address _delegate) - onlyOwners - public - { - require(_delegate != address(0)); - removeRole(_delegate, ROLE_DELEGATE); - } - /** * @dev is the signature of `delegate + sender` from a bouncer? * @return bool @@ -152,45 +117,4 @@ contract Bouncer is RBACOwnable, BouncerDelegate { _sig ); } - - /** - * @dev tests to see if the signature is valid according to the delegate - * The delegate address must have the delegate role AND support isValidSignature - * This allows someone who wants custom validation logic (perhaps they check another contract's state) - * to code their own delegate. The users then submit signatures to the Bouncer, - * which then pings back to the delegate to check if it's cool. - * This is also useful for contracts-as-identities: your identity contract implements - * isValidTicket and can then recover the signer and check it against the whitelisted ACTION keys. - * @return bool validity of the signature - */ - function isDelegatedSignatureValidForHash(address _delegate, bytes32 _hash, bytes _sig) - internal - view - returns (bool) - { - bool isDelegate = hasRole(_delegate, ROLE_DELEGATE); - bool hasInterface = _delegate.supportsInterface(InterfaceId_BouncerDelegate); - - if (isDelegate && hasInterface) { - return IBouncerDelegate(_delegate).isValidSignature(_hash, _sig); - } - - // delegate is invalid, so signature is invalid as well - return false; - } - - /** - * @dev implement `isValidSignature` of IBouncerDelegate for EOA accounts that have the delegate role - */ - function isValidSignature( - bytes32 _hash, - bytes _sig - ) - public - view - returns (bool) - { - address signer = _hash.signerWithSignature(_sig); - return hasRole(signer, ROLE_DELEGATE); - } } diff --git a/contracts/access/BouncerUtils.sol b/contracts/access/BouncerUtils.sol index 3763a4bd033..fc30bc5de75 100644 --- a/contracts/access/BouncerUtils.sol +++ b/contracts/access/BouncerUtils.sol @@ -23,7 +23,7 @@ import "../ECRecovery.sol"; * address(this), * _to, * _amount - * )).signerWithSignature(_sig) == owner, + * )).toEthSignedMessageHash().recover(_sig) == owner, * "invalid signature" * ); * _; @@ -63,16 +63,6 @@ library BouncerUtils { // signature size is 65 bytes (tightly packed v + r + s), but gets padded to 96 bytes uint constant SIGNATURE_SIZE = 96; - function signerWithSignature(bytes32 _hash, bytes _sig) - internal - pure - returns (address) - { - return _hash - .toEthSignedMessageHash() - .recover(_sig); - } - /** * @dev recover the signer of the msg.data, assuming all arguments are checked and the last * argument is a compact vrs signature, padded to 96 bytes. @@ -80,7 +70,7 @@ library BouncerUtils { */ function signerOfMessageData(address _delegate) internal - view + pure returns (address) { bytes32 hashOfMessageData = keccak256( @@ -90,10 +80,11 @@ library BouncerUtils { ) ); - return signerWithSignature( - hashOfMessageData, - getSignatureArgument() - ); + return hashOfMessageData + .toEthSignedMessageHash() + .recover( + getSignatureArgument() + ); } diff --git a/contracts/mocks/BouncerDelegateImpl.sol b/contracts/mocks/SignatureDelegateImpl.sol similarity index 87% rename from contracts/mocks/BouncerDelegateImpl.sol rename to contracts/mocks/SignatureDelegateImpl.sol index 709bd480899..f4c8f384a53 100644 --- a/contracts/mocks/BouncerDelegateImpl.sol +++ b/contracts/mocks/SignatureDelegateImpl.sol @@ -1,10 +1,10 @@ pragma solidity 0.4.24; -import "../access/BouncerDelegate.sol"; +import "../signatures/SignatureDelegate.sol"; import "./BouncerMock.sol"; -contract BouncerDelegateImpl is BouncerDelegate { +contract SignatureDelegateImpl is SignatureDelegate { bool public shouldPass = false; BouncerMock public bouncer; diff --git a/contracts/access/IBouncerDelegate.sol b/contracts/signatures/ISignatureDelegate.sol similarity index 73% rename from contracts/access/IBouncerDelegate.sol rename to contracts/signatures/ISignatureDelegate.sol index 4794d76868b..dfada459409 100644 --- a/contracts/access/IBouncerDelegate.sol +++ b/contracts/signatures/ISignatureDelegate.sol @@ -3,13 +3,13 @@ pragma solidity 0.4.24; import "../introspection/ERC165.sol"; /** - * @title IBouncerDelegate - * @dev Implement this interface so that your contract can be a Bouncer's delegate. + * @title ISignatureDelegate + * @dev Implement this interface so that your contract can validate signatures. * inspired by https://github.com/0xProject/0x-monorepo/blob/v2-prototype/packages/contracts/src/2.0.0/protocol/Exchange/interfaces/IWallet.sol */ -contract IBouncerDelegate is ERC165 { +contract ISignatureDelegate is ERC165 { - bytes4 internal constant InterfaceId_BouncerDelegate = 0x1626ba7e; + bytes4 internal constant InterfaceId_SignatureDelegate = 0x1626ba7e; /** * 0x1626ba7e === * bytes4(keccak256('isValidSignature(bytes32,bytes)')) diff --git a/contracts/signatures/SignatureChecker.sol b/contracts/signatures/SignatureChecker.sol new file mode 100644 index 00000000000..ae8f569a131 --- /dev/null +++ b/contracts/signatures/SignatureChecker.sol @@ -0,0 +1,85 @@ +pragma solidity 0.4.24; + +import "../ownership/rbac/RBACOwnable.sol"; +import "./ISignatureDelegate.sol"; +import "./SignatureDelegate.sol"; +import "../ECRecovery.sol"; +import "../introspection/ERC165Checker.sol"; + + +contract SignatureChecker is RBACOwnable, SignatureDelegate { + using ECRecovery for bytes32; + using ERC165Checker for address; + + string public constant ROLE_DELEGATE = "delegate"; + + constructor() + public + { + // this contract implements ISignatureDelegate for all of its EOA delegate accounts. + addRole(address(this), ROLE_DELEGATE); + } + + /** + * @dev allows the owner to add additional signer addresses + */ + function addDelegate(address _delegate) + onlyOwners + public + { + require(_delegate != address(0)); + addRole(_delegate, ROLE_DELEGATE); + } + + /** + * @dev allows the owner to remove signer addresses + */ + function removeDelegate(address _delegate) + onlyOwners + public + { + require(_delegate != address(0)); + removeRole(_delegate, ROLE_DELEGATE); + } + + /** + * @dev tests to see if the signature is valid according to the delegate + * The delegate address must have the delegate role AND support isValidSignature + * This allows someone who wants custom validation logic (perhaps they check another contract's state) + * to code their own delegate. The users then submit signatures to the Bouncer, + * which then pings back to the delegate to check if it's cool. + * This is also useful for contracts-as-identities: your identity contract implements + * isValidTicket and can then recover the signer and check it against the whitelisted ACTION keys. + * @return bool validity of the signature + */ + function isDelegatedSignatureValidForHash(address _delegate, bytes32 _hash, bytes _sig) + internal + view + returns (bool) + { + bool isDelegate = hasRole(_delegate, ROLE_DELEGATE); + bool hasInterface = _delegate.supportsInterface(InterfaceId_SignatureDelegate); + + if (isDelegate && hasInterface) { + return ISignatureDelegate(_delegate).isValidSignature(_hash, _sig); + } + + // delegate is invalid, so signature is invalid as well + return false; + } + + /** + * @dev implement `isValidSignature` of ISignatureDelegate for EOA accounts that have the delegate role + */ + function isValidSignature( + bytes32 _hash, + bytes _sig + ) + public + view + returns (bool) + { + address signer = _hash.toEthSignedMessageHash().recover(_sig); + return hasRole(signer, ROLE_DELEGATE); + } +} diff --git a/contracts/access/BouncerDelegate.sol b/contracts/signatures/SignatureDelegate.sol similarity index 50% rename from contracts/access/BouncerDelegate.sol rename to contracts/signatures/SignatureDelegate.sol index 6f6513cdf4a..6db684f37ce 100644 --- a/contracts/access/BouncerDelegate.sol +++ b/contracts/signatures/SignatureDelegate.sol @@ -1,19 +1,19 @@ pragma solidity 0.4.24; -import "./IBouncerDelegate.sol"; +import "./ISignatureDelegate.sol"; import "../introspection/SupportsInterfaceWithLookup.sol"; /** - * @title BouncerDelegate - * @dev Partial implementation of IBouncerDelegate that adds ERC165 support + * @title SignatureDelegate + * @dev Partial implementation of ISignatureDelegate that adds ERC165 support * But you as the developer still have to add your signature validation logic. * As a delegate, you must proxy calls from accounts you consider valid. */ -contract BouncerDelegate is IBouncerDelegate, SupportsInterfaceWithLookup { +contract SignatureDelegate is ISignatureDelegate, SupportsInterfaceWithLookup { constructor () public { - _registerInterface(InterfaceId_BouncerDelegate); + _registerInterface(InterfaceId_SignatureDelegate); } } diff --git a/test/access/Bouncer.test.js b/test/access/Bouncer.test.js index 0e9db1cb426..563f4fbc9fc 100644 --- a/test/access/Bouncer.test.js +++ b/test/access/Bouncer.test.js @@ -4,7 +4,7 @@ const { getBouncerTicketGenerator } = require('../helpers/sign'); const { makeInterfaceId } = require('../helpers/makeInterfaceId'); const Bouncer = artifacts.require('BouncerMock'); -const BouncerDelegateImpl = artifacts.require('BouncerDelegateImpl'); +const SignatureDelegateImpl = artifacts.require('SignatureDelegateImpl'); require('chai') .use(require('chai-as-promised')) @@ -190,7 +190,7 @@ contract('Bouncer', ([_, owner, anyone, delegate, newDelegate]) => { context('contract delegate', () => { context('not a delegate', () => { beforeEach(async function () { - this.delegateContract = await BouncerDelegateImpl.new(true, this.bouncer.address, { from: owner }); + this.delegateContract = await SignatureDelegateImpl.new(true, this.bouncer.address, { from: owner }); }); it('should fail', async function () { @@ -202,7 +202,7 @@ contract('Bouncer', ([_, owner, anyone, delegate, newDelegate]) => { context('invalid delegate', () => { beforeEach(async function () { - this.delegateContract = await BouncerDelegateImpl.new(false, this.bouncer.address, { from: owner }); + this.delegateContract = await SignatureDelegateImpl.new(false, this.bouncer.address, { from: owner }); await this.bouncer.addDelegate(this.delegateContract.address, { from: owner }); }); @@ -215,7 +215,7 @@ contract('Bouncer', ([_, owner, anyone, delegate, newDelegate]) => { context('valid delegate', () => { beforeEach(async function () { - this.delegateContract = await BouncerDelegateImpl.new(true, this.bouncer.address, { from: owner }); + this.delegateContract = await SignatureDelegateImpl.new(true, this.bouncer.address, { from: owner }); await this.bouncer.addDelegate(this.delegateContract.address, { from: owner }); }); From ce2d206c5e4915e6eb8dd5a9e04d6a319742287c Mon Sep 17 00:00:00 2001 From: Matt Condon Date: Wed, 25 Jul 2018 17:31:50 +0800 Subject: [PATCH 09/10] fix: abstract to isSignedBy --- contracts/ECRecovery.sol | 22 +++++- contracts/access/Bouncer.sol | 66 ++++++++++++++++-- contracts/mocks/SignatureDelegateImpl.sol | 10 +-- contracts/signatures/SignatureChecker.sol | 85 ----------------------- 4 files changed, 84 insertions(+), 99 deletions(-) delete mode 100644 contracts/signatures/SignatureChecker.sol diff --git a/contracts/ECRecovery.sol b/contracts/ECRecovery.sol index a03a85acc88..6946f3d16b3 100644 --- a/contracts/ECRecovery.sol +++ b/contracts/ECRecovery.sol @@ -1,5 +1,8 @@ pragma solidity ^0.4.24; +import "./signatures/ISignatureDelegate.sol"; +import "./introspection/ERC165Checker.sol"; + /** * @title Elliptic curve signature operations @@ -7,8 +10,11 @@ pragma solidity ^0.4.24; * TODO Remove this library once solidity supports passing a signature to ecrecover. * See https://github.com/ethereum/solidity/issues/864 */ - library ECRecovery { + using ERC165Checker for address; + + // @TODO - de-dup this constant once we can share constants in solidity without a hack + bytes4 internal constant InterfaceId_SignatureDelegate = 0x1626ba7e; /** * @dev Recover signer address from a message by using their signature @@ -69,4 +75,18 @@ library ECRecovery { abi.encodePacked("\x19Ethereum Signed Message:\n32", _hash) ); } + + function isSignedBy(bytes32 _hash, address _delegate, bytes _sig) + internal + view + returns (bool) + { + // if the delegate address supports SignatureDelegation, delegate + if (_delegate.supportsInterface(InterfaceId_SignatureDelegate)) { + return ISignatureDelegate(_delegate).isValidSignature(_hash, _sig); + } + + // otherwise make sure the hash was personally signed by the EOA account + return _delegate == recover(toEthSignedMessageHash(_hash), _sig); + } } diff --git a/contracts/access/Bouncer.sol b/contracts/access/Bouncer.sol index c7ac5b0f676..2297e26e904 100644 --- a/contracts/access/Bouncer.sol +++ b/contracts/access/Bouncer.sol @@ -1,8 +1,9 @@ pragma solidity ^0.4.24; +import "../ownership/rbac/RBACOwnable.sol"; import "../ECRecovery.sol"; import "./BouncerUtils.sol"; -import "../signatures/SignatureChecker.sol"; +import "../signatures/SignatureDelegate.sol"; /** @@ -35,10 +36,12 @@ import "../signatures/SignatureChecker.sol"; * signature in it so the last 96 bytes of msg.data (which represents the _sig data itself) * is ignored when constructing the data hash that is validated. */ -contract Bouncer is SignatureChecker { +contract Bouncer is RBACOwnable, SignatureDelegate { using ECRecovery for bytes32; using BouncerUtils for bytes32; + string public constant ROLE_DELEGATE = "delegate"; + /** * @dev requires that a valid signature of a bouncer was provided * @notice does not validate method arguments @@ -69,6 +72,36 @@ contract Bouncer is SignatureChecker { _; } + + constructor() + public + { + // this contract implements ISignatureDelegate for all of its EOA delegate accounts. + addRole(address(this), ROLE_DELEGATE); + } + + /** + * @dev allows the owner to add additional signer addresses + */ + function addDelegate(address _delegate) + onlyOwners + public + { + require(_delegate != address(0)); + addRole(_delegate, ROLE_DELEGATE); + } + + /** + * @dev allows the owner to remove signer addresses + */ + function removeDelegate(address _delegate) + onlyOwners + public + { + require(_delegate != address(0)); + removeRole(_delegate, ROLE_DELEGATE); + } + /** * @dev is the signature of `delegate + sender` from a bouncer? * @return bool @@ -78,7 +111,7 @@ contract Bouncer is SignatureChecker { view returns (bool) { - return isDelegatedSignatureValidForHash( + return isSignatureValidForHash( _delegate, keccak256(abi.encodePacked(_delegate)), _sig @@ -94,7 +127,7 @@ contract Bouncer is SignatureChecker { view returns (bool) { - return isDelegatedSignatureValidForHash( + return isSignatureValidForHash( _delegate, keccak256(abi.encodePacked(_delegate, BouncerUtils.getMethodId())), _sig @@ -111,10 +144,33 @@ contract Bouncer is SignatureChecker { view returns (bool) { - return isDelegatedSignatureValidForHash( + return isSignatureValidForHash( _delegate, keccak256(abi.encodePacked(_delegate, BouncerUtils.getMessageData())), _sig ); } + + function isSignatureValidForHash(address _delegate, bytes32 _hash, bytes _sig) + internal + view + returns (bool) + { + bool isDelegate = hasRole(_delegate, ROLE_DELEGATE); + return isDelegate && _hash.isSignedBy(_delegate, _sig); + } + + function isValidSignature( + bytes32 _hash, + bytes _sig + ) + public + view + returns (bool) + { + return hasRole( + _hash.toEthSignedMessageHash().recover(_sig), + ROLE_DELEGATE + ); + } } diff --git a/contracts/mocks/SignatureDelegateImpl.sol b/contracts/mocks/SignatureDelegateImpl.sol index f4c8f384a53..3772e56d956 100644 --- a/contracts/mocks/SignatureDelegateImpl.sol +++ b/contracts/mocks/SignatureDelegateImpl.sol @@ -16,15 +16,9 @@ contract SignatureDelegateImpl is SignatureDelegate { bouncer = _bouncer; } - /** - * @dev verifies that a signature of a hash is valid - * @param _hash message hash that is signed - * @param _sig the provided signature - * @return bool validity of signature for the hash - */ function isValidSignature( - bytes32 _hash, - bytes _sig + bytes32, + bytes ) public view diff --git a/contracts/signatures/SignatureChecker.sol b/contracts/signatures/SignatureChecker.sol deleted file mode 100644 index ae8f569a131..00000000000 --- a/contracts/signatures/SignatureChecker.sol +++ /dev/null @@ -1,85 +0,0 @@ -pragma solidity 0.4.24; - -import "../ownership/rbac/RBACOwnable.sol"; -import "./ISignatureDelegate.sol"; -import "./SignatureDelegate.sol"; -import "../ECRecovery.sol"; -import "../introspection/ERC165Checker.sol"; - - -contract SignatureChecker is RBACOwnable, SignatureDelegate { - using ECRecovery for bytes32; - using ERC165Checker for address; - - string public constant ROLE_DELEGATE = "delegate"; - - constructor() - public - { - // this contract implements ISignatureDelegate for all of its EOA delegate accounts. - addRole(address(this), ROLE_DELEGATE); - } - - /** - * @dev allows the owner to add additional signer addresses - */ - function addDelegate(address _delegate) - onlyOwners - public - { - require(_delegate != address(0)); - addRole(_delegate, ROLE_DELEGATE); - } - - /** - * @dev allows the owner to remove signer addresses - */ - function removeDelegate(address _delegate) - onlyOwners - public - { - require(_delegate != address(0)); - removeRole(_delegate, ROLE_DELEGATE); - } - - /** - * @dev tests to see if the signature is valid according to the delegate - * The delegate address must have the delegate role AND support isValidSignature - * This allows someone who wants custom validation logic (perhaps they check another contract's state) - * to code their own delegate. The users then submit signatures to the Bouncer, - * which then pings back to the delegate to check if it's cool. - * This is also useful for contracts-as-identities: your identity contract implements - * isValidTicket and can then recover the signer and check it against the whitelisted ACTION keys. - * @return bool validity of the signature - */ - function isDelegatedSignatureValidForHash(address _delegate, bytes32 _hash, bytes _sig) - internal - view - returns (bool) - { - bool isDelegate = hasRole(_delegate, ROLE_DELEGATE); - bool hasInterface = _delegate.supportsInterface(InterfaceId_SignatureDelegate); - - if (isDelegate && hasInterface) { - return ISignatureDelegate(_delegate).isValidSignature(_hash, _sig); - } - - // delegate is invalid, so signature is invalid as well - return false; - } - - /** - * @dev implement `isValidSignature` of ISignatureDelegate for EOA accounts that have the delegate role - */ - function isValidSignature( - bytes32 _hash, - bytes _sig - ) - public - view - returns (bool) - { - address signer = _hash.toEthSignedMessageHash().recover(_sig); - return hasRole(signer, ROLE_DELEGATE); - } -} From 1e0fd856af89b58da5296c1ab4cc9bffea95d6f6 Mon Sep 17 00:00:00 2001 From: Matt Condon Date: Wed, 25 Jul 2018 17:34:12 +0800 Subject: [PATCH 10/10] fix: formatting of ERC165Checker --- contracts/introspection/ERC165Checker.sol | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/contracts/introspection/ERC165Checker.sol b/contracts/introspection/ERC165Checker.sol index c500f9d388e..30bdbb1d7f8 100644 --- a/contracts/introspection/ERC165Checker.sol +++ b/contracts/introspection/ERC165Checker.sol @@ -59,19 +59,19 @@ library ERC165Checker { // solium-disable-next-line security/no-inline-assembly assembly { - let x := mload(0x40) // Find empty storage location using "free memory pointer" - mstore(x, erc165ID) // Place signature at begining of empty storage to call supportsInterface() - mstore(add(x, 0x04), _interfaceId) // Place first argument directly next to signature + let x := mload(0x40) // Find empty storage location using "free memory pointer" + mstore(x, erc165ID) // Place signature at begining of empty storage to call supportsInterface() + mstore(add(x, 0x04), _interfaceId) // Place first argument directly next to signature success := staticcall( - 30000, // 30k gas - _address, // To addr - x, // Inputs are stored at location x - 0x20, // Inputs are 32 bytes long - x, // Store output over input (saves space) - 0x20) // Outputs are 32 bytes long - - result := mload(x) // Load the result + 30000, // 30k gas + _address, // To addr + x, // Inputs are stored at location x + 0x20, // Inputs are 32 bytes long + x, // Store output over input (saves space) + 0x20 // Outputs are 32 bytes long + ) + result := mload(x) // Load the result } } }