From b0292cf628d9f8d65615bc07dd8af4844eb5a6d6 Mon Sep 17 00:00:00 2001 From: Adam Flesher Date: Sat, 9 Jun 2018 14:45:53 -0700 Subject: [PATCH] Add `isValidSignatureAndData` to Bouncer to verify method calls (#973) --- contracts/access/SignatureBouncer.sol | 71 +++++++++++ contracts/mocks/BouncerMock.sol | 32 +++++ test/access/SignatureBouncer.test.js | 162 ++++++++++++++++++++++++++ 3 files changed, 265 insertions(+) diff --git a/contracts/access/SignatureBouncer.sol b/contracts/access/SignatureBouncer.sol index b037b1ded..37d922a90 100644 --- a/contracts/access/SignatureBouncer.sol +++ b/contracts/access/SignatureBouncer.sol @@ -20,12 +20,25 @@ import "../ECRecovery.sol"; * @dev Then restrict access to your crowdsale/whitelist/airdrop using the * @dev `onlyValidSignature` modifier (or implement your own using isValidSignature). * @dev + * @dev In addition to `onlyValidSignature`, `onlyValidSignatureAndMethod` and + * @dev `onlyValidSignatureAndData` can be used to restrict access to only a given method + * @dev or a given method with given parameters respectively. + * @dev * @dev See the tests Bouncer.test.js for specific usage examples. + * @notice A method that uses the `onlyValidSignatureAndData` modifier must make the _sig + * @notice parameter the "last" parameter. You cannot sign a message that has its own + * @notice signature in it so the last 128 bytes of msg.data (which represents the + * @notice length of the _sig data and the _sig data itself) is ignored when validating. + * @notice Also non fixed sized parameters make constructing the data in the signature + * @notice much more complex. See https://ethereum.stackexchange.com/a/50616 for more details. */ contract SignatureBouncer is Ownable, RBAC { using ECRecovery for bytes32; string public constant ROLE_BOUNCER = "bouncer"; + uint constant METHOD_ID_SIZE = 4; + // (signature length size) 32 bytes + (signature size 65 bytes padded) 96 bytes + uint constant SIGNATURE_SIZE = 128; /** * @dev requires that a valid signature of a bouncer was provided @@ -36,6 +49,24 @@ contract SignatureBouncer is Ownable, RBAC { _; } + /** + * @dev requires that a valid signature with a specifed method of a bouncer was provided + */ + modifier onlyValidSignatureAndMethod(bytes _sig) + { + require(isValidSignatureAndMethod(msg.sender, _sig)); + _; + } + + /** + * @dev requires that a valid signature with a specifed method and params of a bouncer was provided + */ + modifier onlyValidSignatureAndData(bytes _sig) + { + require(isValidSignatureAndData(msg.sender, _sig)); + _; + } + /** * @dev allows the owner to add additional bouncer addresses */ @@ -73,6 +104,46 @@ contract SignatureBouncer is Ownable, RBAC { ); } + /** + * @dev is the signature of `this + sender + methodId` from a bouncer? + * @return bool + */ + function isValidSignatureAndMethod(address _address, 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(address(this), _address, data), + _sig + ); + } + + /** + * @dev is the signature of `this + sender + methodId + params(s)` from a bouncer? + * @notice the _sig parameter of the method being validated must be the "last" parameter + * @return bool + */ + function isValidSignatureAndData(address _address, 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(address(this), _address, data), + _sig + ); + } + /** * @dev internal function to convert a hash to an eth signed message * @dev and then recover the signature and check it against the bouncer role diff --git a/contracts/mocks/BouncerMock.sol b/contracts/mocks/BouncerMock.sol index ff100207b..11c76935b 100644 --- a/contracts/mocks/BouncerMock.sol +++ b/contracts/mocks/BouncerMock.sol @@ -19,4 +19,36 @@ contract SignatureBouncerMock is SignatureBouncer { { } + + function checkValidSignatureAndMethod(address _address, bytes _sig) + public + view + returns (bool) + { + return isValidSignatureAndMethod(_address, _sig); + } + + function onlyWithValidSignatureAndMethod(bytes _sig) + onlyValidSignatureAndMethod(_sig) + public + view + { + + } + + function checkValidSignatureAndData(address _address, bytes _bytes, uint _val, bytes _sig) + public + view + returns (bool) + { + return isValidSignatureAndData(_address, _sig); + } + + function onlyWithValidSignatureAndData(uint _val, bytes _sig) + onlyValidSignatureAndData(_sig) + public + view + { + + } } diff --git a/test/access/SignatureBouncer.test.js b/test/access/SignatureBouncer.test.js index 472a41226..48832032b 100644 --- a/test/access/SignatureBouncer.test.js +++ b/test/access/SignatureBouncer.test.js @@ -15,11 +15,36 @@ export const getSigner = (contract, signer, data = '') => (addr) => { return signHex(signer, message); }; +export const getMethodId = (methodName, ...paramTypes) => { + // methodId is a sha3 of the first 4 bytes after 0x of 'method(paramType1,...)' + return web3.sha3(`${methodName}(${paramTypes.join(',')})`).substr(2, 8); +}; + +export const stripAndPadHexValue = (hexVal, sizeInBytes, start = true) => { + // strip 0x from the font and pad with 0's for + const strippedHexVal = hexVal.substr(2); + return start ? strippedHexVal.padStart(sizeInBytes * 2, 0) : strippedHexVal.padEnd(sizeInBytes * 2, 0); +}; + contract('Bouncer', ([_, owner, authorizedUser, anyone, bouncerAddress, newBouncer]) => { before(async function () { this.bouncer = await Bouncer.new({ from: owner }); this.roleBouncer = await this.bouncer.ROLE_BOUNCER(); this.genSig = getSigner(this.bouncer, bouncerAddress); + this.uintValue = 23; + this.checkValidSignatureAndMethodId = getMethodId('checkValidSignatureAndMethod', 'address', 'bytes'); + this.uintValueData = stripAndPadHexValue(web3.toHex(this.uintValue), 32); + this.authorizedUserData = stripAndPadHexValue(authorizedUser, 32); + this.bytesValue = web3.toHex('bytesValue'); + this.validateSignatureAndDataMsgData = [ + getMethodId('checkValidSignatureAndData', 'address', 'bytes', 'uint256', 'bytes'), + stripAndPadHexValue(authorizedUser, 32), + stripAndPadHexValue(web3.toHex(32 * 4), 32), // bytesValue location + this.uintValueData, + stripAndPadHexValue(web3.toHex(32 * 6), 32), // sig location + stripAndPadHexValue(web3.toHex(this.bytesValue.substr(2).length / 2), 32), // bytesValue size + stripAndPadHexValue(this.bytesValue, 32, false), // bytesValue + ]; }); it('should have a default owner of self', async function () { @@ -54,6 +79,50 @@ contract('Bouncer', ([_, owner, authorizedUser, anyone, bouncerAddress, newBounc ) ); }); + it('should allow valid signature with a valid method for sender', async function () { + const sig = getSigner( + this.bouncer, + bouncerAddress, + getMethodId('onlyWithValidSignatureAndMethod', 'bytes') + )(authorizedUser); + await this.bouncer.onlyWithValidSignatureAndMethod( + sig, + { from: authorizedUser } + ); + }); + it('should not allow invalid signature with method for sender', async function () { + await assertRevert( + this.bouncer.onlyWithValidSignatureAndMethod( + 'abcd', + { from: authorizedUser } + ) + ); + }); + it('should allow valid signature with a valid data for sender', async function () { + const sig = getSigner( + this.bouncer, + bouncerAddress, + [ + getMethodId('onlyWithValidSignatureAndData', 'uint256', 'bytes'), + this.uintValueData, + stripAndPadHexValue(web3.toHex(64), 32), + ].join('') + )(authorizedUser); + await this.bouncer.onlyWithValidSignatureAndData( + this.uintValue, + sig, + { from: authorizedUser } + ); + }); + it('should not allow invalid signature with data for sender', async function () { + await assertRevert( + this.bouncer.onlyWithValidSignatureAndData( + this.uintValue, + 'abcd', + { from: authorizedUser } + ) + ); + }); }); context('signatures', () => { @@ -85,6 +154,99 @@ contract('Bouncer', ([_, owner, authorizedUser, anyone, bouncerAddress, newBounc ); isValid.should.eq(false); }); + it('should accept valid message with valid method for valid user', async function () { + const sig = getSigner( + this.bouncer, + bouncerAddress, + getMethodId('checkValidSignatureAndMethod', 'address', 'bytes') + )(authorizedUser); + const isValid = await this.bouncer.checkValidSignatureAndMethod( + authorizedUser, + sig + ); + isValid.should.eq(true); + }); + it('should not accept valid message with an invalid method for valid user', async function () { + const sig = getSigner( + this.bouncer, + bouncerAddress, + getMethodId('invalidMethod', 'address', 'bytes') + )(authorizedUser); + const isValid = await this.bouncer.checkValidSignatureAndMethod( + authorizedUser, + sig + ); + isValid.should.eq(false); + }); + it('should not accept valid message with a valid method for an invalid user', async function () { + const sig = getSigner( + this.bouncer, + bouncerAddress, + this.checkValidSignatureAndMethodId + )(authorizedUser); + const isValid = await this.bouncer.checkValidSignatureAndMethod( + anyone, + sig + ); + isValid.should.eq(false); + }); + it('should accept valid method with valid params for valid user', async function () { + const sig = getSigner( + this.bouncer, + bouncerAddress, + this.validateSignatureAndDataMsgData.join('') + )(authorizedUser); + const isValid = await this.bouncer.checkValidSignatureAndData( + authorizedUser, + this.bytesValue, + this.uintValue, + sig + ); + isValid.should.eq(true); + }); + it('should not accept an invalid method with valid params for valid user', async function () { + this.validateSignatureAndDataMsgData[0] = getMethodId('invalidMethod', 'address', 'bytes', 'uint256', 'bytes'); + const sig = getSigner( + this.bouncer, + bouncerAddress, + this.validateSignatureAndDataMsgData.join('') + )(authorizedUser); + const isValid = await this.bouncer.checkValidSignatureAndData( + authorizedUser, + this.bytesValue, + this.uintValue, + sig + ); + isValid.should.eq(false); + }); + it('should not accept valid method with invalid params for valid user', async function () { + const sig = getSigner( + this.bouncer, + bouncerAddress, + this.validateSignatureAndDataMsgData.join('') + )(authorizedUser); + const isValid = await this.bouncer.checkValidSignatureAndData( + authorizedUser, + this.bytesValue, + 500, + sig + ); + isValid.should.eq(false); + }); + it('should not accept valid method with valid params for invalid user', async function () { + const sig = getSigner( + this.bouncer, + bouncerAddress, + this.validateSignatureAndDataMsgData.join('') + )(authorizedUser); + const isValid = await this.bouncer.checkValidSignatureAndData( + anyone, + this.bytesValue, + this.uintValue, + sig + ); + isValid.should.eq(false); + }); }); context('management', () => {