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

aflesher · 2018-06-01T22:04:58Z

Fixes #891

🚀 Description

SignatureBouncer.sol has two additional signature verification methods:

isValidSignatureAndMethod which validates that the external method called matches the methodId in the signatures message.
isValidSignatureAndData which validates that the external method called with params matches the methodId and param values in the signatures method.

Details

msg.data for any given external call is comprised of the methodId followed by the values of each parameter. A signature can append the required methodId and optionally the required parameter values to it's message. isValidSignatureAndMethod and isValidSignatureAndData validate that msg.data matches the signed data. SignatureBouncer.test.js demonstrates how to assemble the expected msg.data correctly and add it to the signature.

📘 I've reviewed the OpenZeppelin Contributor Guidelines
✅ I've added tests where applicable to test my new functionality.
📖 I've made sure that my contracts are well-documented.
🎨 I've run the JS/Solidity linters and fixed any issues (npm run lint:all:fix).

shrugs · 2018-06-04T22:14:54Z

test/access/SignatureBouncer.test.js

+    });
+    it('should accept valid method with valid params for valid user', async function () {
+      const methodId = getMethodId('checkValidSignatureAndData', 'address', 'uint256', 'bytes');
+      const val = 23;


let's de-duplicate these variables by bringing them either into a context variable using a beforeEach within a context or just making them constants above these methods

shrugs · 2018-06-04T22:17:27Z

Can we also add a @notice at the top of SignatureBouncer to talk about how _sig must be the last argument at all times? Good to have it on the individual methods as well.

…about onlyValidSignatureAndData last paramater

aflesher · 2018-06-05T00:16:30Z

contracts/access/SignatureBouncer.sol

 */
 contract SignatureBouncer is Ownable, RBAC {
  using ECRecovery for bytes32;

  string public constant ROLE_BOUNCER = "bouncer";
+  uint constant METHOD_ID_SIZE = 4;
+  uint constant SIG_SIZE = 160;


@shrugs I forgot that I wanted to comment on this variable. The signature value is always 160 bytes within msg.data even though _sig.length is always much smaller (64 bytes I think?). I guess it pads this data to the max size? Regardless, it's always correct but I wanted to call attention to the fact that I wasn't totally sure why.

I spent a bit of time playing around with the abi documentation and realized that bytes are encoded as follows

[distance to the beginning of the argument data] -> 32 bytes [the boolean true] -> 32 bytes [length of the data] -> 32 bytes [the data, right-padded to a multiple of 32] -> 64 bytes

and there we have our 160.

I have a question, though, after looking at the abi documentation: can we add a test case to see if it's possible to have a dynamic argument before the final bytes argument? i.e. can we sign a bouncer signature for

function test(bytes _someBytes, bytes _sig) onlyValidSignatureAndData(_sig) { }

my hunch is that this won't work immediately because with dynamically sized arguments, their location in the transaction data is included in the tx data before the argument itself.

i.e. I think a transaction for the above function looks like

0x12345678 // 8 bytes [ location of data for _someBytes ] // (8 bytes + 32 + 32) [ location of data for _sig ] // (8 + 32 + 32 + 32 + len(_someBytes)) [ enc(len(_someBytes) ] [ _someBytes right-padded to modulo 32 ] [ enc(len(_sig)) ] [ _sig right-padded to module 32 ] // (64 bytes long in our case)

which means we're lucky that the examples only use fixed-sized arguments so we don't have to calculate stuff like "at what offset will the encoded signature be in the transaction data for this function call".

Thanks for shedding some light on this. I'll add that test and see what we get

question about non-fixed-sized arguments before _sig

… params. Needs cleanup and more tests

aflesher · 2018-06-05T22:17:15Z

Ok, I altered those tests to verify that variable sized parameters will validate correctly.

The only change to the SignatureBouncer contract was to crop the last 128 bytes instead of the last 160 bytes. The signature is 65 bytes not 64 bytes which is why it gets padded up to 96 bytes. So this is cropped along with the 32 bytes for the signature length size.

shrugs · 2018-06-06T08:06:28Z

Looks like it is necessary to construct the full tx data, sans signature, and then sign it, which is a little cumbersome in practice; it means we won't be able to use the default web3 tx construction code.

Looks like this calls for a little javascript helper in the future, as a separate package most likely.

This looks good to me! @frangio can you take a look as well?

shrugs · 2018-06-06T23:18:28Z

contracts/access/SignatureBouncer.sol

 * @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 160 bytes is of msg.data (which represents _sig)


Can we update this to the new value of 128 and also mention that for this to be easy to construct signatures for, your arguments must be fixed-sized?

(sorry, just took one last pass through it and I think it's worth adding a comment about this)

@shrugs Ah, sorry, my bad. I'll get that changed. I actually went and posted a detailed answer on a stack overflow thread about how msg.data is constructed https://ethereum.stackexchange.com/a/50616/40921 . Do you want me to link this thread in the comments?

sure, that's a solid answer

shrugs · 2018-06-07T21:26:26Z

Still want a second pair of eyes; will ask the maintainers and hopefully get this in soon; I want to start using it immediately :D

come-maiz

This is very interesting. I couldn't find anything wrong on the code, but I recommend to get an extra review from somebody more experienced than me. Maybe @frangio can do at least a safety check.

As an aside, @aflesher @shrugs would you write a blog post explaining this feature? I think it would be a nice complement to get more people using and testing it.

come-maiz · 2018-06-08T04:03:00Z

contracts/access/SignatureBouncer.sol

+ * @notice signature in it so the last 128 bytes is 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://bit.ly/2sGSPBc for more details.


I prefer the original URLs instead of shorteners. Please update the link to:
https://ethereum.stackexchange.com/a/50616

come-maiz · 2018-06-08T04:04:57Z

contracts/access/SignatureBouncer.sol

+ * @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 is of msg.data (which represents the
+ * @notice length of the _sig data and the _sig data itself) is ignored when validating.


I'm having problems parsing this:
so the last 128 bytes is of msg.data [...] is ignored.
The two "is" verbs confuse me, is that a typo?

come-maiz · 2018-06-08T04:09:04Z

contracts/access/SignatureBouncer.sol

 */
 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 SIG_SIZE = 128;


why SIG instead of SIGNATURE?

I was just keeping this variable name consistent with our _sig params naming. SIGNATURE does make more sense though, I'll change it.

come-maiz · 2018-06-08T04:10:26Z

contracts/access/SignatureBouncer.sol

+  /**
+   * @dev requires that a valid signature with a specifed method of a bouncer was provided
+   */
+  modifier onlyValidSignatureAndMethod(bytes _sig)


I would use _signature here too. But for consistency, maybe it's better to leave it and update it in a future branch.

let's follow this up with a change to _signature, yeah.

aflesher · 2018-06-08T20:54:13Z

@ElOpio A blog post explaining this feature is a great idea. I was thinking about writing a javascript helper function to construct this bytecode as well. Would web3.js be the most logical place to propose this feature? Or maybe ethereumjs-util?

shrugs · 2018-06-08T20:56:32Z

It would be neat to have to bundled in the core utils, but I doubt it's general-case enough that they'd merge it. Perhaps ethjs-utils? it may fit into the web3-utils sub-package of web3, though. Hard to say

frangio · 2018-06-09T22:00:50Z

Sorry for taking a while to get to this. Great work @aflesher!

I have some comments/ideas about this but I will open another issue to discuss there.

frangio · 2018-06-11T22:45:55Z

Actualy, I feel more comfortable discussing here a bit because I don't have a concrete proposal, and I have some questions.

Is this complex procedure really necessary? I saw in the comments here that the web3 tx construction code couldn't be used, but I was actually able to use getData for this purpose just fine.

this.bouncer.contract.checkValidSignatureAndData.getData(
  authorizedUser,
  this.bytesValue,
  this.uintValue,
  '0x00000000000000000000000000000000'
).slice(2, -128);

(The .contract part is necessary to retrieve the underlyling web3.Contract instance from the truffle-contract instance, which doesn't expose getData.)

Before realizing that this was possible I was going to say that the huge complexity to use these functions was indicative of an API design problem. There's still something that feels odd to me, though it was already in the previous SignatureBouncer function: why is the _sig argument a dynamic-length bytes? It will always be a fixed size byte array. Making it fixed size would complicate a bit further the serialization of the arguments, though, because fixed size arrays are inlined where they appear instead of appended at the end.

Just wanna hear your thoughts and see if we can come up with some improvements to the usage of these new functions.

shrugs · 2018-06-11T23:33:59Z

@frangio that's actually super smart, I'm amazed we missed that 😨

So we can definitely use the built-in data construction code, but can't use the built-in transaction signing code. So the utility function for this should look something like:

const voucherForCallTo = (fromAddress, contract, method, ...args) => {
  const data = contract.methods[method](...args).encodeABI().slice(0, -120)
  const hash = web3.utils.soliditySha3(`${fromAddress}${contract.address}${data}`)
  return web3.eth.personalSign(hash)
}

which looks super nice (but uses web3-1.0, so we have to update that syntax for the current version
(https://web3js.readthedocs.io/en/1.0/web3-eth-contract.html#methods-mymethod-encodeabi)

on a separate note, I want to explore the idea proposed by @PhABC where a contract can be the authority, but delegate authorizations to other arbitrary logic

so something like:


contract Bouncer {
  function isValidSignature(hash, _sig)
    internal
  {
    const signer = signerOf(_sig)
    if (signer.isContract()) {
      // ERC165 detect and call on isValidSignature
      return signer.isValidSignature(hash, _sig)
    } else {
      return hasRole(signer, ROLE_BOUNCER);
    }
  }
}

contract MyContract {
  function isValidSignature(data, signature)
    public
    returns (bool)
  {
    return isInWhitelist(data.recover(signature));
    // or like `return confirmedSignatures[signature] == true` for multisig authorizations
  }
}

aflesher · 2018-06-13T15:44:02Z

@frangio @shrugs Is there any follow-up action needed here? The unit tests could definitely be cleaned up but it sounds like you guys want to explore a different authorization pattern.

shrugs · 2018-06-13T18:25:55Z

@aflesher it's more of an extension to the authorization pattern; the ability to defer the "isValidSignature" logic to a contract rather than just an account. This is actually something like 0x is doing as part of v2, so I think it's a good add.

I'll be cleaning up the signature construction part of the tests later on when we decide how to structure the contract-as-bouncer logic.

Also in that PR I'm going to update the language because I've been using bouncer to mean both the contract that checks signature and the address that signs the permissions. Gonna call the signer something like Inviter or something, unless y'all have a better name.

frangio · 2018-06-13T20:19:06Z

I'd like to discuss that idea elsewhere, is there an issue already?

shrugs · 2018-06-13T21:15:43Z

@frangio moving to #1005 👍

aflesher added 5 commits June 1, 2018 11:32

Core functionality is now working. Needs cleanup and more unit tests

98538ea

More cleanup, added missing unit tests

273c846

Fixing lint errors

2622a66

Added modifiers and tests

be50c84

Merge branch 'master' into feature/validate-data-and-sig-#891

da1a4f1

shrugs mentioned this pull request Jun 4, 2018

Send by Signature (ERC777 extension) - transfer without paying for gas ethereum/EIPs#965

Closed

shrugs previously approved these changes Jun 4, 2018

View reviewed changes

shrugs mentioned this pull request Jun 4, 2018

rbac mintable erc721 token with signature bouncer #950

Closed

4 tasks

aflesher added 3 commits June 4, 2018 16:23

Merge branch 'master' into feature/validate-data-and-sig-#891

6afc6b3

Added a more detailed description of the new modifiers with a notice …

29036cc

…about onlyValidSignatureAndData last paramater

Refactored signature bouncer test to de-dup variables

a02b258

aflesher commented Jun 5, 2018

View reviewed changes

Merge branch 'master' into feature/validate-data-and-sig-#891

d2f6fc7

aflesher added 4 commits June 5, 2018 10:21

Fixed the signature length issue and added a test with multiple bytes…

91ec2c1

… params. Needs cleanup and more tests

Cleanup and refactor

006db16

Fixing lint errors

b30361d

Merge branch 'master' into feature/validate-data-and-sig-#891

2a8e1d5

shrugs approved these changes Jun 6, 2018

View reviewed changes

Merge branch 'master' into feature/validate-data-and-sig-#891

68d9b74

shrugs suggested changes Jun 6, 2018

View reviewed changes

Added more details in the Contract description

b528c26

shrugs approved these changes Jun 7, 2018

View reviewed changes

come-maiz reviewed Jun 8, 2018

View reviewed changes

Rename SIG_SIZE to SIGNATURE_SIZE and cleaning up comments

aab0822

shrugs approved these changes Jun 8, 2018

View reviewed changes

aflesher and others added 2 commits June 8, 2018 15:42

Merge branch 'master' into feature/validate-data-and-sig-#891

7e050b6

Merge branch 'master' into feature/validate-data-and-sig-#891

3f3495c

shrugs merged commit b0292cf into OpenZeppelin:master Jun 9, 2018

aflesher mentioned this pull request Jun 11, 2018

Add functions to generate the bytecode for methodId and msg.data ethereumjs/ethereumjs-util#134

Closed

aflesher deleted the feature/validate-data-and-sig-#891 branch June 11, 2018 20:51

shrugs mentioned this pull request Jun 13, 2018

Allow contracts to be delegates of a Bouncer #1005

Closed

1 task

shrugs added in progress and removed in progress labels Jul 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

aflesher commented Jun 1, 2018 •

edited by shrugs

Loading

shrugs Jun 4, 2018

shrugs commented Jun 4, 2018

aflesher Jun 5, 2018

shrugs Jun 5, 2018

aflesher Jun 5, 2018

aflesher commented Jun 5, 2018

shrugs commented Jun 6, 2018

shrugs Jun 6, 2018

shrugs Jun 6, 2018

aflesher Jun 6, 2018

shrugs Jun 6, 2018

shrugs commented Jun 7, 2018

come-maiz left a comment

come-maiz Jun 8, 2018

come-maiz Jun 8, 2018

come-maiz Jun 8, 2018

aflesher Jun 8, 2018

come-maiz Jun 8, 2018

shrugs Jun 8, 2018

aflesher commented Jun 8, 2018 •

edited

Loading

shrugs commented Jun 8, 2018

frangio commented Jun 9, 2018

frangio commented Jun 11, 2018

shrugs commented Jun 11, 2018

aflesher commented Jun 13, 2018

shrugs commented Jun 13, 2018

frangio commented Jun 13, 2018

shrugs commented Jun 13, 2018

Add isValidSignatureAndData to Bouncer to verify method calls #973

Add isValidSignatureAndData to Bouncer to verify method calls #973

Conversation

aflesher commented Jun 1, 2018 • edited by shrugs Loading

🚀 Description

Details

Choose a reason for hiding this comment

shrugs commented Jun 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aflesher commented Jun 5, 2018

shrugs commented Jun 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shrugs commented Jun 7, 2018

come-maiz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aflesher commented Jun 8, 2018 • edited Loading

shrugs commented Jun 8, 2018

frangio commented Jun 9, 2018

frangio commented Jun 11, 2018

shrugs commented Jun 11, 2018

aflesher commented Jun 13, 2018

shrugs commented Jun 13, 2018

frangio commented Jun 13, 2018

shrugs commented Jun 13, 2018

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

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

aflesher commented Jun 1, 2018 •

edited by shrugs

Loading

aflesher commented Jun 8, 2018 •

edited

Loading