Slock.it Incubed3

Description

It is possible for a client to send a request to each node of the network to request a signature with proof for every other node in the network. This can result in DDoSing the network as there are no costs for the client to request this and client can send the same request to all the nodes in the network, resulting in n^2 requests.

Examples

1. Client asks each node for in3_nodeList to get all the signer addresses, this could also be done using NodeRegistry contract
2. Client asks each node for a proof with signature, e.g.:

{
    "jsonrpc": "2.0",
    "id": 2,
    "method": "eth_getTransactionByHash",
    "params": ["0xf84cfb78971ebd940d7e4375b077244e93db2c3f88443bb93c561812cfed055c"],
    "in3": {
        "chainId": "0x1",
        "verification": "proofWithSignature",
        "signatures":["0x784bfa9eb182C3a02DbeB5285e3dBa92d717E07a", ALL OTHER SIGNERS HERE]
  }
}

All the nodes are now sending requests to each other with signature required which is an expensive computation. This can go on for more transactions (or blocks, or other Eth_ requests) and can result in DDoS of the network.

Recommendation

Limit the number of signers in proof with signature requests. Also exclude self.signer from the list. This combined with the remediation of issue 6.6 can partially mitigate the attack vector.

Description

TLDR; One node can force all other nodes to convict a specific malicious signer controlled by the attacker and spend gas on something they are not going to be rewarded for. The attacker loses deposit but all other nodes that try to convict and recreate in the same block will lose the fees less or equal to deposit/2. Another variant forces the same node to recreate the blockheaders multiple times within the same block as the node does not check if it is already convicting/recreating blockheaders.

Nodes can request various types of proofs from other nodes. For example, if a node requests a proof when calling one of the eth_getBlock* methods, the in3-server’s method handleBlock will be called. The request should contain a list of addresses registered to the NodeRegistry that are requested to sign the blockhash.

code/in3-server/src/modules/eth/EthHandler.ts:L105-L112

