`MetaIdentityManager::addOwner()` can be called for existing owner

[jessicagmarshall]

uport-identity/contracts/MetaIdentityManager.sol

Lines 127 to 130 in 259435e

	function addOwner(address sender, Proxy identity, address newOwner) public
	onlyAuthorized
	onlyOlderOwner(identity, sender)
	rateLimited(identity, sender)

As either addOwner() function is written, a malicious olderOwner could call addOwner on an existing owner of owner or olderOwner status. This function call overwrites the time that existing owner became an owner in the owners mapping, and may be used as an attack to remove an older owner's admin privileges.

Recommendation

We recommend the addition of a require statement in both functions that ensures that addOwner() can only be called on an owner that is not a current owner.

[oed]

require(!isOlderOwner(identity, newOwner)) should be sufficient right? This allows an older owner to "upgrade" a pending owner to owner status.

[jessicagmarshall]

It would still be possible for an olderOwner to downgrade an almost olderOwner to the beginning of owner status with that requirement, no? I see what you're saying - I think I that's a security/utility design trade off that should be deferred to you.

[oed]

Oh, I see. Yes you are correct, let's use isOwner!