Implement identity proxies #7363

m-chrzan · 2021-03-08T17:47:44Z

Description

This PR introduces the concept of identity proxies, which are smart contract addresses from which calls can be made only by "owners" of a related identity. "Owning" an identity is determined by an on-chain heuristic using the Attestations contract.

Tested

Suite of unit tests

Related issues

Fixes Use CREATE2 in AttestationCallProxyV1 #7164

Backwards compatibility

Only introduces new contracts.

nategraf

I like this direction a lot! It's quite simple and seems to fulfill the needs for this application. left some comments on some specifics. More generally, it would be useful to have a bit more clarity on where the identity rules execution will be and how that will be implemented. How parameters for the rules will be encoded is a question I have as well. My assumption is that it would be encoded into the Hub/Factory (such that a consumer with different parameter tuning needs would deploy a new hub/factory), but would like to see more detail on that.

nategraf · 2021-03-09T19:38:11Z

packages/protocol/contracts/identity/IdentityProxyHub.sol

+contract IdentityProxyHub {
+  mapping(bytes32 => IdentityProxy) public identityProxies;
+
+  function getIdentityProxy(bytes32 identifier) public returns (IdentityProxy) {


In the view that Ethereum is a database, I feel like this should be split into two functions: A view function called getIdentityProxy that returns the address or address(0) and a state-modifying function createIdentityProxy that creates a new proxy of no-ops or reverts if one already exists.

So the getter already exists - identityProxies is a public mapping. I think my intuition for why getIdentityProxy works the way it does is akin to the Singleton pattern. There's essentially supposed to be a singleton IdentityProxy per identifier, and this is the function for getting it.

I agree that the naming here is misleading
imo any function prefixed with get I would expect to only use eth_call against

nategraf · 2021-03-09T19:41:14Z

packages/protocol/contracts/identity/IdentityProxyHub.sol

+    return identityProxies[identifier];
+  }
+
+  function makeCall(bytes32 identifier, address destination, uint256 value, bytes calldata data)


Instead of having the ownership check being here, and needing an external call to the proxy contract, why not have the Identity proxy execute the logic and have the caller call it directly to save gas and a bit of contract complexity. (The caller will already need to be aware of the proxies address to use it for it's application, and it can look up that address as a view call if it doesn't have it)

I was actually thinking about this as well. I don't feel particularly strongly and would be open to moving the heuristic to IdentityProxy instead.

The benefits of the current approach is less deployed bytecode in storage (at the cost of slightly higher gas spent over time) and that you only need to care about the single IdentityProxyHub address if all you're doing is making calls (you don't even need to know your proxy's address since currently makeCall will provision it/look it up for you if you don't know it), which could make some parts of applications simpler.

That said, I do agree that long term this will burn more gas, assuming we expect users to make many makeCalls. Guess that's not entirely true in the use case we're currently building for (account recovery), but could become true depending on what these contracts are going to be used for in the future.

I'm glad you are thinking about this and based on our use case, account recovery, we can assume that we may deploy many contracts that will never be called, and those that are will be called only a few times. So I agree with your assessment that including the check here will use less storage, and its likely to use less gas as well.

At first I found this a bit more complex from a mental model standpoint, but on second reading I no longer think so. The lack of ability to use the IdentityProxy in a standalone fashion also seems like an unnecessary feature with some more thinking. Overall, I like this way of doing it 👍

nategraf · 2021-03-09T19:42:43Z

packages/protocol/contracts/identity/IdentityProxy.sol

