-
Notifications
You must be signed in to change notification settings - Fork 68
IdentityManager - Identity factory with multiple devices and singleton controller support #17
Changes from 29 commits
5dfc653
32380cd
f4ec8d6
048611e
4a6da21
d289ce1
9d4fb31
b5f1771
09ab55c
875ed99
ce9ae06
03a40ff
03a546d
9f8767e
61097de
eca4abc
3ef76aa
e581692
fd749c0
4f84f01
5346800
e2e6420
b266633
8b4d35a
91fb8bf
5dd749a
e8cc4e0
44a7778
d3dab73
200c794
f762602
2f96523
4bbf91e
0c5d59b
e528dd3
f5ff249
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 One way to mitigate this is to add a mapping
that maps a recoveryKey to an identity, and when creating a new identity we include a check that There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@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 (owners[msg.sender][owner] > 0 || recoveryKeys[msg.sender] > 0 ) throw; // Deny any funny business | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to have another event here, like |
||
} | ||
|
||
/// @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 (owners[identity][newOwner] > 0) throw; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could be if (isOwner(identity, newOwner)) throw; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good point! |
||
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) { | ||
owners[identity][owner] = 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe use delete keyword for clarity? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, thanks! |
||
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]; | ||
migrationInitiated[identity] = 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe delete here also |
||
migrationNewAddress[identity] = 0; | ||
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Explanatory comment on my above comment. If |
||
address newIdManager = migrationNewAddress[identity]; | ||
migrationInitiated[identity] = 0; | ||
migrationNewAddress[identity] = 0; | ||
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; | ||
} | ||
} | ||
|
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) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome