Openzeppelin-contracts: [spec] Bouncer/SignatureChecker + Contract Signatures

[shrugs]

🎉 Description

We want to unify the methods by which we check signatures within the OZ contracts. The use-cases are:

• transparently supporting contract signatures via isValidSignature using the same interface as checking EOA signatures
• supporting MetaTx-style transactions
• supporting Bouncer-style off-chain authorization
• supporting signature arrays for collecting multiple off-chain signatures and submitting in a bundle (useful for threshold multisig logic)
• supporting Airdrop contracts ala Crowdsale.sol

I've done a half-job of building all of this out in a local branch, but I don't have the bandwidth to finish it up before the branch gets stale, so I'm going to document the architecture and get back to this later.

Pending Concerns

We should replace the eth_personalSign with eth_signedTypedData like GnosisSafe has done, which means we need to add the same sort of domainSeparator logic they have to the signature checker instead of assuming everything is an Ethereum Signed Message:.

Contracts

ISignatureValidator

https://github.com/ethereum/EIPs/issues/1271

pragma solidity 0.4.24;


interface ISignatureValidator {
  /**
   * @dev Should return whether the signature provided is valid for the provided data
   * @param _data Arbitrary length data signed on the behalf of address(this)
   * @param _signature Signature byte array associated with _data
   *
   * MUST return a bool upon valid or invalid signature with corresponding _data
   * MUST take (bytes, bytes) as arguments
   */
  function isValidSignature(
    bytes _data,
    bytes _signature
  )
    external
    returns (
      bool isValid
    );
}

SignatureChecker

SignatureChecker is a contract designed to be inherited. It offers pluggable authentication and authorization logic, designed to allow contracts to easily validate a set of signatures that a user submits.

It is inspired by logic from https://github.com/gnosis/safe-contracts/blob/development/contracts/GnosisSafe.sol

pragma solidity ^0.4.24;

import "./SignaturesSplitter.sol";
import "./ISignatureValidator.sol";
import "./BytesConverter.sol";
import "./ECRecovery.sol";


contract SignatureChecker {
  using ECRecovery for bytes32;

  // signature size is 65 bytes (tightly packed v (1) + r (32) + s (32))
  uint256 constant SIGNATURE_SIZE = 65;

  modifier onlyValidSignatures(bytes _data, bytes _signatures)
  {
    require(validSignatures(_data, _signatures), "INVALID_SIGNATURES");
    _;
  }

  function numSignatures(bytes _signatures)
    internal
    pure
    returns (uint256)
  {
    return _signatures.length / SIGNATURE_SIZE;
  }

  function validSignaturesLength(bytes _signatures)
    internal
    pure
    returns (bool)
  {
    return (_signatures.length % SIGNATURE_SIZE) == 0;
  }

  function validSignatures(bytes _data, bytes _signatures)
    internal
    view
    returns (bool)
  {
    uint256 numSigs = numSignatures(_signatures);
    if (!validSignaturesLength(_signatures)) {
      return false;
    }

    // There cannot be asigner with address 0
    address lastSigner = address(0);
    address currentSigner;
    bytes memory currentSignerSignature;

    for (uint256 i = 0; i < numSigs; i++) {
      bytes memory signature = SignaturesSplitter.signatureAt(_signatures, i);

      // get signer and signature
      (currentSigner, currentSignerSignature) = signerOf(signature, _data);

      // signer must be authenticated and authorized to sign for this data
      if (!(isAuthenticated(currentSigner) && isAuthorized(currentSigner, _data))) {
        return false;
      }

      // confirm that they've actually signed this
      if (!isSignedBy(_data, currentSigner, currentSignerSignature)) {
        return false;
      }

      // duplicated signature or improper order
      if (currentSigner <= lastSigner) {
        return false;
      }

      lastSigner = currentSigner;
    }

    return true;
  }

  function signerOf(bytes _signature, bytes _data)
    internal
    pure
    returns (
      address signer,
      bytes memory signature
    )
  {
    uint8 v = SignaturesSplitter.getV(_signature);
    // If v is 0 then it is a contract signature
    if (v == 0) {
      // When handling contract signatures the address of the contract is encoded into r
      signer = address(SignaturesSplitter.getR(_signature));
      signature = SignaturesSplitter.getContractSignature(_signature);
    } else {
      // for EOA accounts, signer is encoded in vrs for relevant data hash
      signature = _signature;
      signer = BytesConverter.toBytes32(_data, 0)
        .toEthSignedMessageHash()
        .recover(signature);
    }
  }

  function isSignedBy(
    bytes _data,
    address _signer,
    bytes _signature
  )
    internal
    view
    returns (bool)
  {
    if (_signer.supportsInterface(InterfaceId_SignatureValidator)) {
      // contract
      return ISignatureValidator(_signer).isValidSignature(_data, _signature);
    } else {
      // EOA
      return _signer == BytesConverter.toBytes32(_data, 0)
        .toEthSignedMessageHash()
        .recover(_signature);
    }
  }

  /**
   * @dev Whether or not `_signer` is authenticated within the context of this SignatureChecker
   * param _signer signer
   */
  function isAuthenticated(address)
    internal
    view
    returns (bool)
  {
    require(address(this) == 0, "INHERIT_ME"); // address(this) removes solc state read warning
  }

  /**
   * @dev Whether or not `_signer` is authorized to sign for `_data`
   * param _signer signer
   * param _data data
   */
  function isAuthorized(address, bytes)
    internal
    view
    returns (bool)
  {
    require(address(this) == 0, "INHERIT_ME"); // address(this) removes solc state read warning
  }
}

