The keys that sign metatransactions are intended to be used for raw Ethereum transactions as well, especially when bypassing the relay altogether and sending transactions directly. Signed data can be rebroadcast as raw Ethereum transactions if the plaintext is formatted similarly. This should not be possible given the current metatransaction payload since the payload size differs from a raw transaction, but the safest option is to prefix the payload with data that will never be part of a valid transaction.

Recommendation

Consider using EIP 191 signed messages for metatransactions to guarantee protection from replaying as raw transactions.

replaceDelegates has a few loops including garbageCollect
(in RecoveryQuorum.sol#L47)

Since it's behavior has a wide range of gas consumption, the higher end usage should be documented. There should also be explicitly tests for "worst case" scenarios. A goal is to minimize the risk of the contract locking itself up, and understand when low block gas limits would start endangering the contract to be able to evaluate the severity and likelihood of such scenarios, and how large the margin of safety is.

The 'Should throw if sending zero ether' test actually tests for throws when sending negative ether.

Our initial review found a lack of testing documentation, without which we were forced to make inferences about what is correct and desired testing behavior. This is made difficult by the number of localhost test networks described in truffle.js. Upon discussion with the uPort team, we were directed to testing instructions within CONTRIBUTING.md,

Recommendation

We recommend a note in the README that directs the viewer to tests when linking to CONTRIBUTING.md.

Since proxy.sol is the highest priority, it should have much more tests, possibly even bordering on the paranoid.

Seems there should be tests like the following, as well as mixing combinations of them:

different owners, including an external account, owner is transferred to 0, owner is transferred to address of proxy itself, external owner transferred to a contract, a contract owner transferred to an external account
differentdestination like 0, the owner itself, the proxy itself
different value, including negative, and 1 wei
proxy's balance is <, =, and > than value
different data, including length 0, 1, 2, some intermediate length, some large length
...

Miners have ultimate control over transaction ordering and inclusion of transactions on the Ethereum Blockchain. This means that miners are ultimately able to decide which transactions are filled or canceled. Given this inherent power of miners it opens up a possible form of front running.

The IdentityManager and MetaIdentityManager contracts are susceptible to front running as the order in which owners and recoveryKeys are added and/or removed is very important in the case of an identity takeover attack by a malicious owner.

e.g. In the event that a malicious owner with olderOwner status calls removeOwner() on a benevolent owner in the same block that the benevolent owner calls removeOwner() on the malicious owner, the order in which these transactions are processed are very important.

adminTimeLock helps mitigate front running by preventing a malicious owner from gaining olderOwner status for a short period of time before front running becomes a potential issue. The rateLimited() modifier helps in that it restricts the number of administrative action an older owner may take, decreasing the chances of potentially conflicting transactions in the transaction pool.

Ref: Best Practices: Transaction-Ordering Dependence (TOD) / Front Running

Recommendation

We recommend that the values of userTimeLock, adminTimeLock, and adminRate be chosen with care so as to balance utility with security.

Especially in tests no more extra cruft with then()... and it's so much more pleasant to read and follow tests...
Here's an example (that may be old):
OpenZeppelin/openzeppelin-contracts#97

When an identity is migrated to another controller, the owner that sends the migrate command is cleared from the contract. However, any other owners remain associated with the identity. If the identity is ever migrated back to the same IdentityManager contract, the preexisting owners will still be present.

Recommendation

Update the documentation with a warning about this case. Consider adding a registration timestamp so owners with earlier timestamps can be ignored, but this scenario might not be worth the added complexity.

addDelegate appears to allow up to 15
(in RecoveryQuorum.sol#L75)

but the constructor has no limits on how many delegates can be added initially.

The 15 should also be a constant, instead of a magic number.

delete delegates[delegateAddresses[i]]; seems like it should work here:

(in RecoveryQuorum.sol#L94)

The generated bytecode from the compiler may also save some gas.

IdentityManager expects adminTimeLock to be longer than userTimeLock. This is not enforced. If set incorrectly, the identity creator will not be able to send transactions immediately. Owners added from recovery will receive admin privileges first, which they can use to add themselves as an owner again and receive transaction privileges.

Recommendation

Require adminTimeLock >= userTimeLock.

eg Should changeUserKey be private or internal (if it's only intended to be called by signUserChange) or is the intention for it to be public?

(in RecoveryQuorum.sol#iL34)

https://github.com/ConsenSys/smart-contract-best-practices#explicitly-mark-visibility-in-functions-and-state-variables

zmitton commented 22 days ago
@ethers this one is intended to be able to be called by anyone after a signature is provided (via signUserChange), and then the time period has passed

ethers commented 14 days ago
yes, explicitly using public makes the intent clearer.
(Generally missing code is harder to detect, and so a missing private or internal could cause insecurity in other cases.)

In the finalizeMigration() function of IdentityManager and MetaIdentityManager, the order and purpose of the delete statements are unclear.

Recommendation

Consider adding a rationalization of the delete statements to the documentation or as comments in the contract code to mitigate confusion.

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.

The 'can not remove other owner yet' test actually tests for not being able to add another owner yet.

Each Proxy contract specifies its owner as the single address that may call functions modified by isOwner(), which is more often than not the IdentityManager or MetaIdentityManager.

Within the IdentityManager and MetaIdentityManager, an identity's owners are defined as the whitelisted addresses which may have transactions to that identity's Proxy contract via either identity manager.

Though similar, these variables describe different things, and are confusing.

Recommendation

We recommend the renaming of the owner variable in the Proxy contract.

A very low severity solidity compiler issue in which it was possible to craft the name of a function such that it was executed instead of the fallback function in very specific circumstances was recently fixed in version 0.4.18 on October 18th.

As described in the Etherscan Soldity bug info table:

If a function has a selector consisting only of zeros, is payable and part of a contract that does not have a fallback function and at most five external functions in total, this function is called instead of the fallback function if Ether is sent to the contract without data.

Recommendation

None of these contracts seem to be affected by the ZeroFunctionSelector compiler bug. The benefits of jumping to a few week old 0.4.18 compiler do not seem to outweigh the risks of potential compiler bugs introduced in 0.4.18.

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.

Use the gas opcode as found by @GNSPS in safe-global/safe-smart-account#8

TxRelay.getAddress pulls the first argument to the function to be called and interprets it as an address. The code works fine, but in a comment, it explains:

//36 is the offset of the first param of the data, if encoded properly.
//4 bytes for the function signature, and 32 for the addess.

This is incorrect. The bytes array begins with 32 bytes for the length of the array, then 4 bytes for the function signature, followed by the first argument of the function. It's worth elaborating that this only works if the function being called looks like MetaIdentityManager.forwardTo(address sender, Proxy identity, address destination, uint value, bytes data).

Recommendation

Update the comment with the correct explanation. Consider adding an explanation that the function expects something like forwardTo to be called.

eg looks like collectedSignatures and neededSignatures should be constant

(in RecoveryQuorum.sol#L57)

zmitton commented 22 days ago
ethers I can make this change. I'm unaware what the difference is, in terms of bytecode. I know the API from web3 is slightly different, but no one has been able to tell me the real difference

ethers commented 14 days ago
Currently, it helps web3js frontends ensure they use an eth_call and get the return value instead of a transaction hash

uport-identity was using an interesting implementation of Meta transactions in the TxRelay contract.
Is a similar approach also used in more up to date libraries of uPort?

eg
pragma solidity 0.4.4;
instead of
pragma solidity ^0.4.4;

Helps prevent compiling and thus testing and deploying with unintended or mismatching compiler versions (like tests done with one version, and deployment is with another version).

Also, some files have a different pragma from other files and all files should use the compiler version unless there's a good reason and clear documentation and explanation.

fix the pragma
ifOwner seems like it can be removed
isOwner constant

• Send a meta-tx

A diagram that explains the control flow of a meta transaction refers to a verifySignature() function does not exist in the TxRelay contract. While its purpose may be inferred from the diagram, in the interest of exactness, verification of the meta transaction actually occurs as a require statement within the relayMetaTx() function in the TxRelay contract.

Recommendation

• update the documentation to reflect the require() statement

There are many ways in which the contract system could be misused, either by user error, or as the result of social engineering.

Protecting the user against all known ways they can change their setup (with controller/quorum/userKey, etc) is not possible, and some of this protection is left to the front-end. However, there are scenarios where it is cheap in terms of gas, and obvious wrong choices that the smart SCS can enforce.

For example, changing the Proxy's owner to itself should could be prevented by slightly modifying the transfer function to:

function transfer(address _owner) onlyOwner {
	if (_owner == address(this)){
    	owner = _owner;
	}
}

The rest of the Ethereum ecosystem is unknown, but the smart contracts have relative knowledge about its own threat model, and it would be cheap to implement.

I should be able to fund a TxRelay so it can issue transactions that require ether.

Needs at least a comment, and using a constant is probably better too.

deletedAfter: 31536000000000

(in recoveryQuorum)

Miners have ultimate control over transaction ordering and inclusion of transactions on the Ethereum Blockchain. This means that miners are ultimately able to decide which transactions are filled or canceled. Given this inherent power of miners it opens up a possible form of front running.

The IdentityManager and MetaIdentityManager contracts are susceptible to front running as the order in which owners and recoveryKeys are added and/or removed is very important in the case of an identity takeover attack by a malicious owner.

e.g. In the event that a malicious owner with olderOwner status calls removeOwner() on a benevolent owner in the same block that the benevolent owner calls removeOwner() on the malicious owner, the order in which these transactions are processed are very important.

adminTimeLock helps mitigate front running by preventing a malicious owner from gaining olderOwner status for a short period of time before front running becomes a potential issue. The rateLimited() modifier helps in that it restricts the number of administrative action an older owner may take, decreasing the chances of potentially conflicting transactions in the transaction pool.

Ref: Best Practices: Transaction-Ordering Dependence (TOD) / Front Running

Recommendation

We recommend that the values of userTimeLock, adminTimeLock, and adminRate be chosen with care so as to balance utility with security.

This is to raise awareness that the timeLocks (longTimeLock and shortTimeLock) are "configurable" and can cause overflow with pendingUntils:

• RecoverableController.sol#L41
• RecoverableController.sol#L53
• RecoverableController.sol#L65

The impact of an overflow here is that a "signed key change" could be made permanent (does not expire). This is to raise awareness because I don't see any immediate dangers, and while contrived here's the start of a possible scenario:

users create their identities using some identity factory by going to a uPort website
if the uPort website is compromised silently, it could be made to invoke the factories with timeLocks that would cause overflow
user identities are created without the "signed key change" expirations/protections that were designed in the RecoveryController
EDIT: Also, overflowing with timeLocks can also affect the instant add and removal of delegates in RecoveryQuorum
RecoveryQuorum.sol#L76

• IdentityManager::removeOwner()

As either removeOwner() function is written, an older owner has the ability to remove oneself as an owner. This functionality increases the likelihood that an owner accidentally deletes itself.

Recommendation

Add require() statement in both functions that ensures that removeOwner() cannot be called on oneself.

• Use of sha3 in TxRelay contract

In Solidity, sha3 and keccak256 are aliases for the same function, with keccak256 being the more accurate name which does not mislead new or casual observers about which hash algorithm is being executed (since the standardized SHA-3 is slightly different from Keccak-256).

Recommendation

Use keccak256 instead of sha3.

Is the uPort team considering adopting / supporting the ERC-725 Identity standard?

Many tests in the TxRelay contract test for throws in subcalls by asserting that the remaining gas is below a certain threshold. This approach is deprecated and will no longer work in versions of testrpc where revert() is implemented.

The typical onlyOwner() modifier throws if called by someone other than the owner. Throwing seems to be a slightly more conservative approach. We recognize that there may be reasons related to gas usage for not throwing.

Recommendation

Update access control logic to throw when called by an unauthorized party, or provide comments clarifying the motivation the existing pattern.