+  }
+
+  function makeCall(address destination, uint256 value, bytes calldata data) external onlyDeployer {
+    ExternalCall.execute(destination, value, data);


If the identity logic were executed here instead of in the deployer, it would be possible to deploy these in a trust-less standalone fashion, or via the factory. It would also remove an external call from the transaction execution, which is a nice gas savings.

yorhodes · 2021-03-09T20:15:07Z

packages/protocol/contracts/identity/IdentityProxy.sol

+
+import "../common/ExternalCall.sol";
+
+contract IdentityProxy {


I am against overloading Proxy like this
We have tooling all over the place which assumes this suffix implies OZ unstructured storage proxy pattern
Is there a synonym we could use or a different naming scheme?

On a slightly different naming angle, as the current contract is written it actually has nothing to do with identity and could be used in more generic ways. So we might remove that word from its name unless the heuristic call is moved into this code.

Forwarder? Relayer? Maybe a little more abstract, but IdentityAlias if we keep the semantics of using it in the identity scheme in the name.

I like Relay personally but basically anything other than Proxy is good imo

nategraf · 2021-03-10T00:19:01Z

Based on how this is shaping up, we should consider using CREATE2 here to avoid deploying the IdentityProxy contracts until they are needed. Most users will never need to use account recovery, so delaying deployment until the proxy needs to be use could result in a large storage and gas savings as potentially millions of users are onboarded to this system.

Tooling support for CREATE2 is not perfect, but Solidity added support in 0.6.2 and OpenZeppelin has a guide on how to use CREATE2 before that point as well.

Using CREATE2 here would be relatively simple. On each call of IdentityProxyHub.makeCall, statically calculate the IdentityProxy address and checks if it is a contract. If not, it uses new IdentityProxy{ salt: identifier }() to create it, then makes the call to the precomputed address.

The address can be computed using a formula from the Solidity documentation, making getIdentityProxy a view function.

function getIdentityProxy(bytes32 identifier) public view returns (address) {
     return address(bytes20(keccak256(abi.encodePacked(
          byte(0xff),
          address(this),
          identifier,
          keccak256(abi.encodePacked(
              type(IdentityProxy).creationCode
          ))
      ))));
}

It's a bit more complicated, especially conceptually for those unfamiliar with CREATE2, but it would be a substantial gas savings over the long term.

m-chrzan · 2021-03-10T15:15:53Z

Thanks for all the great comments, @nategraf. I really like the CREATE2 idea - that would really minimize overall gas costs and storage requirements, especially considering the use case of phone number recovery where hopefully not many identity proxies need to be deployed or used.

Small, but important, technical note to what you wrote: I think the address(bytes20(... conversion would need to be changed to something like in this discussion to get the correct bytes of the hash.

m-chrzan · 2021-04-15T07:37:49Z

A few design questions:

For the hasMostCompletions step of the identity heuristic, we currently use a non-strict >= equality. This has the benefit that when you change to a new address but keep your phone number, you only need complete the same number of attestations you had before. If we change to >, after each address change you would need at least one additional attestation. On the other hand, this currently allows multiple addresses to be allowed by the identity heuristic, which could be seen as bad design.
Does the amount of usage and cost of deployment of these proxies warrant adding a similar EIP-1167 setup as with MTW? From my intuition it's not as necessary since the recovery flow will (hopefully!) be much less common than onboarding flow, and these proxies are deployed lazily thanks to CREATE2, but long term using EIP-1167 clones would lead to much less bytecode deployed.

nategraf

Overall, looks great! I had several smaller comments

On the hasMostCompletions using strictly greater than, I agree with the current implementation, allowing the call if the caller has at least as many completions as any other account. It's true that this allows for more than one account to be linked to a number under this heuristic at a time, but that is not entirely unexpected, especially given the application to account recovery.

On EIP-1167, I agree with your intuition that we should skip that for now.

nategraf · 2021-04-15T21:50:25Z

packages/protocol/contracts/identity/IdentityProxyHub.sol

+    abi.encodePacked(type(IdentityProxy).creationCode)
+  );
+
+  mapping(bytes32 => IdentityProxy) public identityProxies;


Do we need this mapping given that we can always calculate the address from the identifier in a pure manner? If we don't have this we can save storage and reduce the number of state reads and writes, which ultimately saves gas as well.

It's usage below, as a way to tell if the contract is already deployed, could be replaced by a call to EXTCODESIZE to check if it is nonzero. (stack overflow link). (IIUC, we are not susceptible to the trick here of the code size being zero during constructor execution because we deploy the contract from this factory)

nategraf · 2021-04-15T22:02:20Z

packages/protocol/contracts/identity/IdentityProxyHub.sol

+   * @return True if the given address is the likely owner of the given
+   * identifier, false otherwise.
+   */
+  function passesIdentityHeuristic(address addr, bytes32 identifier) public returns (bool) {


Suggested change

function passesIdentityHeuristic(address addr, bytes32 identifier) public returns (bool) {

function passesIdentityHeuristic(address addr, bytes32 identifier) public view returns (bool) {

nategraf · 2021-04-15T22:05:42Z

packages/protocol/contracts/identity/IdentityProxyHub.sol

+    IAttestations attestations = getAttestations();
+    (uint32 completed, uint32 requested) = attestations.getAttestationStats(identifier, addr);
+
+    bool hasEnoughCompletions = completed >= 3;


It would be nice to have the parameters of this heuristic be set as a contract field in the constructor. (Better still if it were immutable , but I notice we are not yet using Solidity ^0.6.5)

Is it possible to set these as constants in a solidity constructor? Would be great to avoid unnecessary storage reads in the proxy functionality.

So that's what immutable does IIUC. (i.e. immutable variables get writing into the deployed bytecode rather than being a storage variable)

If we can't use immutable we might still use a constant just to make it a bit more parameterized (even though it can't be set in the constructor)

nategraf · 2021-04-15T22:19:06Z

packages/protocol/contracts/identity/IdentityProxyHub.sol

+
+    bool completedOverHalfRequests = false;
+    if (completed > 0) {
+      completedOverHalfRequests = requested / completed < 2;


A UX consideration for having this be strictly less than rather than less than or equal is that if a user attempts verification once, and fails all three, they again and completes all three, they will not be able to pass this rule. We should compare the relative security of half vs half+1 and compare that to this UX tradeoff.

nategraf · 2021-04-15T22:25:42Z

packages/protocol/contracts/identity/IdentityProxyHub.sol

+        if (otherCompleted > completed) {
+          hasMostCompletions = false;
+          break;
+        }


Now for a totally unimportant suggestion

Suggested change

if (otherCompleted > completed) {

hasMostCompletions = false;

break;

}

hasMostCompletions = hasMostCompletions && otherCompleted <= completed

don't we want the short-circuit from the break?

In my opinion, no. We should optimize for the passing case, and I don't think we need to worry about gas usage in the failing case. The suggested change gets rid of a conditional (i.e. a JUMPI), replacing it with an AND, on every iteration to save a bit gas, and it makes the control flow a bit easier to reason about.

nategraf · 2021-04-15T22:28:08Z

packages/protocol/contracts/identity/IdentityProxyHub.sol

+
+    bool hasMostCompletions = true;
+    address[] memory addresses = attestations.lookupAccountsForIdentifier(identifier);
+    for (uint256 i = 0; i < addresses.length; i++) {


Is there a DoS vector here where an attacker fills up the account list for an identifier until this function cannot be executed?

it will always just run out of gas
isn't this the power of gas limit?

An attestations record isn't saved to storage until at least one attestation is completed by an address for a given identifier. So this DoS would only be possible for an attacker that can easily generate valid attestations - i.e. actually has control over the number or mass validator collusion.

nategraf · 2021-04-15T22:32:17Z

packages/protocol/contracts/identity/IdentityProxy.sol

+   * @dev This can only be called by the deployer of this contract, presumably
+   * the IdentityProxyHub after it checks the identity heuristic.
+   */
+  function makeCall(address destination, uint256 value, bytes calldata data) external onlyDeployer {


Should this be payable to allow passing value through? (Same question applies for IdentityProxyHub.makeCall)

Great catch, sending value was incorrectly implemented. Improved and added a test.

nategraf · 2021-04-15T22:34:59Z

packages/protocol/test/identity/identityproxy.ts

+const IdentityProxy: IdentityProxyContract = artifacts.require('IdentityProxy')
+const IdentityProxyTest: IdentityProxyTestContract = artifacts.require('IdentityProxyTest')
+
+contract('IdentityProxyHub', (accounts: string[]) => {


I notice this does not test sending calls with value 👀 Is that a feature we are not supporting here?

nategraf · 2021-04-15T22:41:22Z

packages/protocol/test/identity/identityproxyhub.ts

+    beforeEach(async () => {
+      address = await identityProxyHub.getIdentityProxy(identifier)
+      await identityProxyHub.getOrDeployIdentityProxy(identifier)
+    })


Having beforeEach here means the case where the relayer contract is deployed as part of makeCall is not tested. Testing each of the following cases with the relayer predeployed and not would be good.

I don't think it's worth the mess of redoing all tests both ways. Do you think just removing the call to getOrDeploy here would be enough? That way we're never relying on the proxy having been deployed beforehand. The function itself is tested by itself in some unit tests above.

I'd be happy with that solution 👍

yorhodes · 2021-04-19T16:31:40Z

packages/protocol/contracts/identity/IdentityProxyHub.sol

+
+    bool hasMostCompletions = true;
+    address[] memory addresses = attestations.lookupAccountsForIdentifier(identifier);
+    for (uint256 i = 0; i < addresses.length; i++) {


it will always just run out of gas
isn't this the power of gas limit?

yorhodes · 2021-04-19T16:32:29Z

packages/protocol/contracts/identity/IdentityProxyHub.sol

+        if (otherCompleted > completed) {
+          hasMostCompletions = false;
+          break;
+        }


don't we want the short-circuit from the break?

yorhodes · 2021-04-19T16:35:11Z

packages/protocol/contracts/common/Create2.sol

@@ -0,0 +1,25 @@
+pragma solidity ^0.5.13;
+
+library Create2 {


this is nice!

yorhodes · 2021-04-19T16:41:33Z

packages/protocol/contracts/common/Create2.sol

+    assembly {
+      deployedAddress := create2(0, add(initCode, 32), mload(initCode), salt)
+      if iszero(extcodesize(deployedAddress)) {
+        revert(0, 0)


can we revert with something more descriptive?
I'm not familiar with how the calling contract can consume this value

I tried to poke around assembly a bit, but didn't find a way that would be nice.

yorhodes

Exciting!

yorhodes · 2021-05-06T19:49:37Z

packages/protocol/test/identity/identityproxyhub.ts

+        await attestations.request(identifier, 0, '0x0', '0x0')
+        await attestations.request(identifier, 0, '0x0', '0x0')
+        await attestations.request(identifier, 0, '0x0', '0x0')
+        await attestations.request(identifier, 0, '0x0', '0x0')
+        await attestations.request(identifier, 0, '0x0', '0x0')


we could dedupe these easily but nbd for now

m-chrzan added 2 commits March 6, 2021 18:06

Add a light identity proxy

44e9dbe

Implement an identity proxy hub

b5617f1

nategraf reviewed Mar 9, 2021

View reviewed changes

yorhodes reviewed Mar 9, 2021

View reviewed changes

m-chrzan added 10 commits March 20, 2021 17:19

Use CREATE2 for deterministic deployments

75cd011

Merge branch 'master' into m-chrzan/identity-proxy

dad8208

Appease the linter

60893ce

Merge branch 'master' into m-chrzan/identity-proxy

a24f69c

Move ts-ignore after prettification

d09433c

Merge branch 'master' into m-chrzan/identity-proxy

4b41126

Add clarifying comment

6ced874

Implement identity heuristic

aab18b8

Add natspecs

5f00132

Merge branch 'master' into m-chrzan/identity-proxy

ab791e3

m-chrzan marked this pull request as ready for review April 15, 2021 07:37

m-chrzan requested a review from a team April 15, 2021 07:37

nategraf reviewed Apr 15, 2021

View reviewed changes

m-chrzan added 2 commits April 19, 2021 15:08

Merge branch 'master' into m-chrzan/identity-proxy

2b78c4c

Reduce storage use

c823b53

yorhodes reviewed Apr 19, 2021

View reviewed changes

m-chrzan added 5 commits April 21, 2021 18:08

Make function view

732b6e7

Send msg.value with calls

189ae8e

Don't predeploy proxy before unit tests

7959829

Simplify most completions logic

32183db

Merge branch 'master' into m-chrzan/identity-proxy

d037427

yorhodes mentioned this pull request Apr 26, 2021

Celo Core Contracts Audit 5 #7519

Closed

Merge branch 'master' into m-chrzan/identity-proxy

7f1798e

m-chrzan added 2 commits May 4, 2021 18:17

Silence weird linter errors

b542b04

Merge branch 'master' into m-chrzan/identity-proxy

9cc3531

yorhodes approved these changes May 6, 2021

View reviewed changes

m-chrzan added the automerge Have PR merge automatically when checks pass label May 6, 2021

Merge branch 'master' into m-chrzan/identity-proxy

ad1ac7b

mergify bot merged commit 5efbc25 into master May 6, 2021

mergify bot deleted the m-chrzan/identity-proxy branch May 6, 2021 21:53

nategraf mentioned this pull request May 27, 2021

[N04] Simplify identity heuristic calculation #7998

Closed


		import "../common/ExternalCall.sol";

		contract IdentityProxy {

	function passesIdentityHeuristic(address addr, bytes32 identifier) public returns (bool) {
	function passesIdentityHeuristic(address addr, bytes32 identifier) public view returns (bool) {

Implement identity proxies #7363

Implement identity proxies #7363

Conversation

m-chrzan commented Mar 8, 2021 • edited Loading

Description

Tested

Related issues

Backwards compatibility

nategraf 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nategraf Mar 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nategraf commented Mar 10, 2021 • edited Loading

m-chrzan commented Mar 10, 2021 • edited Loading

m-chrzan commented Apr 15, 2021

nategraf 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

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

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

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

yorhodes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-chrzan commented Mar 8, 2021 •

edited

Loading

nategraf Mar 9, 2021 •

edited

Loading

nategraf commented Mar 10, 2021 •

edited

Loading

m-chrzan commented Mar 10, 2021 •

edited

Loading