Bouncer

A Bouncer, then, is just some convenience functions for accepting signatures and assumes that the data being signed is the hash of msg.data.

We call the signatures "tickets" to imply that they're valid for the current transaction.

pragma solidity ^0.4.24;

import "./BouncerUtils.sol";
import "../../signatures/SignatureChecker.sol";
import "../../signatures/BytesConverter.sol";


contract Bouncer is SignatureChecker {

  modifier onlyValidTickets(bytes _tickets)
  {
    require(validTickets(_tickets), "INVALID_TICkETS");
    _;
  }

  function validTickets(bytes _tickets)
    internal
    view
    returns (bool)
  {
    return validSignatures(
      BytesConverter.toBytes(BouncerUtils.messageDataHash(_tickets.length)),
      _tickets
    );
  }
}

BouncerWithTrustedSigners

A Bouncer with trusted signers via Roles.sol. This implements the SignatureChecker functions.

pragma solidity ^0.4.24;

import "./Bouncer.sol";
import "../rbac/Roles.sol";

contract BouncerWithTrustedSigners is Bouncer {
  using Roles for Roles.Role;

  Roles.Role private signers;

  modifier onlyTrustedSigner() {
    require(signers.has(msg.sender), "DOES_NOT_HAVE_SIGNER_ROLE");
    _;
  }

  constructor(address[] _signers)
    public
  {
    signers.addMany(_signers);
  }

  /**
   * @dev allows trusted signer to add additional signers
   */
  function addTrustedSigner(address _signer)
    public
    onlyTrustedSigner
  {
    require(_signer != address(0), "NULL_SIGNER");
    signers.add(_signer);
  }

  /**
   * @dev allows trusted signer to remove other signers
   * @param _signer signer
   */
  function removeTrustedSigner(address _signer)
    public
    onlyTrustedSigner
  {
    signers.remove(_signer);
  }

  function isTrustedSigner(address _signer)
    public
    view
    returns (bool)
  {
    return signers.has(_signer);
  }

  /**
   * @dev Whether or not `_signer` is authenticated within the context of this SignatureChecker
   * @param _signer signer
   */
  function isAuthenticated(address _signer)
    internal
    view
    returns (bool)
  {
    return isTrustedSigner(_signer);
  }

  /**
   * @dev Whether or not `_signer` is authorized to sign for `_data`
   */
  function isAuthorized(address, bytes)
    internal
    view
    returns (bool)
  {
    // any signer is authorized for any signature
    // replace this function if you want to support stuff like multisig thresholds
    return true;
  }
}

BouncerWithNonceTracking

BouncerWithNonceTracking (name tbd) helps keep track of nonces per-address.

pragma solidity ^0.4.24;

contract BouncerWithNonceTracking is Bouncer {
  mapping(address => uint256) internal nonces;

  modifier withNonce(address _actor, uint256 _nonce) {
    require(_nonce > nonces[_actor], "INVALID_NONCE");
    _;
    nonces[_actor] = _nonce;
  }
}

Airdrop

And airdrop, then, would look like

pragma solidity ^0.4.24;

contract Airdrop is BouncerWithTrustedSigners, BouncerWithNonceTracking {
  function mint(address _beneficiary, IERC20 _token, uint256 _amount, bytes _tickets)
    public
    onlyValidTickets(_tickets)
    withNonce(_beneficiary, 1)
  {
    // trust arguments and can only be called once per beneficiary
    _token.transfer(_beneficiary, _amount);
  }
}

