Feature/146382715 metatx controller #38

naterush · 2017-06-15T20:14:15Z

This contains the first attempt at a singleton meta-tx relay contract. An overview, the design decisions, attacks, and thoughts about this can be found on this document: https://consensys.quip.com/CoClAjSTyBZI

Tests are very incomplete -- as of now, just test to make sure it works but not very thorough. Will be adding more in the coming days.

Would love some basic feedback on what exists so far.

…ownership Also don’t allow recoveryKey to overwrite existing owners

Fails due to various timestamp related issues

Added tests for migration. Would love some feedback on them :~)

This stops the following attack: Assume there is a single evil owner and any number of good owners. 1. EO calls changeRecovery and changes the recovery to 0. 2. EO then calls forwardTo, and sends data through the proxy to the registerIdentity function in the IdentityManager. 3. In this forward, the EO provides a new ownerKey and a recoveryKey of 0 to the registerIdentity function. Thus, the conditional in the first line is false, and this does not throw. 4. The new owner is then added immediately with full power. 5. Thus, the evilOwner could effectively add a ton of new evil owners and overpower the good identity.

oed · 2017-06-19T16:40:23Z

What is the reason for having a separate MetaTx contract? Wouldn't it make sense to put the metaTx verification directly into the MetaIdentityManager?
Another alternative is to have an abstract contract that the MetaIdentityManager inherits.
Anyway having yet another contract in the proxy call stack seems like bloat to me.

naterush · 2017-06-19T17:12:04Z

@oed This is originally what @christianlundkvist did here: https://github.com/uport-project/uport-identity/blob/feature/identity-manager-metatx/contracts/IdentityManager.sol

That specific approach led to a lot of duplicated code, but I think you are right. Moving these functions into the MetaIdenitityManager itself seems like a good idea.

naterush · 2017-06-19T17:14:36Z

It's worth noting that a benefit of separating this into a separate contract is that it becomes a "singleton" meta-tx relay. All contracts that conform to the changes necessary for the relay (see here in the section "Changes to Existing Contracts:") are able to interact with it without having to add it to their code. This approach is likely very similar to an abstract contract, as you mentioned.

oed · 2017-07-07T11:04:51Z

contracts/MetaIdentityManager.sol

