-
Notifications
You must be signed in to change notification settings - Fork 69
IdentityManager - Identity factory with multiple devices and singleton controller support #17
Conversation
c2c9cb9
to
e9c749c
Compare
I rebased it and changed a few things. I need to update the tests more and add tests for the different attack scenarios |
We would probably also want a factory function that creates a controller to an already existing proxy contract. That would make it much easier to switch from using an older controller. |
Good idea @oed |
With rateLimited, it limits an existing owner to adding a single owner within an hour. But then that owner can add another owner immediately. Essentially, owner1 could add owner2 who could add owner3... w/ no delay. I think this could be a way to avoid rateLimited and add unlimited owners with no time lock. This could be fixed by added a line limiter[newOwner] = now; in the addOwner function. |
dbc7fb8
to
4f32a81
Compare
Just a small note: Can you change the pragma solidity to 0.4.8, without the |
…ownership Also don’t allow recoveryKey to overwrite existing owners
Fails due to various timestamp related issues
153e5de
to
eca4abc
Compare
@pelle I wonder if there is a benefit to having a distinction between onlyOwner and onlyOlderOwner now that there is a rate limiter. While it allows for different time locks for 1) using proxy, and 2) adding/removing other owners, but I'm not sure this is a distinction worth making. Something we've discussed is that added devices should be able to transact immediately for UX reasons -- which I think makes sense. Thus, it might make sense to remove onlyOlderOwner completely, and just have an onlyOwner modifier, and just use the rateLimiter to control access to addOwner, removerOwner, etc. |
@naterush @christianlundkvist I do feel that it is a good idea to limit admin like funcitonality to older users. I also think there should be some sort of small limit for new owners as I think it's safer, but we can configure it to 0 in the constructor if we feel it's ok. |
@christianlundkvist yes, this PR is no longer needed. |
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.
I can't Request Changes since I'm the original author. @oed see my comments. Small check needed. Otherwise everything looks great.
contracts/IdentityManager.sol
Outdated
@@ -83,6 +83,7 @@ contract IdentityManager { | |||
// Factory function | |||
// gas 289,311 | |||
function CreateIdentity(address owner, address recoveryKey) { | |||
if (recoveryKey == address(0)) throw; |
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.
I didn't know about this address(0)
. Nice.
contracts/IdentityManager.sol
Outdated
} | ||
|
||
// an owner can migrate away to a new IdentityManager | ||
function initiateMigration(Proxy identity, address newIdManager) onlyOlderOwner(identity) { |
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.
@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.
contracts/IdentityManager.sol
Outdated
// 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 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
contracts/IdentityManager.sol
Outdated
@@ -133,6 +133,7 @@ contract IdentityManager { | |||
|
|||
// an owner can migrate away to a new IdentityManager | |||
function initiateMigration(Proxy identity, address newIdManager) onlyOlderOwner(identity) { | |||
if (newIdManager == address(0)) throw; |
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.
@oed could use the validAddress modifier here also.
contracts/IdentityManager.sol
Outdated
|
||
/// @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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
good point!
contracts/IdentityManager.sol
Outdated
/// @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 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.
contracts/IdentityManager.sol
Outdated
|
||
/// @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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
yes, thanks!
contracts/IdentityManager.sol
Outdated
/// @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 comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe delete here also
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.
Changes look great so far
contracts/IdentityManager.sol
Outdated
// - 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 | ||
modifier validAddress(address addr) { //protects against some weird attacks |
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
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.
Looks good except for the possible nuisance attack and possible mitigation (didn't think of this one before).
/// @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 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.
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.
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 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.
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.
@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 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.
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.
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 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.
@pelle see comments above |
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.
If we mitigate the nuisance attack by having the recovery key update the registry we should be good to go.
I'm working on getting tests working, and I don't want to merge w/out doing so, but can we change the isOwner function? It's a one liner but could save us from some headaches later. Essentially, as of now, it may return that someone is an owner when they are not. To fix this, we just have to change the function to say: return (owners[identity][owner] > 0 && (owners[identity][owner] + userTimeLock) <= now ); This way it will match with the modifier isOwner. After that, let's merge :) Note: edited after posting to change msg.sender -> owner. |
I had to do some funky stuff updating truffle wallet provider, etc. Not exactly sure what the issue was, but I think this should be good to merge :). |
Deployed on Rinkeby + Kovan. Updated compiler version for latest version of Truffle |
Creating an identity is much cheaper than in
IdentityFactory
~289,311 vs ~2,400,000 gas
Original idea inspired by @narush. It no longer creates a controller per identity but uses a shared controller implemented in the contract itself.
It also supports adding multiple devices per identity and simplfied recovery rules