Add `isValidSignatureAndData` to Bouncer to verify method calls (#973)

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

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
+  {
+
+  }
 }

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', () => {