IdentityManager - Identity factory with multiple devices and singleton controller support #17
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -70,93 +70,114 @@ contract IdentityManager { | |
| } 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. 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. 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. @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. @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. @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. 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.
@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); | ||
|
There was a problem hiding this comment. 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 (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) { | ||
|
There was a problem hiding this comment. Explanatory comment on my above comment. If |
||
| 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; | ||
| } | ||
| } | ||
|
|
||
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