+  /// @param owner Key who can use this contract to control proxy. Given full power
+  /// @param recoveryKey Key of recovery network or address from seed to recovery proxy
+  /// Gas cost of ~300,000
+  function CreateIdentity(address owner, address recoveryKey) validRecovery(recoveryKey) {


This method should start with a small caps letter.

@oed will update this now.

naterush · 2017-07-07T16:15:03Z

@oed These are good points, thanks. I'll work on doing this now. Any thoughts on what version of solc to go to? :)

oed · 2017-07-07T16:29:55Z

@naterush The version supported by the latest truffle might be a good idea. Make sure to specify the truffle version without ^ so that we don't run in to errors due to this in the future.

coder5876 · 2017-07-11T15:50:22Z

These changes look good to me, I.e. name change, updated solc and dependencies. 👍

Finished. Updated solc, and renamed.

aldigjo · 2017-07-11T16:15:47Z

contracts/TxRelay.sol

+   * @param sigV, sigR, sigS ECDSA signature on some data to be forwarded
+   * @param data The bytes necessary to call the function in the destination contract.
+                 Note, the first encoded argument in data must be address of the signer
+   * @param claimedSender Address of the user who is having tx forwarded


Does this need to be updated since it doesn't seem to be a direct Param?

@aldigjo good catch. Will update now.

oed · 2017-07-13T15:04:49Z

contracts/IdentityManager.sol

+  //Checks that address a is the first input in msg.data.
+  //Has very minimal gas overhead.
+  function checkMessageData(address a) constant internal returns (bool t) {
+    if (msg.data.length < 36) return false;


Is this check really needed?

Won't function calls fail if they don't match function signature?

Won't the eq test return false (or throw?) if msg.data.lenth < 36?

I thought so also, but I had a test case that was failing b/c of this, somehow. Mabe should try to replicate, but I guess this does not really hurt.

hm, all tests work for me without that line. The only reason I ask is because solidity-coverage says that that line isn't covered, and I can't figure out a way to test it. But I guess it's fine to leave it there.

@naterush what was the original reason for the 36 magic number?

@oed anything else you feel need to be addressed? I'm eager to merge this one in.

@christianlundkvist nope, let's merge :)

@oed @christianlundkvist requires one more review, but it is probably good to get this out, so we can start testing.

oed · 2017-07-18T12:02:49Z

It might be a good idea to add functions for isOwner and isRecovery as we currently have no way of checking this without trying to call another function and see if it fails.

naterush · 2017-07-18T23:07:37Z

@oed happy to add when free on Thursday/Friday. Otherwise, if you want to merge sooner, feel free to add :~)

coder5876 · 2017-07-20T21:52:38Z

@oed @naterush I think it might also be good to have a mapping/function that takes a recovery key and returns the corresponding identity.

Also, the way we're doing actual recovery right now might open up an attack:

If I create a new identity with someone elses recoveryKey (that I don't control), then you might be able to block the recovery from that person.

naterush · 2017-07-20T23:54:54Z

contracts/IdentityManager.sol

@@ -208,4 +208,12 @@ contract IdentityManager {
        t := eq(a, and(mask, calldataload(4)))
    }
  }
+
+  function isOwner(address identity, address user) constant returns (bool) {


User might be a little misleading langauge @oed. Maybe key (as below) would be better?

yeah, maybe change to owner and recoveryKey in the isRecovery function. Then it is consistant with everywhere else.

naterush · 2017-07-20T23:55:44Z

@christianlundkvist Currently, the mapping is from identity => recoveryKey. This helper function would add an extra mapping (extra cost and thing to update, not sure it is worth it). Let me know what you think.

Also, not sure I follow the attack you describe. As the mapping is from identity => recovery key, couldn't multiple identities have the same recovery with no issue?

pelle

@naterush @oed we now need a separate IdentityManager for metaTx's so we can deploy them separately.

pelle

Really good. The meta tx code is pretty clear and isolated. So all we need is a separate MetaTxIdentityManager contract now and we should be good to go.

pelle · 2017-07-28T23:37:53Z

contracts/TxRelay.sol

+    bytes32 h = sha3(this, nonce[claimedSender], destination, data, msg.sender);
+    address addressFromSig = ecrecover(h, sigV, sigR, sigS);
+
+    nonce[claimedSender]++;


@naterush I think it does make sense to move it below just for clarity.

pelle · 2017-07-28T23:43:29Z

contracts/TxRelay.sol

+    if (b.length < 36) return address(0);
+    assembly {
+        let mask := 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF
+        a := and(mask, mload(add(b, 36)))


pelle · 2017-07-28T23:47:46Z

contracts/IdentityManager.sol

+import "./Proxy.sol";
+
+
+contract IdentityManager {


@naterush @oed As I mentioned we need a new contract here. This may actually be a good candidate for inheritance as with the design here the only thing different is the constructor, onlyAuthorized and checkMessageData. That would also ensure that changes to the normal IdentityManager would get reflected here automatically.

I can rework this over the weekend. For a name, I think MetaIdentityManager works well.

Also, you mean just define a "ManagerInterface," right? If we do inheritance, I'm not sure we can just add modifiers on to functions (aka, would have to redefine functions anyway).

I do think it would make sense to make sure they share the same interface, though. Let me know what you think @pelle

naterush · 2017-07-29T03:45:36Z

@pelle thanks. Will update this weekend. For the "I think it does make sense to move it below just for clarity," which line are you referring to?

pelle · 2017-07-29T21:13:52Z

contracts/TxRelay.sol

+    //Validity Checks (if these throw this time, they will always throw)
+    if (claimedSender != addressFromSig) throw;
+    if (!checkAddress(data, addressFromSig)) throw;
+


@naterush move line 41 down here

Again just for clarity. I know the functionality is the same.

pelle · 2017-08-02T00:29:55Z

Consolidated and replaced by #45 and #17

pelle and others added 27 commits May 24, 2017 20:01

IdentitManager which creates identities and acts as a controller

5dfc653

Adding of multiple devices as well as a change by recoveryKey

32380cd

tests for removal of owners

f4ec8d6

tests for changeRecovery

048611e

only allow owners who have been around for more than a day to modify …

4a6da21

…ownership Also don’t allow recoveryKey to overwrite existing owners

Reorganizing stuff based on latest changes in repo

d289ce1

added a general purpose rate limiter

9d4fb31

add a rate limiter to IdentityManager

b5f1771

Also limit owners added by owners

09ab55c

Add registerIdentity function for registering an existing proxy

875ed99

beginning of reorganizing tests

ce9ae06

Fails due to various timestamp related issues

set explicit 0.4.8 pragma

03a40ff

Configurable time locks

03a546d

Pushing refactored tests. Still failing.

9f8767e

Fix time lock calculations and rate limiter

61097de

Add remaining tests

eca4abc

Add function to migrate IdentityManager.

3ef76aa

Migration: added events, fixed bug, and cleaned up

e581692

Migration Tests

fd749c0

Added tests for migration. Would love some feedback on them :~)

Fixed ref to recovery quorum in indes.js

ef6112b

Bumped version

be34616

Add eth-lightwallet to packages

998175a

Add meta-tx contracts

6f07f58

Add migration and update index

3dfb959

Add basic relay tests

d71120d

Merge branch 'develop' into feature/146382715-metatx-controller

cc35bc5

oed previously requested changes Jul 7, 2017

View reviewed changes

naterush added 5 commits July 7, 2017 12:48

change MetaIdentity -> Identity. Create -> create

573d34c

update artifacts

96e4d88

update solc, truffle, tests

7b6ed37

Merge branch 'develop' into feature/146382715-metatx-controller

cda3b71

update dependencies, small linting fixes.

4a95c63

naterush force-pushed the feature/146382715-metatx-controller branch from 8329461 to 4a95c63 Compare July 11, 2017 15:17

aldigjo reviewed Jul 11, 2017

View reviewed changes

udpate comments

6010aca

oed reviewed Jul 13, 2017

View reviewed changes

oed mentioned this pull request Jul 14, 2017

Added coverage report #43

Merged

coder5876 mentioned this pull request Jul 14, 2017

IdentityManager - Identity factory with multiple devices and singleton controller support #17

Merged

naterush commented Jul 20, 2017

View reviewed changes

Added isOwner and isRecovery functions to IdentityManager

0f15d2c

oed force-pushed the feature/146382715-metatx-controller branch from 3de6310 to 0f15d2c Compare July 21, 2017 12:12

pelle suggested changes Jul 28, 2017

View reviewed changes

pelle reviewed Jul 29, 2017

View reviewed changes

pelle closed this Aug 2, 2017

naterush deleted the feature/146382715-metatx-controller branch November 15, 2017 22:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/146382715 metatx controller #38

Feature/146382715 metatx controller #38

naterush commented Jun 15, 2017

oed commented Jun 19, 2017

naterush commented Jun 19, 2017

naterush commented Jun 19, 2017 •

edited

Loading

oed Jul 7, 2017

naterush Jul 7, 2017

naterush commented Jul 7, 2017

oed commented Jul 7, 2017

coder5876 commented Jul 11, 2017

aldigjo Jul 11, 2017

naterush Jul 11, 2017

oed Jul 13, 2017

naterush Jul 14, 2017

oed Jul 14, 2017

coder5876 Jul 14, 2017

oed Jul 14, 2017

naterush Jul 16, 2017

oed commented Jul 18, 2017

naterush commented Jul 18, 2017

coder5876 commented Jul 20, 2017

naterush Jul 20, 2017

oed Jul 21, 2017

naterush commented Jul 20, 2017

pelle left a comment

pelle left a comment

pelle Jul 28, 2017

pelle Jul 28, 2017

pelle Jul 28, 2017

naterush Jul 29, 2017

naterush commented Jul 29, 2017

pelle Jul 29, 2017

pelle Jul 29, 2017

pelle commented Aug 2, 2017

Feature/146382715 metatx controller #38

Feature/146382715 metatx controller #38

Conversation

naterush commented Jun 15, 2017

oed commented Jun 19, 2017

naterush commented Jun 19, 2017

naterush commented Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naterush commented Jul 7, 2017

oed commented Jul 7, 2017

coder5876 commented Jul 11, 2017

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

oed commented Jul 18, 2017

naterush commented Jul 18, 2017

coder5876 commented Jul 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naterush commented Jul 20, 2017

pelle left a comment

Choose a reason for hiding this comment

pelle 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

naterush commented Jul 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pelle commented Aug 2, 2017

naterush commented Jun 19, 2017 •

edited

Loading