Skip to content
This repository has been archived by the owner on Apr 15, 2020. It is now read-only.

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

Merged
merged 36 commits into from
Jul 28, 2017
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
5dfc653
IdentitManager which creates identities and acts as a controller
pelle Apr 17, 2017
32380cd
Adding of multiple devices as well as a change by recoveryKey
pelle Apr 17, 2017
f4ec8d6
tests for removal of owners
pelle Apr 17, 2017
048611e
tests for changeRecovery
pelle Apr 17, 2017
4a6da21
only allow owners who have been around for more than a day to modify …
pelle Apr 21, 2017
d289ce1
Reorganizing stuff based on latest changes in repo
pelle Apr 22, 2017
9d4fb31
added a general purpose rate limiter
pelle May 18, 2017
b5f1771
add a rate limiter to IdentityManager
pelle May 18, 2017
09ab55c
Also limit owners added by owners
pelle May 19, 2017
875ed99
Add registerIdentity function for registering an existing proxy
pelle May 19, 2017
ce9ae06
beginning of reorganizing tests
pelle May 19, 2017
03a40ff
set explicit 0.4.8 pragma
pelle May 24, 2017
03a546d
Configurable time locks
pelle May 24, 2017
9f8767e
Pushing refactored tests. Still failing.
pelle May 24, 2017
61097de
Fix time lock calculations and rate limiter
pelle May 25, 2017
eca4abc
Add remaining tests
pelle May 25, 2017
3ef76aa
Add function to migrate IdentityManager.
coder5876 Jun 5, 2017
e581692
Migration: added events, fixed bug, and cleaned up
naterush Jun 7, 2017
fd749c0
Migration Tests
naterush Jun 8, 2017
4f84f01
Stopped recoveryKey from ever equaling zero.
naterush Jun 8, 2017
5346800
Small cleanup
oed Jun 21, 2017
e2e6420
Merge branch 'develop' into feature/identity-manager
oed Jun 21, 2017
b266633
Tests foro IdentityManager now uses async await
oed Jun 21, 2017
8b4d35a
Use assert.match for errors
oed Jun 22, 2017
91fb8bf
Zero address check on initiateMigration
oed Jul 21, 2017
5dd749a
Updated contract docstrings
oed Jul 21, 2017
e8cc4e0
Moved address check to modifier
oed Jul 21, 2017
44a7778
Owner added by owner can now transact directly after being added
oed Jul 21, 2017
d3dab73
IdentityManager now has isOwner and isRecovery functions
oed Jul 21, 2017
200c794
Some small fixes
oed Jul 21, 2017
f762602
minor change to isOwner
naterush Jul 25, 2017
2f96523
Deployed IdentityFactory on Kovan+Rinkeby
coder5876 Jul 27, 2017
4bbf91e
Updated truffle version
coder5876 Jul 27, 2017
0c5d59b
update circle.yml in the right branch
Jul 27, 2017
e528dd3
Update tests for updated truffle.
coder5876 Jul 28, 2017
f5ff249
Merge branch 'develop' into feature/identity-manager
pelle Jul 28, 2017
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
162 changes: 162 additions & 0 deletions contracts/IdentityManager.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,162 @@
pragma solidity 0.4.8;
import "./Proxy.sol";

contract IdentityManager {
uint adminTimeLock;
uint userTimeLock;
uint adminRate;

event IdentityCreated(
address indexed identity,
address indexed creator,
address owner,
address indexed recoveryKey);

event OwnerAdded(
address indexed identity,
address indexed owner,
address instigator);

event OwnerRemoved(
address indexed identity,
address indexed owner,
address instigator);

event RecoveryChanged(
address indexed identity,
address indexed recoveryKey,
address instigator);

event MigrationInitiated(
address indexed identity,
address indexed newIdManager,
address instigator);

event MigrationCanceled(
address indexed identity,
address indexed newIdManager,
address instigator);

event MigrationFinalized(
address indexed identity,
address indexed newIdManager,
address instigator);

mapping(address => mapping(address => uint)) owners;
mapping(address => address) recoveryKeys;
mapping(address => mapping(address => uint)) limiter;
mapping(address => uint) migrationInitiated;
mapping(address => address) migrationNewAddress;

modifier onlyOwner(address identity) {
if (owners[identity][msg.sender] > 0 && (owners[identity][msg.sender] + userTimeLock) <= now ) _ ;
else throw;
}

modifier onlyOlderOwner(address identity) {
if (owners[identity][msg.sender] > 0 && (owners[identity][msg.sender] + adminTimeLock) <= now) _ ;
else throw;
}

modifier onlyRecovery(address identity) {
if (recoveryKeys[identity] == msg.sender) _ ;
else throw;
}

modifier rateLimited(Proxy identity) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is Proxy used here when address is used on the other modifiers?

if (limiter[identity][msg.sender] < (now - adminRate)) {
limiter[identity][msg.sender] = now;
_ ;
} else throw;
}

// Instantiate IdentityManager with the following limits:
// - userTimeLock - Time before new owner can control proxy
// - adminTimeLock - Time before new owner can add/remove owners
// - adminRate - Time period used for rate limiting a given key for admin functionality
function IdentityManager(uint _userTimeLock, uint _adminTimeLock, uint _adminRate) {
adminTimeLock = _adminTimeLock;
userTimeLock = _userTimeLock;
adminRate = _adminRate;
}

// Factory function
// gas 289,311
function CreateIdentity(address owner, address recoveryKey) {
if (recoveryKey == address(0)) throw;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know about this address(0). Nice.

Proxy identity = new Proxy();
owners[identity][owner] = now - adminTimeLock; // This is to ensure original owner has full power from day one
recoveryKeys[identity] = recoveryKey;
IdentityCreated(identity, msg.sender, owner, recoveryKey);
}