// handle special jspn-rpc
if (request.in3.verification.startsWith('proof'))
  switch (request.method) {
    case 'eth_getBlockByNumber':
    case 'eth_getBlockByHash':
    case 'eth_getBlockTransactionCountByHash':
    case 'eth_getBlockTransactionCountByNumber':
      return handleBlock(this, request)

in3-server will subsequently reach out to it’s connected blockchain node execute the eth_getBlock* call to get the block data. If the block data is available the in3-server, it will try to collect signatures from the nodes that signature was requested from (request.in3.signatures, collectSignatures())

code/in3-server/src/modules/eth/proof.ts:L237-L243

// create the proof
response.in3 = {
  proof: {
    type: 'blockProof',
    signatures: await collectSignatures(handler, request.in3.signatures, [{ blockNumber: toNumber(blockData.number), hash: blockData.hash }], request.in3.verifiedHashes)
  }
}

If the node does not find the address it will throw an exception. Note that if this exception is not caught it will actually allow someone to boot nodes off the network - which is critical.

code/in3-server/src/chains/signatures.ts:L58-L60

const config = nodes.nodes.find(_ => _.address.toLowerCase() === adr.toLowerCase())
if (!config) // TODO do we need to throw here or is it ok to simply not deliver the signature?
  throw new Error('The ' + adr + ' does not exist within the current registered active nodeList!')

If the address is valid and existent in the NodeRegistry the in3-node will ask the node to sign the blockhash of the requested blocknumber:

code/in3-server/src/chains/signatures.ts:L69-L84

// send the sign-request
let response: RPCResponse
try {
  response = (blocksToRequest.length
    ? await handler.transport.handle(config.url, { id: handler.counter++ || 1, jsonrpc: '2.0', method: 'in3_sign', params: blocksToRequest })
    : { result: [] }) as RPCResponse
  if (response.error) {
    //throw new Error('Could not get the signature from ' + adr + ' for blocks ' + blocks.map(_ => _.blockNumber).join() + ':' + response.error)
    logger.error('Could not get the signature from ' + adr + ' for blocks ' + blocks.map(_ => _.blockNumber).join() + ':' + response.error)
    return null
  }
} catch (error) {
  logger.error(error.toString())
  return null
}

For all the signed blockhashes that have been returned the in3-server will subsequently check if one of the nodes provided a wrong blockhash.

We note that nodes might:

• decided to not follow the in_3sign request and just not provide a signed response
• a node might sign with a different key
• a node might sign a different blockheader
• a node might sign a previous blocknumber

In all these cases, the node will not be convicted, even though it was able to request other nodes to perform work.

If another node signed a wrong blockhash the in3-server will automatically try to convict it. If the block is within the most recent 255 it will directly call convict() on the NodeRegistry (takes less gas). if it is an older block, it will try to recreate the blockchain in the RlockhashRegistry (takes more gas).

code/in3-server/src/chains/signatures.ts:L128-L152

const convictSignature: Buffer = keccak(Buffer.concat([bytes32(s.blockHash), address(singingNode.address), toBuffer(s.v, 1), bytes32(s.r), bytes32(s.s)]))

if (diffBlocks < 255) {

  await callContract(handler.config.rpcUrl, nodes.contract, 'convict(uint,bytes32)', [s.block, convictSignature], {
    privateKey: handler.config.privateKey,
    gas: 500000,
    value: 0,
    confirm: true                       //  we are not waiting for confirmation, since we want to deliver the answer to the client.
  })

  handler.watcher.futureConvicts.push({
    convictBlockNumber: latestBlockNumber,
    signer: singingNode.address,
    wrongBlockHash: s.blockHash,
    wrongBlockNumber: s.block,
    v: s.v,
    r: s.r,
    s: s.s,
    recreationDone: true
  })
}
else {
  await handleRecreation(handler, nodes, singingNode, s, diffBlocks)
}

The recreation and convict is only done if it is profitable for the node. (Note the issue mentioned in issue 6.13)

code/in3-server/src/chains/signatures.ts:L209-L213

const costPerBlock = 86412400000000
const blocksMissing = latestSS - s.block
const costs = blocksMissing * costPerBlock * 1.25

if (costs > (deposit / 2)) {

A malicious node can exploit the hardcoded profit economics and the fact that in3-server implementation will try to auto-convict nodes in the following scenario:

• malicious node requests a blockproof with an eth_getBlock* call from the victim node (in3-server) for a block that is not in the most recent 256 blocks (to maximize effort for the node). This equals to spending more gas in order to convict the node (costs <= (deposit / 2)).
• the malicious node prepares the BlockhashRegistry to contain a blockhash that would maximize the gas needed to convict the malicious node (can be calculated offline; must fulfill costs <= (deposit /2).
• with the blockproof request the malicious node asks the in3-server to get the signature from a specific signer. The signer will also be malicious and is going to sign a wrong blockhash with a valid signature.
• the malicious signer is going to lose it’s deposit but the deposit also incentivizes other nodes to spend gas on the conviction process. The higher the deposit, the more an in3-server is willing to spend on the conviction.

In this scenario one malicious node tries to trick another node into convicting a malicious signer while having to spend the maximum amount of gas to make it profitable for the node.

The problem is, that the malicious node can ask multiple (or even all other nodes in the registry) to provide a blockproof and ask the malicious signer for a signed blockhash. All nodes will come to the conclusion that the signer returned an invalid hash and will try to convict the node. They will try to recreate the blockchain in the BlockhashRegistry for a barely profitable scenario. Since in3-nodes do not monitor the tx-pool they will not know that other nodes are already trying to convict the node. All nodes are going to spend gas on recreating the same blockchain in the BlockhashRegistry leading to all but the first transaction in the block to lose funds (up to deposit/2 based on the hardcoded costPerBlock)

Another variant of the same issue is that nodes do not check if they already convicted another node (or recreated blockheaders). An attacker can therefore force a specific node to convict a malicious node multiple times before the nodes transactions are actually in a block as the nodes does not check if it is already convicting that node. The node might lose gas on the recreation/conviction process multiple times.

Recommendation

To reduce the impact of multiple nodes trying to update the blockhashRegistry at the same time and avoid nodes losing gas by recreating the same blocks over and over again, the BlockhashRegistry should require that the target blockhash for the blocknumber does not yet exist in the registry (similar to the issue mentioned in https://github.com/ConsenSys/slockit-in3-audit-2019-09/issues/24).

Description

convict(uint _blockNumber, bytes32 _hash) and revealConvict() are designed to prevent front-running and they do so for the purpose they are designed for. However, if the malicious node, is still sending out the wrong blockhash for the convicted block, anyone seeing the initial convict transaction, can check the convicted blocknumber with the nodes and send his own revealConvict before the original sender.

The original sender will be the one updating the block headers recreateBlockheaders(_blockNumber, _blockheaders), and the attacker can just watch for the update headers to perform this attack.

Recommendation

For the first attack vector, remove the blocknumber from the convict(uint _blockNumber, bytes32 _hash) inputs and just use the hash.

Description

As outlined in issue 6.9 the NodeRegistry allows anyone to register nodes with arbitrary URLs. The url is then used by in3-server or clients to connect to other nodes in the system. Signers can only be convicted if they sign wrong blockhashes. However, if they never provide any signatures they can stay in the registry for as long as they want and sabotage the network. The Registry implements an admin functionality that is available for the first year to remove misbehaving nodes (or spam entries) from the Registry. However, this is insufficient as an attacker might just re-register nodes after the minimum timeout they specify or spend some more finneys on registering more nodes. Depending on the eth-price this will be more or less profitable.

From an attackers perspective the NodeRegistry is a good source of information for reconnaissance, allows to de-anonymize and profile nodes based on dns entries or netblocks or responses to in3_stats (https://github.com/ConsenSys/slockit-in3-audit-2019-09/issues/49), makes a good list of target for DoS attacks on the system or makes it easy to exploit nodes for certain yet unknown security vulnerabilities.

Since nodes and potentially clients (not in scope) do not validate the rpc URL received from the NodeRegistry they will try to connect to whatever is stored in a nodes url entry.

code/in3-server/src/chains/signatures.ts:L58-L75

const config = nodes.nodes.find(_ => _.address.toLowerCase() === adr.toLowerCase())
if (!config) // TODO do we need to throw here or is it ok to simply not deliver the signature?
  throw new Error('The ' + adr + ' does not exist within the current registered active nodeList!')

// get cache signatures and remaining blocks that have no signatures
const cachedSignatures: Signature[] = []
const blocksToRequest = blocks.filter(b => {
  const s = signatureCaches.get(b.hash) && false
  return s ? cachedSignatures.push(s) * 0 : true
})

// send the sign-request
let response: RPCResponse
try {
  response = (blocksToRequest.length
    ? await handler.transport.handle(config.url, { id: handler.counter++ || 1, jsonrpc: '2.0', method: 'in3_sign', params: blocksToRequest })
    : { result: [] }) as RPCResponse
  if (response.error) {

This allows for a wide range of attacks not limited to:

• An attacker might register a node with an empty or invalid URL. The in3-server does not validate the URL and therefore will attempt to connect to the invalid URL, spending resources (cpu, file-descriptors, ..) to find out that it is invalid.
• An attacker might register a node with a URL that is pointing to another node’s rpc endpoint and specify weights that suggest that it is capable of service a lot of requests to draw more traffic towards that node in an attempt to cause a DoS situation.
• An attacker might register a node for a http/https website at any port in an extortion attempt directed to website owners. The incubed network nodes will have to learn themselves that the URL is invalid and they will at least attempt to connect the website once.
• An attacker might update the node information in the NodeRegistry for a specific node every block, providing a new url (or a slightly different URLs issue 6.9) to avoid client/node URL blacklists.
• An attacker might provide IP addresses instead of DNS resolvable names with the url in an attempt to draw traffic to targets, avoiding canonicalization and blacklisting features.
• An attacker might provide a URL that points to private IP netblocks for IPv4 or IPv6 in various formats. Combined with the ability to ask another node to connect to an attacker defined url (via blockproof, signatures[] -> signer_address -> signer.url) this might allow an attacker to enumerate services in the LAN of node operators.
• An attacker might provide the loopback IPv4, IPv6 or resolvable name as the URL in an attempt to make the node connect to local loopback services (service discovery, bypassing authentication for some local running services - however this is very limited to the requests nodes may execute).
• URLs may be provided in various formats: resolvable dns names, IPv4, IPv6 and depending on the http handler implementation even in Decimal, Hex or Octal form (i.e. http://2130706433/)
• A valid DNS resolvable name might point to a localhost or private IP netblock.

Since none of the rpc endpoints provide signatures they cannot be convicted or removed (unless the unregisterKey does it within the first year. However, that will not solve the problem that someone can re-register the same URLs over and over again)

Recommendation

It is a fundamental design decision of the system architecture to allow rpc urls in the Node Registry, therefore this issue can only be partially mitigated unless the system design is reworked. It is therefore suggested to add checks to both the registry contract (coarse validation to avoid adding invalid urls) and node implementations (rigorous validation of URL’s and resolved IP addresses) and filter out any potentially harmful destinations.

Description

In case of reorgs it is possible to have more than 6 blocks in a node that gets replaced by a new longer chain. Also for forks, such as upcoming Istanbul fork, it’s common to have some nodes taking some time to update and they will be in the wrong chain for the time being. In both cases, in3-nodes are prone to sign blocks that are considered invalid in the main chain. Malicious nodes can catch these instances and convict the honest users in the main chain to get 50% of their deposits.

Recommendation

No perfect solution comes to mind at this time. One possible mitigation method for forks could be to disable the network on the time of the fork but this is most certainly going to be a threat to the system itself.

Description

The in3-node implementation should provide features for client request throttling to avoid that a client can consume most of the nodes resources by causing a lot of resource intensive requests.

This is a general problem to the system which is designed to make sure that low resource clients can verify blockchain properties. What this means is that almost all of the client requests are very lightweight. Clients can request nodes to sign data for them. A sign request involves cryptographic operations and a http-rpc request to a back-end blockchain node. The imbalance is clearly visible in the case of blockProofs where a client may request another node to interact with a smart contract (NodeRegistry) and ask other nodes to sign blockhashes. All other nodes will have to get the requested block data from their local blockchain nodes and the incubed node requesting the signatures will have to wait for all responses. The client instead only has to send out that request once and may just leave that tcp connection open. It might even consume more resources from a specific node by requesting the same signatures again and again not even waiting for a response but causing a lot of work on the node that has to collect all the signatures. This combined with unbound requests for signatures or other properties can easily be exploited by a powerful client implementation with a mission to stall the whole incubed network.

Recommendation

According to the threat model outlines a general DDoS scenario specific to rpcUrls. It discusses that the nodes are themselves responsible for DDoS protection. However, DDoS protection is a multi-layer approach and it is highly unlikely that every node-operator will hide their nodes behind a DDoS CDN like cloudflare. We therefore suggest to also build in strict limitations for clients that can be checked in code. Similar to checkPerformanceLimits which is just checking for some specific it is suggested to implement a multi-layer throttling mechanism that prevents nodes from being abused by single clients. Methods must be designed with (D)DoS scenarios in mind to avoid that third parties are abusing the network for DDoS campaigns or trying to DoS the incubed network.

code/in3-server/src/modules/eth/EthHandler.ts:L74-L91

private checkPerformanceLimits(request: RPCRequest) {
  if (request.method === 'eth_call') {
    if (!request.params || request.params.length < 2) throw new Error('eth_call must have a transaction and a block as parameters')
    const tx = request.params as TxRequest
    if (!tx || (tx.gas && toNumber(tx.gas) > 10000000)) throw new Error('eth_call with a gaslimit > 10M are not allowed')
  }
  else if (request.method === 'eth_getLogs') {
    if (!request.params || request.params.length < 1) throw new Error('eth_getLogs must have a filter as parameter')
    const filter: LogFilter = request.params[0]
    let toB = filter && filter.toBlock
    if (toB === 'latest' || toB === 'pending' || !toB) toB = this.watcher && this.watcher.block && this.watcher.block.number
    let fromB = toB && filter && filter.fromBlock
    if (fromB === 'earliest') fromB = 1;
    const range = fromB && (toNumber(toB) - toNumber(fromB))
    if (range > (request.in3.verification.startsWith('proof') ? 1000 : 10000))
      throw new Error('eth_getLogs for a range of ' + range + ' blocks is not allowed. limits: with proof: 1000, without 10000 ')
  }
}

• implement request throttling per client
• implement caching mechanism for similar requests if it is expected that the same response is to be delivered multiple times
• implement general performance limits and reject further requests if the node is close to exhausting its resources (soft DoS)
• make sure the node does not exhaust the systems resources
• implement throttling per request method
• design methods to prevent (D)DoS in the first place. Methods that allow a client to send one request that causes a node to perform multiple client controlled requests must be avoided or at least bound and throttled (issue 6.7, https://github.com/ConsenSys/slockit-in3-audit-2019-09/issues/50).

Description

It is free for the client to ask the nodes to sign block hashes (and also other requests). in3.sign([{"blockNumber": 123}]) Takes an array of objects that will result in multiple requests in the node. This sample request has (at least) two internal requests, one eth_getBlockByNumber and signing the block hash.

These requests can be continuously sent out to clients and result in using computation power of the nodes without any expense from the client.

Examples

Request to get and sign the first 200 blocks:

web3.manager.request_blocking("in3_sign", [{'blockNumber':i} for i in range(200)])

Recommendation

Limit the number of blocks (input), or do not accept arrays for input.

Description

Secure and efficient key management is a challenge for any cryptographic system. Incubed nodes for example require an account on the ethereum blockchain to actively participate in the incubed network. The account and therefore a private-key is used to sign transactions on the ethereum blockchain and to provide signed proofs to other in3-nodes.

This means that an attacker that is able to discover the keys used by an in3-server by any mechanism may be able to impersonate that node, steal the nodes funds or sign wrong data on behalf of the node which might also lead to a loss of funds.

The private key for the in3-server can be specified in a configuration file called config.json residing in the program working dir. Settings from the config.json can be overridden via command-line options. The application keeps configuration parameters available internally in an IN3RPCConfig object and passes this object as an initialization parameter to other objects.

The key can either be provided in plaintext as a hex-string starting with 0x or within an ethereum keystore format compatible protected keystore file. Either way it is provided it will be held in plaintext in the object.

The application accepts plaintext private keys and the keys are stored unprotected in the applications memory in JavaScript objects. The in3-server might even re-use the nodes private key which may weaken the security provided by the node. The repository leaks a series of presumably ’test private keys’ and the default config file already comes with a private key set that might be shared across unvary users that fail to override it.

code/in3-server/config.json:L1-L4

{
  "privateKey": "0xc858a0f49ce12df65031ba0eb0b353abc74f93f8ccd43df9682fd2e2293a4db3",
  "rpcUrl": "http://rpc-kovan.slock.it"
}

code/in3-server/package.json:L20-L31

"docker-run": "docker run -p 8500:8500 docker.slock.it/slockit/in3-server:latest --privateKey=0x3858a0f49ce12df65031ba0eb0b353abc74f93f8ccd43df9682fd2e2293a4db3 --chain=0x2a --rpcUrl=https://kovan.infura.io/HVtVmCIHVgqHGUgihfhX --minBlockHeight=6 --registry=0x013b82355a066A31427df3140C5326cdE9c64e3A --persistentFile=false --logging-host=logs7.papertrailapp.com --logging-name=Papertrail --logging-port=30571 --logging-type=winston-papertrail",
"docker-setup": "docker run -p 8500:8500 slockit/in3-server:latest --privateKey=0x3858a0f49ce12df65031ba0eb0b353abc74f93f8ccd43df9682fd2e2293a4db3 --chain=0x2a --rpcUrl=https://kovan.infura.io/HVtVmCIHVgqHGUgihfhX --minBlockHeight=6 --registry=0x013b82355a066A31427df3140C5326cdE9c64e3A --persistentFile=false --autoRegistry-url=https://in3.slock.it/kovan1 --autoRegistry-capabilities-proof=true --autoRegistry-capabilities-multiChain=true --autoRegistry-deposit=1",
"local": "export NODE_ENV=0 && npm run build && node ./js/src/server/server.js --privateKey=0xD231FCF9349A296F555A060A619235F88650BBA795E5907CFD7F5442876250E4 --chain=0x2a --rpcUrl=https://rpc.slock.it/kovan --minBlockHeight=6 --registry=0x27a37a1210df14f7e058393d026e2fb53b7cf8c1 --persistentFile=false",
"ipfs": "docker run -d -p 5001:5001 jbenet/go-ipfs  daemon --offline",
"linkIn3": "cd node_modules; rm -rf in3; ln -s ../../in3 in3; cd ..",
"lint:solium": "node node_modules/ethlint/bin/solium.js -d contracts/",
"lint:solium:fix": "node node_modules/ethlint/bin/solium.js -d contracts/ --fix",
"lint:solhint": "node node_modules/solhint/solhint.js \"contracts/**/*.sol\" -w 0",
"local-env": "export NODE_ENV=0 && npm run build && node ./js/src/server/server.js --privateKey=0x9e53e6933d69a28a737943e227ad013c7489e366f33281d350c77f089d8411a6 --chain=0x111 --rpcUrl=http://localhost:8545 --minBlockHeight=6 --registry=0x31636f91297C14A8f1E7Ac271f17947D6A5cE098 --persistentFile=false --autoRegistry-url=http://127.0.0.1:8500 --autoRegistry-capabilities-proof=true --autoRegistry-capabilities-multiChain=true --autoRegistry-deposit=0",
"local-env2": "export NODE_ENV=0 && npm run build && node ./js/src/server/server.js --privateKey=0xf7db260e6edcdfe396d75f8283aad5aed835815f7d1db4458896310553a8a1a9 --chain=0x111 --rpcUrl=http://localhost:8545 --minBlockHeight=6 --registry=0x31636f91297C14A8f1E7Ac271f17947D6A5cE098 --persistentFile=false --autoRegistry-url=http://127.0.0.1:8501 --autoRegistry-capabilities-proof=true --autoRegistry-capabilities-multiChain=true --autoRegistry-deposit=0",
"local-env3": "export NODE_ENV=0 && npm run build && node ./js/src/server/server.js --privateKey=0xf7db260e6edcdfe396d75f8283aad5aed835815f7d1db4458896310553a8a1a9 --chain=0x5 --rpcUrl=https://rpc.slock.it/goerli --minBlockHeight=6 --registry=0x85613723dB1Bc29f332A37EeF10b61F8a4225c7e --persistentFile=false",
"local-env4": "export NODE_ENV=0 && npm run build && node ./js/src/server/server.js --privateKey=0xf7db260e6edcdfe396d75f8283aad5aed835815f7d1db4458896310553a8a1a9 --chain=0x2a --rpcUrl=https://rpc.slock.it/kovan --minBlockHeight=6 --registry=0x27a37a1210df14f7e058393d026e2fb53b7cf8c1 --persistentFile=false"

The private key is also passed as arguments to other functions. In error cases these may leak the private key to log interfaces or remote log aggregation instances (sentry). See txargs.privateKey in the example below:

code/in3-server/src/util/tx.ts:L100-L100

const key = toBuffer(txargs.privateKey)

code/in3-server/src/util/tx.ts:L134-L140

const txHash = await transport.handle(url, {
  jsonrpc: '2.0',
  id: idCount++,
  method: 'eth_sendRawTransaction',
  params: [toHex(tx.serialize())]
}).then((_: RPCResponse) => _.error ? Promise.reject(new SentryError('Error sending tx', 'tx_error', 'Error sending the tx ' + JSON.stringify(txargs) + ':' + JSON.stringify(_.error))) as any : _.result + '')

Recommendation

• Keys should never be stored or accepted in plaintext format.
  • Keys should not be stored in plaintext on the file-system as they might easily be exposed to other users. Credentials on the file-system must be tightly restricted by access control.
  • Keys should not be provided as plaintext via environment variables as this might make them available to other processes sharing the same environment (child-processes, e.g. same shell session)
  • Keys should not be provided as plaintext via command-line arguments as they might persist in the shell’s command history or might be available to privileged system accounts that can query other processes startup parameters.
• Keys should only be accepted in an encrypted and protected format.
• A single key should be used for only one purpose. Keys should not be shared.
  • The use of the same key for two different cryptographic processes may weaken the security provided by one or both of the processes.
  • The use of the same key for two different applications may weaken the security provided by one or both of the applications.
  • Limiting the use of a key limits the damage that could be done if the key is compromised.
  • Node owners keys should not be re-used as signer keys.
• The application should support developers in understanding where cryptographic keys are stored within the application as well as in which memory regions they might be accessible for other applications.
• Keys should be protected in memory and only decrypted for the duration of time they are actively used.
• Keys should not be stored with the applications source-code repository.
• Use standard libraries for cryptographic operations.
• Use the system keystore and API to sign and avoid to store key material at all.
• The application should store the keys eth-address (util.getAddress()) instead of re-calculating it multiple times from the private key.
• Do not leak credentials and key material in debug-mode, to local log-output or external log aggregators.

Description

One of the requirements for Node registration is to have a unique URL which is not already used by a different owner. The uniqueness check is done by hashing the provided _url and checking if someone already registered with that hash of _url.

However, byte-equality checks (via hashing in this case) to enforce uniqueness will not work for URLs. For example, while the following URLs are not equal and will result in different urlHashes they can logically be the same end-point:

• https://some-server.com/in3-rpc
• https://some-server.com:443/in3-rpc
• https://some-server.com/in3-rpc/
• https://some-server.com/in3-rpc///
• https://some-server.com/in3-rpc?something
• https://some-server.com/in3-rpc?something&something
• https://www.some-server.com/in3-rpc?something (if www resolves to the same ip)

code/in3-contracts/contracts/NodeRegistry.sol:L547-L553

bytes32 urlHash = keccak256(bytes(_url));

// make sure this url and also this owner was not registered before.
// solium-disable-next-line
require(!urlIndex[urlHash].used && signerIndex[_signer].stage == Stages.NotInUse,
    "a node with the same url or signer is already registered");

This leads to the following attack vectors:

• A user signs up multiple nodes that resolve to the same end-point (URL). A minimum deposit of 0.01 ether is required for each registration. Registering multiple nodes for the same end-point might allow an attacker to increase their chance of being picked to provide proofs. Registering multiple nodes requires unique signer addresses per node.

• Also one node can have multiple accounts, hence one node can have slightly different URL and different accounts as the signers.

• DoS - A user might register nodes for URLs that do not serve in3-clients in an attempt to DDoS e.g. in an attempt to extort web-site operators. This is kind of a reflection attack where nodes will request other nodes from the contract and try to contact them over RPC. Since it is http-rpc it will consume resources on the receiving end.

• DoS - A user might register Nodes with RPC URLs of other nodes, manipulating weights to cause more traffic than the node can actually handle. Nodes will try to communicate with that node. If no proof is requested the node will not even know that someone else signed up other nodes with their RPC URL to cause problems. If they request proof the original signer will return a signed proof and the node will fail due to a signature mismatch. However, the node cannot be convicted and therefore forced to lose the deposit as conviction is bound the signer and the block was not signed by the rogue node entry. There will be no way to remove the node from the registry other than the admin functionality.

Recommendation

Canonicalize URLs, but that will not completely prevent someone from registering nodes for other end-points or websites. Nodes can be removed by an admin in the first year but not after that. Rogue owners cannot be prevented from registering random nodes with high weights and minimum deposit. They cannot be convicted as they do not serve proofs. Rogue owners can still unregister to receive their deposit after messing with the system.

Description

A node that is signing wrong blockhashes might get their deposit slashed from the registry. The entity that is convicting a node that signs a wrong blockhash is awarded half of the deposit.

A threat to this kind of system is that blocks might constantly be reorganized in the chain, especially with the latest block. Allowing a node to sign the latest block will definitely put the node’s deposit at stake with every signature they provide.

A node can configure the minBlockHeight it is about to sign with a configurative option. The option defaults to a minBlockHeight of 6 in the default config:

code/in3-server/src/server/config.ts:L32-L32

minBlockHeight: 6,

And again in the signing function for blockheaders:

code/in3-server/src/chains/signatures.ts:L189-L189

const blockHeight = handler.config.minBlockHeight === undefined ? 6 : handler.config.minBlockHeight

handleSign will refuse to sign any block that is within the last 5 blocks. The 6th block will be signed.

code/in3-server/src/chains/signatures.ts:L190-L193

const tooYoungBlock = blockData.find(block => toNumber(blockNumber) - toNumber(block.number) < blockHeight)
if (tooYoungBlock)
  throw new Error(' cannot sign for block ' + tooYoungBlock.number + ', because the blockHeight must be at least ' + blockHeight)

However, a user is not prevented from configuring an insecure minBlockHeight (e.g. 0) which will very likely lead to the loss of funds because the node will be signing the latest block.

The current default of 6 blocks leads to an approximate lag of 14 (avg blocktime) *6 (blocks) = 84 seconds. While this is a favorable setting because it allows nodes to provide signatures for blocks that are at least older than 6 blocks it might still not be secure. For example, CryptoExchange Kraken requires at least 30 confirmation (abt. 6 minutes) until a transaction is confirmed. For Bitcoin it is said to be safe to wait more than 6 blocks (abt. 1 hr) for a transaction to be confirmed. ETC even underwent a deep chain reorg that could have caused many nodes to lose their deposits. The ethereum whitepaper defines an uncle that can be referenced in a block to have the following property: It must be a direct child of the k-th generation ancestor of B, where 2 <= k <= 7. This suggests that k=7‘th block can at least still be an uncle. Bitfinex requires a minimum of 10 confirmations. Some blockchain explorers and analytics tools also require a minimum of 10 confirmations. Scraped data from https://etherscan.io/blocks_forked?ps=100 shows 3 forks of depth 3 since they started keeping records 115 days ago, and no forks deeper than 3. So some applications might legitimately pick a number somewhere between 5 and 20, trading some security for better UX. However, it should be re-evaluated whether the current default provides enough security to protect the nodes funds with a trade-off of lag to the network.

Given these values it is suggested to revalidate the default of a minBlockHeight of 6 in favor of a more secure depth to make sure that - with a default setting - nodes will not lose funds in case of re-orgs.

Recommendation

• config.minBlockHeight should always be set to a sane value when loading the configuration. There should be no need to reset it to a hardcoded default value of 6 in handleSign. Do not hardcode the values in various places in the config.
• normalize and sanitize the settings to make sure that after loading they are always valid and within reasonable bounds. the application should refuse to run with a minBlockHeader set to 0 as this is a guarantee for losing funds. Other nodes can enumerate nodes that are misconfigured (e.g. with minBlockHeight being 0) to request signatures just to convict them on micro-forks.
• assume a secure default setting for every chain (note that this might be different for every chain). allow to override the value by the user. warn the user of less secure settings and do not allow to set settings that are obviously leading to the loss of funds.
• re-evaluate the minBlockHeight of 6 for the ethereum blockchain and choose a koservative secure default.

Description

According to the specification incubed requests must specify whether they want to have a proof or not. There are three variants of proofs that can be requested:

• never - no proof appended
• proof - proof but no signed blockhashes
• proofWithSignature- proof and a request to sign blockhashes from the list of addresses provided in signatures.

Note that the name signatures for the array of signers a blockhash signature is requested from is misleading. It is actually signer addresses as listed in the NodeRegistry and not signatures.

Following the in3-server we found at least one inconsistency (and suspect more) with the proof requested by a client. The graceful check for the existence of something starting with proof will pass proof and proofWithSignature but also any other proofXYZ to the blockproof handler.

code/in3-server/src/modules/eth/EthHandler.ts:L106-L112

if (request.in3.verification.startsWith('proof'))
  switch (request.method) {
    case 'eth_getBlockByNumber':
    case 'eth_getBlockByHash':
    case 'eth_getBlockTransactionCountByHash':
    case 'eth_getBlockTransactionCountByNumber':
      return handleBlock(this, request)

Following through handleBlock we cannot find any check for proofWithSignature. The string is not found in the whole codebase which also suggests it is not tested. However, the code assumes that because request.in3.signatures is not empty, signatures were requested. This is inconsistent with the specification and a protocol violation.

code/in3-server/src/modules/eth/proof.ts:L237-L244

// create the proof
response.in3 = {
  proof: {
    type: 'blockProof',
    signatures: await collectSignatures(handler, request.in3.signatures, [{ blockNumber: toNumber(blockData.number), hash: blockData.hash }], request.in3.verifiedHashes)
  }
}

The same is valid for all other types of proofs. proofWithSignature is never checked and it is assumed that proofWithSignature was requested just because request.in3.signatures is present non-empty.

The same is true for ’never’ which is actually never handled in code.

Recommendation

The protocol should be strictly enforced without allowing any ambiguities and unsharpness. Ambiguities and gracefulness in the protocol can lead to severe inconsistencies and encourage client authors to not strictly adhere to the protocol. This makes it hard to update and maintain the protocol in the future and may allow potential attackers enough freedom to exploit the protocol. Furthermore the specification must be kept up-to-date at all times. The specification is to lead development and code must always be verified against the specification.

Description

There are many instances of hardcoded gas limit in in3-server that depending on the complexity of the transaction or gas cost changes in Ethereum could result in failed transactions.

Examples

convict():

code/in3-server/src/chains/signatures.ts:L132-L137

await callContract(handler.config.rpcUrl, nodes.contract, 'convict(uint,bytes32)', [s.block, convictSignature], {
  privateKey: handler.config.privateKey,
  gas: 500000,
  value: 0,
  confirm: true                       //  we are not waiting for confirmation, since we want to deliver the answer to the client.
})

recreateBlockheaders():

code/in3-server/src/chains/signatures.ts:L275-L280

await callContract(handler.config.rpcUrl, blockHashRegistry, 'recreateBlockheaders(uint,bytes[])', [latestSS - diffBlock, txArray], {
  privateKey: handler.config.privateKey,
  gas: 8000000,
  value: 0,
  confirm: true                       //  we are not waiting for confirmation, since we want to deliver the answer to the client.
})

Other instances of hard coded gasLimit or gasPrice:

code/in3-server/src/modules/eth/EthHandler.ts:L78-L79

  if (!tx || (tx.gas && toNumber(tx.gas) > 10000000)) throw new Error('eth_call with a gaslimit > 10M are not allowed')
}

Recommendation

Use web3 gas estimate instead. To be sure, there can be an additional gas added to the estimated value or max(HARDCODED_GAS, estimated_amount)

Description

A node that wants to convict another node for false proof must update the BlockhashRegistry for signatures provided in blocks older than the most recent 256 blocks. Only when the smart contract is able to verify that the signed blockhash is wrong the convicting node will be able to receive half of its deposit.

The in3-server implements an automated mechanism to recreate blockhashes. It first searches for an existing blockhash within a range of blocks. If one is found and it is profitable (gas spend vs. amount awarded) the node will try to recreate the blockchain updating the registry.

• The call to searchForAvailableBlock might return 0 (default) because no block is actually found within the range, this will cause costs to be negative and the code will proceed trying to convict the node even though it cannot work.

• The call to searchForAvailableBlock might also return the convict block number (latestSS==s.block) in which case costs will be 0 and the code will still proceed trying to recreate the blockheaders and convict the node.

code/in3-server/src/chains/signatures.ts:L207-L231

const [, deposit, , , , , , ,] = await callContract(handler.config.rpcUrl, nodes.contract, 'nodes(uint):(string,uint,uint64,uint64,uint128,uint64,address,bytes32)', [toNumber(singingNode.index)])
const latestSS = toNumber((await callContract(handler.config.rpcUrl, blockHashRegistry, 'searchForAvailableBlock(uint,uint):(uint)', [s.block, diffBlocks]))[0])
const costPerBlock = 86412400000000
const blocksMissing = latestSS - s.block
const costs = blocksMissing * costPerBlock * 1.25

if (costs > (deposit / 2)) {

  console.log("not worth it")
  //it's not worth it
  return
}
else {

  // it's worth convicting the server
  const blockrequest = []
  for (let i = 0; i < blocksMissing; i++) {
    blockrequest.push({
      jsonrpc: '2.0',
      id: i + 1,
      method: 'eth_getBlockByNumber', params: [
        toHex(latestSS - i), false
      ]
    })
  }

Please note that certain parts of the code rely on hardcoded gas values. Gas economics might change with future versions of the evm and have to be re-validated with every version. It is also good practice to provide inline comments about how and on what base certain values were selected.

Recommendation

Verify that the call succeeds and returns valid values. Check if the block already exists in the BlockhashRegistry and avoid recreation. Also note that searchForAvailableBlock can wrap with values close to uint_max even though that is unlikely to happen. In general, return values for external calls should be validated more rigorously.

Description

The system has centralized power structure for the first year after deployment. An unregisterKey (creator of the contract) is allowed to remove Nodes that are in state Stages.Active from the registry, only in 1st year.

However, there is no possibility to remove malicious nodes from the registry after that.

code/in3-contracts/contracts/NodeRegistry.sol:L249-L264

/// @dev only callable in the 1st year after deployment
function removeNodeFromRegistry(address _signer)
    external
    onlyActiveState(_signer)
{

    // solium-disable-next-line security/no-block-members
    require(block.timestamp < (blockTimeStampDeployment + YEAR_DEFINITION), "only in 1st year");// solhint-disable-line not-rely-on-time
    require(msg.sender == unregisterKey, "only unregisterKey is allowed to remove nodes");

    SignerInformation storage si = signerIndex[_signer];
    In3Node memory n = nodes[si.index];

    unregisterNodeInternal(si, n);

}

Recommendation

Provide a solution for the network to remove fraudulent node entries. This could be done by voting mechanism (with staking, etc).

Description

An owner can register a node with the signer not being the owner by calling registerNodeFor. The owner submits a message signed for the owner including the properties of the node including the url.

• The signed data does not include the registryID nor the NodeRegistry’s address and can therefore be used by the owner to submit the same node to multiple registries or chains without the signers consent.

• The signed data does not expire and can be re-used by the owner indefinitely to submit the same node again to future contracts or the same contract after the node has been removed.

• Arguments are not validated in the external function (also see issue 6.17)

code/in3-contracts/contracts/NodeRegistry.sol:L215-L223

bytes32 tempHash = keccak256(
    abi.encodePacked(
        _url,
        _props,
        _timeout,
        _weight,
        msg.sender
    )
);

Recommendation

Include registryID and an expiration timestamp that is checked in the contract with the signed data. Validate function arguments.

Description

getParentAndBlockhash takes an rlp-encoded blockheader blob, extracts the parent parent hash and returns both the parent hash and the calculated blockhash of the provided data. The method is used to add blockhashes to the registry that are older than 256 blocks as they are not available to the evm directly. This is done by establishing a trust-chain from a blockhash that is already in the registry up to an older block

1. The method assumes that valid rlp encoded data is provided but the structure is not verified (rlp decodes completely; block number is correct; timestamp is younger than prevs, …), giving a wide range of freedom to an attacker with enough hashing power (or exploiting potential future issues with keccak) to forge blocks that would never be accepted by clients, but may be accepted by this smart contract. (threat: mining pool forging arbitrary non-conformant blocks to exploit the BlockhashRegistry)

2. It is not checked that input was actually provided. However, accessing an array at an invalid index will raise an exception in the EVM. Providing a single byte > 0xf7 will yield a result and succeed even though it would have never been accepted by a real node.

3. It is assumed that the first byte is the rlp encoded length byte and an offset into the provided _blockheader bytes-array is calculated. Memory is subsequently accessed via a low-level mload at this calculated offset. However, it is never validated that the offset actually lies within the provided range of bytes _blockheader leading to an out-of-bounds memory read access.

4. The rlp encoded data is only partially decoded. For the first rlp list the number of length bytes is extracted. For the rlp encoded long string a length byte of 1 is assumed. The inline comment appears to be inaccurate or might be misleading. // we also have to add "2" = 1 byte to it to skip the length-information

5. Invalid intermediary blocks (e.g. with parent hash 0x00) will be accepted potentially allowing an attacker to optimize the effort needed to forge invalid blocks skipping to the desired blocknumber overwriting a certain blockhash (see issue 6.18)

6. With one collisions (very unlikely) an attacker can add arbitrary or even random values to the BlockchainRegistry. The parent-hash of the starting blockheader cannot be verified by the contract ([target_block_random]<--parent_hash--[rnd]<--parent_hash--[rnd]<--parent_hash--...<--parent_hash--[collision]<--parent_hash_collission--[anchor_block]). While nodes can verify block structure and bail on invalid structure and check the first blocks hash and make sure the chain is in-tact the contract can’t. Therefore one cannot assume the same trust in the blockchain registry when recreating blocks compared to running a full node.

code/in3-contracts/contracts/BlockhashRegistry.sol:L98-L126

function getParentAndBlockhash(bytes memory _blockheader) public pure returns (bytes32 parentHash, bytes32 bhash) {

    /// we need the 1st byte of the blockheader to calculate the position of the parentHash
    uint8 first = uint8(_blockheader[0]);

    /// calculates the offset
    /// by using the 1st byte (usually f9) and substracting f7 to get the start point of the parentHash information
    /// we also have to add "2" = 1 byte to it to skip the length-information
    require(first > 0xf7, "invalid offset");
    uint8 offset = first - 0xf7 + 2;

    /// we are using assembly because it's the most efficent way to access the parent blockhash within the rlp-encoded blockheader
    // solium-disable-next-line security/no-inline-assembly
    assembly { // solhint-disable-line no-inline-assembly
        // mstore to get the memory pointer of the blockheader to 0x20
        mstore(0x20, _blockheader)

        // we load the pointer we just stored
        // then we add 0x20 (32 bytes) to get to the start of the blockheader
        // then we add the offset we calculated
        // and load it to the parentHash variable
        parentHash :=mload(
            add(
                add(
                    mload(0x20), 0x20
                ), offset)
        )
    }
    bhash = keccak256(_blockheader);

Recommendation

1. Validate that the provided data is within a sane range of bytes that is expected (min/max blockheader sizes).
2. Validate that the provided data is actually an rlp encoded blockheader.
3. Validate that the offset for the parent Hash is within the provided data.
4. Validate that the parent Hash is non zero.
5. Validate that blockhashes do not repeat.

Description

Methods and Functions usually live in one of two worlds:

• public API - methods declared with visibility public or external exposed for interaction by other parties
• internal API - methods declared with visibility internal, private that are not exposed for interaction by other parties

While it is good practice to visually distinguish internal from public API by following commonly accepted naming convention e.g. by prefixing internal functions with an underscore (_doSomething vs. doSomething) or adding the keyword unsafe to unsafe functions that are not performing checks and may have a dramatic effect to the system (_unsafePayout vs. RequestPayout), it is important to properly verify that inputs to methods are within expected ranges for the implementation.

Input validation checks should be explicit and well documented as part of the code’s documentation. This is to make sure that smart-contracts are robust against erroneous inputs and reduce the potential attack surface for exploitation.

It is good practice to verify the methods input as early as possible and only perform further actions if the validation succeeds. Methods can be split into an external or public API that performs initial checks and subsequently calls an internal method that performs the action.

The following lists some public API methods that are not properly checking the provided data:

• BlockhashRegistry.reCalculateBlockheaders - bhash can be zero; blockheaders can be empty
• BlockhashRegistry.getParentAndBlockhash- blockheader structure can be random as long as parenthash can be extracted
• BlockhashRegistry.recreateBlockheaders - blockheaders can be empty; Arguments should be validated before calculating values that depend on them:

code/in3-contracts/contracts/BlockhashRegistry.sol:L70-L70

assert(_blockNumber > _blockheaders.length);

• BlockhashRegistry.searchForAvailableBlock - _startNumber + _numBlocks can be > block.number; _startNumber + _numBlocks can overflow.

• NodeRegistry.removeNode - should check require(_nodeIndex < nodes.length) first before any other action.

code/in3-contracts/contracts/NodeRegistry.sol:L602-L609

function removeNode(uint _nodeIndex) internal {
    // trigger event
    emit LogNodeRemoved(nodes[_nodeIndex].url, nodes[_nodeIndex].signer);
    // deleting the old entry
    delete urlIndex[keccak256(bytes(nodes[_nodeIndex].url))];
    uint length = nodes.length;

    assert(length > 0);

• NodeRegistry.registerNodeFor - Signature version v should be checked to be either 27 || 28 before verifying it.

code/in3-contracts/contracts/NodeRegistry.sol:L200-L212

function registerNodeFor(
    string calldata _url,
    uint64 _props,
    uint64 _timeout,
    address _signer,
    uint64 _weight,
    uint8 _v,
    bytes32 _r,
    bytes32 _s
)
    external
    payable
{

• NodeRegistry.revealConvict - unchecked signer

code/in3-contracts/contracts/NodeRegistry.sol:L321-L321

SignerInformation storage si = signerIndex[_signer];

• NodeRegistry.revealConvict - signer status can be checked earlier.

code/in3-contracts/contracts/NodeRegistry.sol:L344-L344

require(si.stage != Stages.Convicted, "node already convicted");

• NodeRegistry.updateNode - the check if the newURL is registered can be done earlier

code/in3-contracts/contracts/NodeRegistry.sol:L444-L444

require(!urlIndex[newURl].used, "url is already in use");

Recommendation

Use Checks-Effects-Interactions pattern for all functions.

Description

It is assumed that a blockhash of 0x00 is invalid, but the method accepts intermediary parent hashes extracted from blockheaders that are zero when establishing the trust chain.

recreateBlockheaders relies on reCalculateBlockheaders to correctly establish a chain of trust from the provided list of _blockheaders to a valid blockhash stored in the contract. However, reCalculateBlockheaders fails to raise an exception in case getParentAndBlockhash returns a blockhash of 0x00. Subsequently it will skip over invalid blockhashes and continue to establish the trust chain without raising an error.

This may allow an attacker with enough hashing power to store a blockheader hash that is actually invalid on the real chain but accepted within this smart contract. This may even only be done temporarily to overwrite an existing hash for a short period of time (see https://github.com/ConsenSys/slockit-in3-audit-2019-09/issues/24).

code/in3-contracts/contracts/BlockhashRegistry.sol:L141-L147

for (uint i = 0; i < _blockheaders.length; i++) {
    (calcParent, calcBlockhash) = getParentAndBlockhash(_blockheaders[i]);
    if (calcBlockhash != currentBlockhash) {
        return 0x0;
    }
    currentBlockhash = calcParent;
}

Recommendation

Stop processing the array of _blockheaders immediately if a blockheader is invalid.

Description

The method is used to re-create blockhashes from a list of rlp-encoded _blockheaders. However, the method never checks if _blockheaders actually contains items. The result is, that the method will unnecessarily store the same value that is already in the blockhashMapping at the same location and wrongly log LogBlockhashAdded even though nothing has been added nor changed.

• 1. assume _blockheaders is empty and the registry already knows the blockhash of _blockNumber

code/in3-contracts/contracts/BlockhashRegistry.sol:L61-L67

function recreateBlockheaders(uint _blockNumber, bytes[] memory _blockheaders) public {

    bytes32 currentBlockhash = blockhashMapping[_blockNumber];
    require(currentBlockhash != 0x0, "parentBlock is not available");

    bytes32 calculatedHash = reCalculateBlockheaders(_blockheaders, currentBlockhash);
    require(calculatedHash != 0x0, "invalid headers");

• 2. An attempt is made to re-calculate the hash of an empty _blockheaders array (also passing the currentBlockhash from the registry)

code/in3-contracts/contracts/BlockhashRegistry.sol:L66-L66

bytes32 calculatedHash = reCalculateBlockheaders(_blockheaders, currentBlockhash);

• 3. The following loop in reCalculateBlockheaders is skipped and the currentBlockhash is returned.

code/in3-contracts/contracts/BlockhashRegistry.sol:L134-L149

function reCalculateBlockheaders(bytes[] memory _blockheaders, bytes32 _bHash) public pure returns (bytes32 bhash) {

    bytes32 currentBlockhash = _bHash;
    bytes32 calcParent = 0x0;
    bytes32 calcBlockhash = 0x0;

    /// save to use for up to 200 blocks, exponential increase of gas-usage afterwards
    for (uint i = 0; i < _blockheaders.length; i++) {
        (calcParent, calcBlockhash) = getParentAndBlockhash(_blockheaders[i]);
        if (calcBlockhash != currentBlockhash) {
            return 0x0;
        }
        currentBlockhash = calcParent;
    }

    return currentBlockhash;

• 4. The assertion does not fire, the bnr to store the calculatedHash is the same as the one initially provided to the method as an argument.. Nothing has changed but an event is emitted.

code/in3-contracts/contracts/BlockhashRegistry.sol:L69-L74

    /// we should never fail this assert, as this would mean that we were able to recreate a invalid blockchain
    assert(_blockNumber > _blockheaders.length);
    uint bnr = _blockNumber - _blockheaders.length;
    blockhashMapping[bnr] = calculatedHash;
    emit LogBlockhashAdded(bnr, calculatedHash);
}

Recommendation

The method is crucial for the system to work correctly and must be tightly controlled by input validation. It should not be allowed to overwrite an existing value in the contract (issue 6.29) or emit an event even though nothing has happened. Therefore validate that user provided input is within safe bounds. In this case, that at least one _blockheader has been provided. Validate that _blockNumber is less than block.number and do not expect that parts of the code will throw and safe the contract from exploitation.

Description

When the owner calls updateNode() function providing a new url for the node, the signer of the url is replaced by msg.sender which in this case is the owner of the node. Note that new URL can resolve to the same URL as before (See https://github.com/ConsenSys/slockit-in3-audit-2019-09/issues/36).

code/in3-contracts/contracts/NodeRegistry.sol:L438-L452

if (newURl != keccak256(bytes(node.url))) {

    // deleting the old entry
    delete urlIndex[keccak256(bytes(node.url))];

    // make sure the new url is not already in use
    require(!urlIndex[newURl].used, "url is already in use");

    UrlInformation memory ui;
    ui.used = true;
    ui.signer = msg.sender;
    urlIndex[newURl] = ui;
    node.url = _url;
}

Furthermore, the method emits a LogNodeRegistered event when the node structure is updated. However, the event will always emit msg.sender as the signer even though that might not be true. For example, if the url does not change, the signer can still be another account that was previously registered with registerNodeFor and is not necessarily the owner.

code/in3-contracts/contracts/NodeRegistry.sol:L473-L478

emit LogNodeRegistered(
    node.url,
    _props,
    msg.sender,
    node.deposit
);

code/in3-contracts/contracts/NodeRegistry.sol:L30-L30

event LogNodeRegistered(string url, uint props, address signer, uint deposit);

Recommendation

• The updateNode() function gets the signer as an input used to reference the node structure and this signer should be set for the UrlInformation.

function updateNode(
        address _signer,
        string calldata _url,
        uint64 _props,
        uint64 _timeout,
        uint64 _weight
    )

• The method should actually only allow to change node properties when owner==signer otherwise updateNode is bypassing the strict requirements enforced with registerNodeFor where e.g. the url needs to be signed by the signer in order to register it.
• The emitted event should always emit node.signer instead of msg.signer which can be wrong.
• The method should emit its own distinct event LogNodeUpdated for audit purposes and to be able to distinguish new node registrations from node structure updates. This might also require software changes to client/node implementations to listen for node updates.

Description

NodeRegistry In3Node memory n is never used inside the modifier onlyActiveState.

code/in3-contracts/contracts/NodeRegistry.sol:L125-L133

modifier onlyActiveState(address _signer) {

    SignerInformation memory si = signerIndex[_signer];
    require(si.stage == Stages.Active, "address is not an in3-signer");

    In3Node memory n = nodes[si.index];
    assert(nodes[si.index].signer == _signer);
    _;
}

Recommendation

Use n in the assertion to access the node signer assert(n.signer == _signer); or directly access it from storage and avoid copying the struct.

Description

Important state changing functions should emit an event for the purpose of having an audit trail and being able to monitor the smart contract usage and performance.

Recommendation

Emit events for returnDeposit and transferOwnership.

Description

in3_stat shows information from node activities in the currentMonth, currentDay, currentHour which can result in leaking information about the functionality that node is being used for. This information might be valuable when an attacker wants to find out how utilized a node is and if any reflection attacks are successful (https://github.com/ConsenSys/slockit-in3-audit-2019-09/issues/50).

Examples

{
    'profile': AttributeDict({
        'name': 'Slockit2',
        'icon': 'https://slock.it/assets/slock_logo.png',
        'url': 'https://slock.it'
    }),
    'stats': Attr
    ibuteDict({
        'upSince': 1568400626355,
        'currentMonth': AttributeDict({
            'requests': 47618,
            'lastRequest': 1569422025347,
            'methods': AttributeDict({
                'nd-2': 2,
                'eth_call': 940,
                'eth_blockNumber': 25,
                'eth_getBlockByNumber': 45395,
                'web3_clientVersion': 386,
                'admin_datadir': 7,
                'admin_peers': 11,
                'shh_version': 7,
                'shh_info': 14,
                'admin_nodeInfo ': 9, '
                txpool_status ': 9, '
                personal_listAccounts ': 3, '
                eth_chainId ': 12, '
                eth_protocolVersion ': 6, '
                net_listening ': 6, '
                net_peerCount ': 6, '
                eth_syncing ': 6,
                'eth_mining': 6,
                'eth_hashrate': 6,
                'eth_gasPrice': 18,
                'eth_coinbase': 44,
                'eth_accounts': 54,
                'eth_getBalance': 321,
                'personal_unlockAccount': 61,
                'personal_
                importRawKey ': 5, '
                personal_newAccount ': 8, '
                eth_estimateGas ': 16, '
                eth_sendRawTransaction ': 9, '
                eth_getTransactionReceipt ': 49, '
                in3_sign ': 59, '
                eth_getCode ': 33,
                'eth_getTransactionCount': 15,
                'eth_getLogs': 8,
                'in3_stats': 16,
                'in3_validatorlist': 15,
                'in3_nodeList': 15,
                'in3_call': 15,
                'proof_in3_sign': 1
            })
        }),
        'currentDay': AttributeDict({
            'requests': 144,
            'lastRequest': 1569422025347,
            'methods': AttributeDict({
                'eth_getBlockByNumber': 135,
                'web3_clientVersion': 6,
                'eth_coinbase': 1,
                'eth_accounts': 1,
                'in3_stats': 1
            })
        }),
        'currentHour': AttributeDict({
            'requests': 144,
            'lastRequest': 1569422025346,
            'methods': AttributeDict({
                'eth_getBlockByNumber': 135,
                'web3_clientVersion': 6,
                'eth_coinbase': 1,
                'eth_accounts': 1,
                'in3_stats': 1
            })
        })
    })
}

Recommendation

Make sure if this information is needed, if not enable it just for debugging purposes.

Description

removeNode removes a node from the Nodes array. This is done by copying the last node of the array to the _nodeIndex of the node that is to be removed. Finally the node array size is decreased.

A Node’s index is also referenced in the SignerInformation struct. This index needs to be adjusted when removing a node from the array as the last node is copied to the index of the node that is to be removed.

When adjusting the Node’s index in the SignerInformation struct removeNode casts the index to uint64. This is both unnecessary as the struct defines the index as uint and theoretically dangerous if a node at an index greater than uint64_max is removed. The resulting SignerInformation index will be truncated to uint64 leading to an inconsistency in the contract.

code/in3-contracts/contracts/NodeRegistry.sol:L60-L69

struct SignerInformation {
    uint64 lockedTime;                  /// timestamp until the deposit of an in3-node can not be withdrawn after the node was removed
    address owner;                      /// the owner of the node

    Stages stage;                       /// state of the address

    uint depositAmount;                 /// amount of deposit to be locked, used only after a node had been removed

    uint index;                         /// current index-position of the node in the node-array
}

code/in3-contracts/contracts/NodeRegistry.sol:L614-L620

// move the last entry to the removed one.
In3Node memory m = nodes[length - 1];
nodes[_nodeIndex] = m;

SignerInformation storage si = signerIndex[m.signer];
si.index = uint64(_nodeIndex);
nodes.length--;

Recommendation

Do not cast and therefore truncate the index.

Description

• NodeRegistry - check for addr(0) being passed. This is anyway only done in the constructor and will not require a lot of gas.

code/in3-contracts/contracts/NodeRegistry.sol:L138-L139

constructor(BlockhashRegistry _blockRegistry) public {
    blockRegistry = _blockRegistry;

• NodeRregistry - unnecessary payable

code/in3-contracts/contracts/NodeRegistry.sol:L535-L535

address payable _owner,

• NodeRegistry - deposit checks can be combined into one function to make the code more readable. The min deposit amount could be exposed as public const to allow other entities to query the contracts minimum deposit similar to the max ether amount. MAX_ETHER_LIMIT should make clear that this limit is only applicable in the first year (e.g. MAX_ETHER_LIMIT_FIRST_YEAR) .

code/in3-contracts/contracts/NodeRegistry.sol:L543-L545

require(_deposit >= 10 finney, "not enough deposit");

checkNodeProperties(_deposit, _timeout);

code/in3-contracts/contracts/NodeRegistry.sol:L120-L120

uint constant internal MAX_ETHER_LIMIT = 50 ether;

• NodeRegistry - require(si.owner == msg.sender) can be checked before accessing the nodes array

code/in3-contracts/contracts/NodeRegistry.sol:L402-L404

SignerInformation storage si = signerIndex[_signer];
In3Node memory n = nodes[si.index];
require(si.owner == msg.sender, "only for the in3-node owner");

• NodeRegistry - removeNode resets index to a valid node array index of 0. Even though the code will access the index it is good practice to set this to an invalid value to make sure it raises an error condition if it is wrongly accessed in a future revision of the code. This is mainly a safeguard. Another option is to invalidate the 0 index.

code/in3-contracts/contracts/NodeRegistry.sol:L612-L612

signerIndex[nodes[_nodeIndex].signer].index = 0;

• NodeRegistry - implicitly set defaults are hard to maintain. This should be a constant state variable that can be queried to be transparent about minimum and maximum values. Prefer throwing an exception instead of automatically setting the value to a minimum as this might be unexpected by a client and can cover error conditions.

code/in3-contracts/contracts/NodeRegistry.sol:L565-L565

m.timeout = _timeout > 1 hours ? _timeout : 1 hours;

• NodeRegistry - one year startup period: instead of storing the deployment timestamp the contract should store the end-of-admin-timestamp.

code/in3-contracts/contracts/NodeRegistry.sol:L256-L256

require(block.timestamp < (blockTimeStampDeployment + YEAR_DEFINITION), "only in 1st year");// solhint-disable-line not-rely-on-time

• NodeRegistry - inefficient re-calculation of hash

code/in3-contracts/contracts/NodeRegistry.sol:L438-L441

if (newURl != keccak256(bytes(node.url))) {

    // deleting the old entry
    delete urlIndex[keccak256(bytes(node.url))];

• NodeRegistry - weight should be part of proofHash

code/in3-contracts/contracts/NodeRegistry.sol:L490-L502

function calcProofHash(In3Node memory _node) internal pure returns (bytes32) {

    return keccak256(
        abi.encodePacked(
            _node.deposit,
            _node.timeout,
            _node.registerTime,
            _node.props,
            _node.signer,
            _node.url
        )
    );
}

• NodeRegistry - updateNode if the new timeout is smaller than the current timeout it will silently be ignored. This may be unexpected by the caller and cover error conditions where a client provides wrong inputs. Raising an exception should be preferred in such cases instead of gracefully assuming values.

code/in3-contracts/contracts/NodeRegistry.sol:L463-L465

if (_timeout > node.timeout) {
    node.timeout = _timeout;
}

• NodeRegistry - admin functionality should be clearly named as such for transparency reasons (e.g. adminRemovenodeFromRegistry) .

Description

The following code can be optimized by removing mload and mstore:

code/in3-contracts/contracts/BlockhashRegistry.sol:L106-L125

require(first > 0xf7, "invalid offset");
uint8 offset = first - 0xf7 + 2;

/// we are using assembly because it's the most efficent way to access the parent blockhash within the rlp-encoded blockheader
// solium-disable-next-line security/no-inline-assembly
assembly { // solhint-disable-line no-inline-assembly
    // mstore to get the memory pointer of the blockheader to 0x20
    mstore(0x20, _blockheader)

    // we load the pointer we just stored
    // then we add 0x20 (32 bytes) to get to the start of the blockheader
    // then we add the offset we calculated
    // and load it to the parentHash variable
    parentHash :=mload(
        add(
            add(
                mload(0x20), 0x20
            ), offset)
    )
}

Recommendation

assembly { // solhint-disable-line no-inline-assembly
            // mstore to get the memory pointer of the blockheader to 0x20
            //mstore(0x20, _blockheader)  //@audit should assign 0x20ptr to variable first and use it.

            // we load the pointer we just stored
            // then we add 0x20 (32 bytes) to get to the start of the blockheader
            // then we add the offset we calculated
            // and load it to the parentHash variable
            parentHash :=mload(
                add(
                    add(
                        _blockheader, 0x20
                    ), offset)
            )
        }

Description

The smart contracts enable experimental compiler features. Please note that these features are experimental for a reason and should be avoided unless explicitly required.

code/in3-contracts/contracts/BlockhashRegistry.sol:L21-L21

pragma experimental ABIEncoderV2;

Seems that NodeRegistry does not require any ABIEncoderV2 specific functionality.

code/in3-contracts/contracts/NodeRegistry.sol:L21-L21

pragma experimental ABIEncoderV2;

Description

There are different levels of trust attached to blockhashes stored in the BlockhashRegistry. On one side there are blockhashes which data-source is the evm ( blockhash(blocknumber)) and on the other side there are blockhashes that have been fed into the system by recalculating block-headers and establishing a trust chain to an already existing blockhash in the contract. While the contract can trust the result of blockhash(blocknumber) for the most recent 256 blocks because the information is coming directly from the evm, blockhashes that are re-created by calling recreateBlockheaders are manually verified and trust relies on the proper validation of the chain of block-headers provided.

Side-effect: Also saves gas by avoiding unnecessary calculations within the recreateBlockheaders() codepath as blockhash is already available via evm.

Recommendation

recreateBlockheaders() should prefer to use blockhash(number) by calling saveBlockNumber() instead of re-calculating the blockhash from the user provided chain of blockheaders, if the blockhash can easily be accessed by the evm (most recent 256 blocks, except current block). Check if _blockheaders.length > 0 && _blockNumber.sub(_blockheaders.length) < block.number-256.

Description

Last 256 blocks, that are available in the EVM environment, are stored in BlockhashRegistry by calling snapshot() or saveBlockNumber(uint _blockNumber) functions. Older blocks are recreated by calling recreateBlockheaders.

The methods will overwrite existing blockhashes.

code/in3-contracts/contracts/BlockhashRegistry.sol:L79-L87

function saveBlockNumber(uint _blockNumber) public {

    bytes32 bHash = blockhash(_blockNumber);

    require(bHash != 0x0, "block not available");

    blockhashMapping[_blockNumber] = bHash;
    emit LogBlockhashAdded(_blockNumber, bHash);
}

code/in3-contracts/contracts/BlockhashRegistry.sol:L72

blockhashMapping[bnr] = calculatedHash;

Recommendation

If a block is already saved in the smart contract, it can be checked and a SSTORE can be prevented to save gas. Require that blocknumber hash is not stored.

require(blockhashMapping[_blockNumber] == 0x0, "block already saved");