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 30 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
183 changes: 183 additions & 0 deletions contracts/IdentityManager.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,183 @@
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(address identity) {
if (limiter[identity][msg.sender] < (now - adminRate)) {
limiter[identity][msg.sender] = now;
_ ;
} else throw;
}

modifier validAddress(address addr) { //protects against some weird attacks
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome

if (addr != address(0)) _;
else throw;
}

/// @dev Contract constructor sets initial timelock limits
/// @param _userTimeLock Time before new owner can control proxy
/// @param _adminTimeLock Time before new owner can add/remove owners
/// @param _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;
}

/// @dev Creates a new proxy contract for an owner and recovery
/// @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 289,311
function createIdentity(address owner, address recoveryKey) validAddress(recoveryKey) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There seems to be a possible nuisance attack here: Let's say I create a new identity with a recoveryKey that's used by someone else. This will then be stored here and in the registry, and when Vishnu is indexing the search will pull up two identities with this recoveryKey.

One way to mitigate this is to add a mapping

mapping(address => address) public recoveryKeyToIdentity;

that maps a recoveryKey to an identity, and when creating a new identity we include a check that recoveryKeyToIdentity[recoveryKey] is zero. This will also allow us to resolve the proxy address given a recoveryKey without using Vishnu.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think there is an issue about us not knowing where to look for the key. The mobile app needs to keep track of the IdentityManager address in order to be able to send transactions anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

@christianlundkvist what would be the motivation for searching based on recoveryKey? It seems unlikely to me that a recovery key would ever say wonder what it is the recovery for, given that the recovery key will likely be the owner of the identity themselves (with SSS).

Also, Vishnu could just index the first one. This does open the system up to very mild front running attacks, but it seems like it is a very mild issue.

I'm not sure it is worth the extra storage place. Not sure though. Let me know what you think.

Copy link
Contributor

Choose a reason for hiding this comment

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

@christianlundkvist If we decide to do this, should we do the same for owners as well? It seems like the same nuisance attack may be possible in this case (though the reasons for going to Vishnu is even more questionable)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@naterush @christianlundkvist the real case that we're currently using vishnu for is I have a recovery seed and I want to look up my uPort address from which I can now get my Identity Manager as well. If a user knows his uport address this is not an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The proposed solution to nuisance attack wouldn't do much good with multiple identity managers anyway.

Perhaps the real solution is to have the recoveryAddress itself record in the registry which address they are recovering for.

Copy link
Contributor

@coder5876 coder5876 Jul 24, 2017

Choose a reason for hiding this comment

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

Perhaps the real solution is to have the recoveryAddress itself record in the registry which address they are recovering for.

@pelle Yeah this would definitely work, I like this solution.

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);
}

/// @dev Allows a user to transfer control of existing proxy to this contract. Must come through proxy
/// @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
/// Note: User must change owner of proxy to this contract after calling this
function registerIdentity(address owner, address recoveryKey) validAddress(recoveryKey) {
if (recoveryKeys[msg.sender] > 0) throw; // Deny any funny business
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.

}

/// @dev Allows a user to forward a call through their proxy.
function forwardTo(Proxy identity, address destination, uint value, bytes data) onlyOwner(identity) {
identity.forward(destination, value, data);
}

/// @dev Allows an olderOwner to add a new owner instantly
function addOwner(Proxy identity, address newOwner) onlyOlderOwner(identity) rateLimited(identity) {
owners[identity][newOwner] = now - userTimeLock;
OwnerAdded(identity, newOwner, msg.sender);
}

/// @dev Allows a recoveryKey to add a new owner with userTimeLock waiting time
function addOwnerFromRecovery(Proxy identity, address newOwner) onlyRecovery(identity) rateLimited(identity) {
if (isOwner(identity, newOwner)) throw;
owners[identity][newOwner] = now;
OwnerAdded(identity, newOwner, msg.sender);
}

/// @dev Allows an owner to remove another owner instantly
function removeOwner(Proxy identity, address owner) onlyOlderOwner(identity) rateLimited(identity) {
delete owners[identity][owner];
OwnerRemoved(identity, owner, msg.sender);
}

/// @dev Allows an owner to change the recoveryKey instantly
function changeRecovery(Proxy identity, address recoveryKey)
onlyOlderOwner(identity)
rateLimited(identity)
validAddress(recoveryKey)
{
recoveryKeys[identity] = recoveryKey;
RecoveryChanged(identity, recoveryKey, msg.sender);
}

/// @dev Allows an owner to begin process of transfering proxy to new IdentityManager
function initiateMigration(Proxy identity, address newIdManager)
onlyOlderOwner(identity)
validAddress(newIdManager)
{
migrationInitiated[identity] = now;
migrationNewAddress[identity] = newIdManager;
MigrationInitiated(identity, newIdManager, msg.sender);
}

/// @dev Allows an owner to cancel the process of transfering proxy to new IdentityManager
function cancelMigration(Proxy identity) onlyOwner(identity) {
address canceledManager = migrationNewAddress[identity];
delete migrationInitiated[identity];
delete migrationNewAddress[identity];
MigrationCanceled(identity, canceledManager, msg.sender);
}

/// @dev Allows an owner 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];
delete migrationInitiated[identity];
delete migrationNewAddress[identity];
identity.transfer(newIdManager);
MigrationFinalized(identity, newIdManager, msg.sender);
}
}

function isOwner(address identity, address owner) constant returns (bool) {
return owners[identity][owner] != 0;
}

function isRecovery(address identity, address recoveryKey) constant returns (bool) {
return recoveryKeys[identity] == recoveryKey;
}
}

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/RecoveryQuorum.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