feat: Bouncer refactor, test streamlining, and vocabulary updates

[shrugs]

🚀 Description

Update the SignatureBouncer to have the correct signature size and make constructing signatures way easier.

• 📘 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).

[frangio]

Much improve! 🐶

[frangio]

It's Solidity's fault but if a contract has both Ownable and RBACOwnable in its inheritance graph there is no warning about the clashing modifier names... I'm not even sure what will happen.

Having both of those in the inheritance graph is probably a mistake, so in an ideal world the clashing modifiers would raise an error. Given that there is no error, I'm not sure what I prefer. 😟

It's also not clear to me why we're changing Bouncer into RBACOwnable. Perhaps we can undo that so as to sidestep the issue for now...

[shrugs]

modifiers will be overwritten just like function implementations afaik, so as long as RBAC is higher in the tree, this one takes precedence. I agree that it's confusing, but I bet it's expected behavior.

Contracts should only inherit from RBACOwnable or Ownable; I can't think of a reason a contract would inherit from both.

In any case, I still think RBACOwnable is a better primitive than ownable, and want to use it as basic access control on new features. thoughts?

[nventuro]

Not sure about using the current RBACOwnable for new features: I would either change the name of the modifier, or remove Ownable altogether.

While no-one should explicitly use both, I worry about the danger of someone inadvertently doing it indirectly, e.g., by extending a RefundableCrowdsale and then either adding RBAC ownership on top or inheriting from another contract that uses RBAC as the ownership model.

[shrugs]

I could change it to onlyOwners—how's that?

[nventuro]

Hm, I think that could work, specially considering that (how many owners there are) is the only difference between Ownable and RBACOwnable.

[frangio]

When does this happen?

[shrugs]

it occurs with function overloading; web3 < 1.0 will construct javascript functions like

methodName: () => {} // the first function def it saw int he abi
'methodName()': () => {} // same as above
'methodName(address)': () => {} // the next function signature

so this is necessary to be able to call overloaded function names

[frangio]

Why this change? I guess it's related to web3.sha3 vs soliditySha3... What's the difference between those two?

[shrugs]

soliditySha3 abi encodes and then tightly-packs arguments and sha3 just operates on a buffer or whatever you give it

[shrugs]

cc @PhABC and @frangio for comments on the latest commit

[PhABC]

It is important to note that msg.sender might not be the delegated contract address.

Take for example a token sale contract that utilizes bouncer functionalities to have an "off-chain" whitelist. The "signer" (the owner), who whitelists buyers address, is a multisig contract with a threshold of 3 signatures (out of 5). Hence, every buyer now has 3 signatures they would need to pass to the tokensale contract to be able to participate in the sale. In this case, the buyer is msg.sender, the "validator" is the multisig contract (e.g. ROLE_BOUNCER == WHITELIST_SIGNER). and the signers are one of the 5 team members.

Hence, I believe one need to pass the delegate / bouncer address which contains the validation logic. `

[shrugs]

I see what you mean, yeah; so we should allow a _delegate as part of the calldata, which is then signed and used in this function. Cool, will make that change

[PhABC]

Why use InterfaceId_BouncerDelegate? Isn't 165 used to make sure a given contract has implemented the proper function when it is called? I believe the right call would be supportsInterface(InterfaceId_IsValidSignature) to make sure the bouncer contract supports isValidSignature().

[shrugs]

In this case we're asking the sender (which is the supposed delegate) if they support the correct function. In this case we're calling isValidSignature on the Delegate, not the Bouncer, so I think this is correct.

[PhABC]

Sorry, I when I meant Bouncer contract, I meant the contract that has a bouncer role, i.e. the delegate. Why would the delegate contract support the InterfaceId_BouncerDelegate? Does it need to? All the delegate contract needs to have is isValidSignature(), no?

[shrugs]

That's what the InterfaceId_BouncerDelegate is testing, yeah :) — it checks the delegate for the isValidSignature method implementation.

[PhABC]

Ah, yes. We are on the same page then! Just saw the comment in IBouncerDelegate.sol.

[shrugs]

I dm'd these questions to @PhABC but asking @frangio and @nventuro as well:

1. so right now, Bouncer requires that messages are signed for a specific sender, which doesn't let anyone in an open market submit a transaction (i.e. a decentralized ethereum alarm clock). do you think it's ok to relax that requirement? Not sure what the security concerns would be off the top of my head. Since if you cared about, say, who's getting tokens in an airdrop, you'd put something like mint(address _to, uint256 _amount, bytes _ticket) onlyValidTicket(_ticket) and force whomever the sender is to use the original _to argument that was signed.

2. I also realized that the bouncer contract can/should be its own delegate so instead of requiring that contractAddress be the first 20 bytes that are signed, we can just put that into the method itself like onlyValidTicket(address _delegate, bytes _ticket) and then check _delegate. The Bouncer contract implements isValidSignature on behalf of all of the ethereum account delegates that it tracks.

But if I remove the address(this) from the hash, it requires that _delegate is passed into the function so it gets included in the hash; aka, only verifying the signature or only verifying the method ID won't work. I think I'm ok with that because I can't think of a ton of examples where I wouldn't want total control over the arguments being submitted to a contract on my behalf. And if someone does care about allowing someone to choose an argument after signing, that can be implemented separately.

So my plan is to

1. Remove the :contractAddress + :sender prefix from the hash, and remove the "only validate signature" and "only validate method ID" functions.
2. require it such that _delegate must be provided in the function's arguments as well as _ticket (this used to be _sig, but I'm unifying the language)

Another question: if we have two required arguments (_delegate, _ticket) would it make sense to require that they be compacted into a single bytes argument and then parsed out? I think I prefer it as two separate arguments like

function mint(address _to, uint256 _amount, address _delegate, bytes _ticket)
  onlyValidTicket(_delegate, _ticket)
  public
{
  MyToken.mint(_to, _amount);
}

note that if you're using an Ethereum Signed Message, you'd just make the Bouncer contract the _delegate argument.

Edit: we might not want that feature, since then it would allow someone to submit an identity contract delegated signature to two separate Bouncers. Bleh, yeah, it should probably force the to: param of an Ethereum TX.

[frangio]

Do you think it's ok to relax that requirement?

I think it's ok security-wise.

As for point 2, I'm gonna have to think about it a bit more and get back to you tomorrow. I'm not up to date on how Bouncer is being generalized for types of delegates other than ECDSA signers.

[shrugs]

@frangio for things other than direct ECDSA sigs, my primary use-case is identity contract support; i.e. "my contract is my identity and because it can't directly sign things, it delegates that responsibility to some keypairs that it's decided should be able to sign on its behalf"

[frangio]

The PR has diverged from a refactor into a big new feature. Judging from @PhABC's review, it seems like we're not all in the same page with regards to the design of delegates, so I'm going to mark it on hold until we figure out the requirements and a good design in #1005. (I'll be posting a summary of my thoughts there later today!)

[shrugs]

this PR depends on #1005 which depends on #1104 which depends on ethereum/EIPs#1258 which is discussed in ethereum/EIPs#1271

So: direct all discussion of validation standards to ethereum/EIPs#1271 after which we'll implement that standard and use it as part of this Bouncer refactor PR.

[shrugs]

closing in favor of #1272