GnosisSafe

GnosisSafe could use this pattern as well: they would replace checkSignatures with validSignatures. They could implement threshold approvals in two ways:

1. check that numSignatures is gte approval threshold, for off-chain signature aggregation
2. check that the message hash has been approved as part of SignatureChecker#isAuthorized(...) for on-chain approvals

[shrugs]

I'm going to close PRs #1024, #812, #1020, #812, and #983 for cleanliness. They are all affected by this change and will be stale by the time we implement it.

[frangio]

Can you explain further how threshold signatures fit into this? Would there be a component in OpenZeppelin to specifically support them?

[shrugs]

threshold signatures probably isn't the best term, since that's already a real thing. But I'm referencing "be able to validate an array of signatures rather than a single signature" and then make it easy to apply some threshold logic to them. Specifically in the case of multisig schemes where a transaction might require 3 signatures from owners or something.

[frangio]

Right. That's the kind of thing I was expecting to delegate to contract signatures. So we can just say "we support isValidSignature" and Gnosis Safe can implement that function with their own threshold logic. Do you see OpenZeppelin having its own multisig validator component? I personally think it's best if we delegate that to actual multisig wallet contracts.

[shrugs]

@frangio I think we have the same opinion here! I'll clarify the text.

[biocrypto730]

?

[biocrypto730]

Is there a crowdsale example using this yet? It seems kinda necessary for KYC... And the code above looks great

[nventuro]

@projectoblio there are no crowdsales using signatures, but we do have WhitelistCrowdsale, which may suit your needs.

[biocrypto730]

I saw that, and I also read through someone else's argument in a closed issue that said that signature based crowdsales somehow cost the EVM a lot more money that WhitelistCrowdsales.

I just want to say that (I _think_) this has to be false, in a WhitelistCrowdsale you have a transaction (signature) from the owner to add an address (string), and another transaction from the end-user (signature). In a Bouncer crowdsale you have only a transaction (signature) from the end-user which contains a signature from the owner (signature).

So the WhitelistCrowdsale requires: Transaction + address + Transaction
Whereas the BouncerCrowdsale only requires: Transaction + Signature
--> I assume here that a Transaction is about the same size (or more) as a signature [ typical cryptocurrency transactions are just signatures moving money from one place to another, right?] then it gets added to an address so it's even bigger
--> BouncerCrowdsale also doesn't need to store long-term data in the chain (can just be a nonce or whatever, instead of a list of addresses that lasts forever) so yea it has to be a lot cheaper long-term, it's mostly computation at the time of the tx

Not only that, the UX experience is probably about equal. In the WhitelistCrowdsale, users have to wait for an owner transaction to be communicated to the blockchain before donated. In the BouncerCrowdsale, users can communicate their signature immediately.

This is my first time working with OZ so I haven't examined the gas costs of using each of the contracts. But as far as data storage goes I think the BouncerCrowdsale has to be a lot less.

Why u write all that, just submit a PR -- I actually would do this but my solution isn't backwards compatible with the rest of the OZ contracts. It basically requires adding an input parameter to buyTokens .I don't know enough about Solidity to know how to prevent re-entrancy ( although it doesn't seem like my crowdsale can be negatively impacted by it ). So that's what makes the above code so hot and attractive, is because it looks like it can be implemented in OZ really easily.

[nventuro]

It basically requires adding an input parameter to buyTokens

This could be implemented by inheriting the original buyTokens and disabling it (e.g. by having it always revert).

I agree with your statements regarding usefulness of a SignedCrowdsale, but am unsure as to whether we should first iterate a bit on SignatureBouncer itself, which hasn't been worked on in a while. @frangio thoughts?

[biocrypto730]

Thanks yea I might've tried that but there was also this big warning that said "DO NOT OVERRIDE" above buyTokens so I wasn't sure i was even doing it right. Rewriting the function inside Crowdsale.sol was easier on my late-night's psyche even though now i realize that's basically the exact same thing. And what am i even saying, is overriding the function the same as inherting it and disabling it? fuck man

[nventuro]

Ah you're right, the strategy I suggested wouldn't work on the Crowdsale model. What we should do is have an internal _buyTokens function, and then just have buyTokens call it. In your use case, you'd disable the public one, but not the internal, which is what the other contracts in the inheritance tree are calling.