// An identity Proxy can use this to register itself with the IdentityManager
// Note they also have to change the owner of the Proxy over to this, but after calling this
function registerIdentity(address owner, address recoveryKey) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oed @christianlundkvist this is what I wrote to do the transfer. Isn't this what you meant? We would still have to do the actual transfer of the proxy as a second transaction. But that would just use forwardTo I thought.

Copy link
Contributor

@coder5876 coder5876 May 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The transfer of the ownership of the proxy needs to use a special function in the proxy contract:

function transfer(address _owner) onlyOwner { owner = _owner; }

so no way to use forwardTo for this. I.e. this needs a new transaction in the IdentityManager.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@christianlundkvist You mean for transfering an proxy away from the IdentityManager?
To transfer to the IdentityManager this function works fine given that you call transfer(<address of IM>) from your old controller after you call this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But yes, we need a function to transfer the identity away from the IdentityManager

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok I'll have a look @christianlundkvist

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oed Yep what I meant was we need a function in the IdentityManager to transfer a proxy to another IdentityManager.

if (recoveryKey == address(0)) throw;
if (owners[msg.sender][owner] > 0 || recoveryKeys[msg.sender] > 0 ) throw; // Deny any funny business
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First check here is not needed. As we enforce the recoveryKey invariant, this is not an issue.

owners[msg.sender][owner] = now - adminTimeLock; // This is to ensure original owner has full power from day one
recoveryKeys[msg.sender] = recoveryKey;
IdentityCreated(msg.sender, msg.sender, owner, recoveryKey);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to have another event here, like IndentityRegistered? I don't have very strong feelings either way, but it's probably good to distinguish between these events for logging/analytics purposes.

}

// Primary forward function
function forwardTo(Proxy identity, address destination, uint value, bytes data) onlyOwner(identity) {
identity.forward(destination, value, data);
}

// an owner can add a new device instantly
function addOwner(Proxy identity, address newOwner) onlyOlderOwner(identity) rateLimited(identity) {
owners[identity][newOwner] = now;
OwnerAdded(identity, newOwner, msg.sender);
}

// a recovery key owner can add a new device with 1 days wait time
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update comment to mention adminRate rather than "1 day".

function addOwnerForRecovery(Proxy identity, address newOwner) onlyRecovery(identity) rateLimited(identity) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name addOwnerForRecovery is a bit unclear what it means. It sounds like we are adding an owner that is to be used as a recovery. How about addOwnerByRecovery or addOwnerUsingRecovery or similar?

if (owners[identity][newOwner] > 0) throw;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be if (isOwner(identity, newOwner)) throw;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point!

owners[identity][newOwner] = now;
OwnerAdded(identity, newOwner, msg.sender);
}

// an owner can remove another owner instantly
function removeOwner(Proxy identity, address owner) onlyOlderOwner(identity) rateLimited(identity) {
owners[identity][owner] = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use delete keyword for clarity?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, thanks!

OwnerRemoved(identity, owner, msg.sender);
}

// an owner can add change the recoverykey whenever they want to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"add change the recoverykey" -> "add or change the recoverykey"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"add change the recoverykey" -> "add or change the recoverykey"

function changeRecovery(Proxy identity, address recoveryKey) onlyOlderOwner(identity) rateLimited(identity) {
if (recoveryKey == address(0)) throw;
recoveryKeys[identity] = recoveryKey;
RecoveryChanged(identity, recoveryKey, msg.sender);
}

// an owner can migrate away to a new IdentityManager
function initiateMigration(Proxy identity, address newIdManager) onlyOlderOwner(identity) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oed Based on the other changes checking for null recovery keys, we should do the same here. Basically if newIdManager is null it should fail.

migrationInitiated[identity] = now;
migrationNewAddress[identity] = newIdManager;
MigrationInitiated(identity, newIdManager, msg.sender);
}

// any owner can cancel a migration
function cancelMigration(Proxy identity) onlyOwner(identity) {
address canceledManager = migrationNewAddress[identity];
migrationInitiated[identity] = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe delete here also

migrationNewAddress[identity] = 0;
MigrationCanceled(identity, canceledManager, msg.sender);
}

// owner needs to finalize migration once adminTimeLock time has passed
// WARNING: before transfering to a new address, make sure this address is "ready to recieve" the proxy.
// Not doing so risks the proxy becoming stuck.
function finalizeMigration(Proxy identity) onlyOlderOwner(identity) {
if (migrationInitiated[identity] > 0 && migrationInitiated[identity] + adminTimeLock < now) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explanatory comment on my above comment. If migrationNewAddress[identity] == address(0) here we could loose an identity completely. But the right place to check for it is in initiateMigration

address newIdManager = migrationNewAddress[identity];
migrationInitiated[identity] = 0;
migrationNewAddress[identity] = 0;
identity.transfer(newIdManager);
MigrationFinalized(identity, newIdManager, msg.sender);
}
}
}

1 change: 1 addition & 0 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ module.exports = {
ArrayLib: require('./build/contracts/ArrayLib.json'),
IdentityFactory: require('./build/contracts/IdentityFactory.json'),
IdentityFactoryWithRecoveryKey: require('./build/contracts/IdentityFactoryWithRecoveryKey.json'),
IdentityManager: require('./build/contracts/IdentityManager.json'),
RecoverableController: require('./build/contracts/RecoverableController.json'),
Proxy: require('./build/contracts/Proxy.json'),
RecoveryQuorum: require('./build/contracts/Proxy.json'),
Expand Down
5 changes: 5 additions & 0 deletions migrations/3_deploy_identity_manager.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
const IdentityManager = artifacts.require('./IdentityManager.sol')

module.exports = function (deployer) {
deployer.deploy(IdentityManager)
}
Loading