Slock.it Incubed3 Audit

1 Summary

ConsenSys Diligence conducted a security audit on Slock.it’s Trustless Incentivized Remote Node Network, in short INCUBED. Incubed is to solve the issue of connecting IoT devices to the blockchain (Ethereum). Many IoT devices are severely limited in terms of computing capacity, connectivity and often also power supply. Connecting an IoT device to a remote node enables even low-performance devices to be connected to blockchain.

2 Audit Scope

The audit scope is defined as follows:

  1. The Incubed Algorithm

    • Review the specification for security considerations
    • Checking against the provided Threat Model
    • Prepare a report with additional threats and an adapted risk assessment of the currently identified
  2. Smart Contract Audit of the in3-contracts

    • Validating the correct implementation of the specification.
    • Assuring that no funds can be lost.
    • Identifying other vulnerabilities and unexpected behaviour
  3. Review of the Incubed Node RPC extension to verify that

    • The private keys are managed securely
    • The node never signs an invalid blockhash (which would lead to loss of funds)

This audit covered the contracts included in in3-contracts (Commit hash: e25c758a115aef0c0640bc446027259aa7cb1a52) and the client code included in in3-server (Commit hash: fdc5eb104c4c3aba0af79222d54e6effaef6f3a7) repositories. The contracts located in the in3-server repository are out of scope for the Smart Contract Audit. The client implementation and an in3-server code audit is not in scope for the review.

List of the files covered in this audit:

File SHA-1 hash
in3-contracts/contracts/NodeRegistry.sol 85a9632c9e8e41057ae11d772c623a0fb92af7f8
in3-contracts/contracts/BlockhashRegistry.sol 1818e466573344fbf826fd144b96a2c6ef4f786e

Directories covered for the Incubed Node RPC extension review:

File Main Files
in3-server/src server.ts, rpc.ts, EthHandler.ts, BaseHandler.ts, signatures.ts

A detailed audit of the provided proofs was not in scope for this audit.

Update (December 2, 2019): It should be noted that the IN3 architecture has changed since the original audit in September 2019. This report does not extensively reflect these changes and was written based on the original submitted code. The new changes, including, but not limited to:

The audit team evaluated that the system is secure, resilient, and working according to its specifications. The audit activities can be grouped into the following three broad categories:

  1. Security: Identifying security related issues within the contract.
  2. Architecture: Evaluating the system architecture through the lens of established smart contract best practices.
  3. Code quality: A full review of the contract source code. The primary areas of focus include:
    • Correctness
    • Readability
    • Scalability
    • Code complexity
    • Quality of test coverage

2.1 Documentation

The following documentation was available to the project team:

3 System Overview

The high level architecture of the Incubed network consists of the following three layers. Each layer is described in more detail in this section.

slockit-tm

3.1 in3-Registries

Two Smart-Contracts build the backbone of the system - NodeRegistry and BlockhashRegistry. They are deployed on the Ethereum blockchain and provide the following functionalities:

NodeRegistry

This is the central node registry of the Incubed system. Clients start up and initially contact a hardcoded list of boot-nodes. in3-nodes have hardcoded list of the registry contracts and can provide clients with information about registered nodes.

NodeRegistry

BlockhashRegistry

The ethereum virtual machine (evm) can only provide the blockhash of the most recent 256 blocks, excluding the current block. The ethereum block times vary and are typically within 10s and 20s. This means that the evm can provide blockhashes back in time for only around 1 hour. This means that nodes would only be able to convict rogue nodes that return wrong blockhashes within that window. In order to tackle this problem and allow nodes to convict even older responses for blockhashes the system provides a BlockhashRegistry.

The only incentive for someone to actually spend gas on adding new blockhashes to the BlockhashRegistry is to convict a node in the NodeRegistry. In case a participant attempts to convict a signer for a blockhash that cannot be provided by the evm, the NodeRegistry will call out to BlockhashRegistry and check if the blockhash is available there. If it is not available in the BlockhashRegistry the signer cannot be convicted. In order to successfully prove that the signer signed a wrong blockhash and is to be convicted - burning half of the signer’s deposit and awarding the other half to the convictor - the convictor has to provide the correct blockhash to the BlockhashRegistry. Adding new blockhashes is not trivial and requires the convictor to provide a chain of rlp-encoded blockheaders up to an already trusted blockhash in the registry (trust anchor).

The registry provides the following functionality:

BlockhashRegistry

Inheritance

inheritance

[dot file]

Call Graph

graph

[dot file]

3.2 in3-Nodes

This layer is a network of in3-nodes that can communicate with each other via an extension to the Ethereum RPC protocol. in3-nodes facilitate access to ethereum blockchain nodes by transparently proxying Ethereum RPC interface to other in3-nodes.

The node-to-node and client-to-node communication follows a request-response schema. A client (or node) can use a node to get access to the ethereum blockchain using the Ethereum RPC interface. An in3-node extends the Ethereum RPC interface with the following methods:

In addition to that, merkle-proofs can be requested for specific Ethereum RPC calls:

Only blockhashes are signed by nodes when they are requested to do so by calling in3_sign. Proofs are not signed but can contain a signed blockhash. Unhonest nodes can only be convicted for signing wrong blockhashes.

3.3 in3-Clients

  1. The in3-network layer - A network of in3-server nodes that can communicate with each other via an extended Ethereum RPC protocol, providing access to Ethereum blockchain data.
  2. The in3-client layer - Further information about the general concept and architecture can be found here.

The in3-Client is not in scope for this audit.

4 Key Observations/Recommendations

3.1 in3-Registries

4.2 in3-server

5 Security Specification

This section describes, from a security perspective, the expected behavior of the system under audit. It is not a substitute for documentation. The purpose of this section is to identify specific security properties that were validated by the audit team.

slockit-tm

5.1 Actors

The relevant actors are as follows:

5.2 Trust Model

In any smart contract system, it’s important to identify what trust is expected/required between various actors. For this audit, we established the following trust model:

5.3 Threat Model

This section is designed to discuss threat model prepared by Slock.it team, Thread Model for Incubed(src). This threat model is not complete without considering many other concerns mentioned in other sections such as, Key Observations/Recommendations, Trust Model, and Important Security Properties.

Component Title Details Status Notes
Nodes Long Time Attack see doc open Using bootstrap nodes mitigates this issue. However, if an old node comes back online, it might not have access to new bootstrap nodes to connect to. Also the system is vulnerable to compromised bootstrap nodes.
Registry Inactive Server Spam Attack see doc partially addressed Solutions such as Dynamic Min Deposit and Voting that are proposed will result in a more secure implementation.
Registry Self-Convict Attack see doc closed The solution used is to burn 50% of the deposit. However depending on the use case, the attacked client could result in more profit for attacker than 50% of the deposit, hence still profitable to sign a wrong block hash and self-convict. Also if someone convicts your node, you can self convict and get at least 50% back.
Registry Convict Frontrunner Attack see doc closed The implemented solution, obfuscates the convict work flow. In order to convict:
• if the blockNumber is in last 256: the solution almost covers most cases, however the blocknumber and msg.sender will be known in the commit time, this information leakage can result in different front-running attacks.
• if blockNumber is older than 256 blocks, user must first update blockHashRegistry which signals to everyone in the network about a possible conviction, considering the only incentive for external parties to update blocks is to get half of the convicted deposits, this could be more profitable for other network participants.
Network Blacklist Attack see doc partially addressed
Network DDoS Attacks see doc closed No DoS protection is implemented in in3-server. It is unrealistic to expect each node to buy DoS protection plans.
Network None Verifying Data Provider see doc closed Unclear threat description. Adding a chain of signatures can mitigate possible issues, however assurance of client’s proper checking of the signatures is a crucial part of this implementation.
Privacy Private Keys as API Keys see doc closed Unclear threat description
Privacy Filtering of Nodes see doc partially addressed Not implemented yet. (Nodes can be filtered in the bootstrapping layer)
Privacy Inspecting Data in Relays or Proxies see doc open Not Implemented. Enforce HTTPS and Proper implementation and verification of PKI is required on both ends.
Risk Risk Calculations see doc open

5.4 Important Security Considerations

The following is a non-exhaustive list of security properties, concerns and threats that were verified in this audit:

Smart Contracts

in3-node layer

6 Issues

Each issue has an assigned severity:

6.1 in3-server - amplified DDoS on incubed requests on proof with signature Critical ✓ Fixed

Resolution

Mitigated by adding maxBlocksSigned and maxSignatures for requests of any client. “The Numbers of signatures a client can ask to fetch is now limited to maxSignatures which defaults to 5” in merge_requests/101. The full extent of this fix is outside the scope of this audit.

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.

6.2 BlockProof - Node conviction race condition may trick all but one node into losing funds Critical ✓ Fixed

Resolution

Mitigated by:

  • 80bb6ecf by checking if blockhash exists and prevent an overwrite, saving gas
  • Client will blacklist the server if the signature is missing, has a wrong signer or is invalid.
  • 6cc0dbc0 Removing nodes from local available nodes list when the server detects wrong responses
  • Other commits to mitigate the mentioned vulnerable scenarios

With the new handling, the client will not call convict immediately (as this could be exploited again). Instead, the client will do the calculation whether it’s worth convicting the server before even calling convict.

It should be noted that the changes are scattered and modified in the final source code, and this behaviour of IN3-server code is outside the scope of this audit.

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 issue 6.29).

6.3 NodeRegistry Front-running attack on convict() Critical ✓ Fixed

Resolution

Blocknumber is removed from convict function, which removes any signal for an attacker in the scenario provided. However, the order of the transactions to convict a wrong signed hash is necessary to prevent any front-running attacks: 1. Convict(_Blockhash) 2. recreate Blockheaders 3. RevealConvict (minimum 2 blocks after convict but as soon as recreateBlockheaders is confirmed)

The fixes were introduced in ecf2c6a6 and f4250c9a, although later on NodeRegistry contract was split in two other contracts NodeRegistryLogic and NodeRegistryData and further changes were done in the conviction flow in different commits.

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.

6.4 NodeRegistry - URL can be arbitrary dns resolvable names, IP’s and even localhost or private subnets Major ✓ Fixed

Resolution

This issue has been addressed with the following commits:

  • 4c93a10f adding 48 hours delay in the server code before they communicate with the newly registered nodes.
  • merge_requests/111 adding a whole new smart contract to the IN3 system, IN3WhiteList.sol, and supporting code in the server.
  • issues/94 To prevent attacker to use nodes as a DoS network, a DNS record verification is discussed to be implemented.

It is a design decision to base the Node registry on URLs (DNS resolvable names). This has the implications outlined in this issue and they cannot easily be mitigated. Adding a delay until nodes can be used after registration only delays the problem. Assuming that an entity curates the registry or a whitelist is in place centralizes the system. Adding DNS record verification still allows an owner of a DNS entry to point its name to any IP address they would like it to point to. It certainly makes it harder to add RPC URLs with DNS names that are not in control of the attacker but it also adds a whole lot more complexity to the system (including manual steps performed by the node operator). In the end, the system allows IP based URLs in the registry which cannot be used for DNS validation.

Note that the server code changes, and the new smart contract IN3WhiteList.sol are outside the scope of the original audit. We strongly recommend to reduce complexity and audit the final codebase before mainnet deployment.

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 (issue 6.23), 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.

6.5 Malicious clients can use forks or reorgs to convict honest nodes Major  Won't Fix

Resolution

Default value for past signed blocks is changed to 10 blocks. Slockit plans to use their off-chain channels to notify clients for planned forks. They also looking into using fork oracles in the future releases to detect planned hardforks to mitigate risks.

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.

6.6 in3-server - should protect itself from abusive clients Major ✓ Fixed

Resolution

Slockit implemented their own DOS protection for incubed server in merge_requests/99. The variant of this implementation adds more complexity to the code base. The benchmark and testing of the new DOS protection is not in scope for this audit.

The incubed server has now an additional DOS-Protection build in. Here we first estimate a Weight of such a request and add them together for all incoming requests per IP of the client per Minute. Since we estimate the execution, we can prevent a client running DOS-Attacks from the same IP with heavy requests (such as eth_getLogs)

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, issue 6.1).

6.7 in3-server - DoS on in3.sign and other requests Major ✓ Fixed

Resolution

Similar to issue 6.1, Mitigated by adding maxBlocksSigned and maxSignatures for requests of any client. “The Numbers of signatures a client can ask to fetch is now limited to maxSignatures which defaults to 5” in merge_requests/101. The full extent of this fix is outside the scope of this audit.

We have limited the number of block you can ask to sign in the in3_sign-request. The default is 10, because this function is also used for eth_getLogs to provide proof for all events. This limit will also limit the result of logs returned to include only max 10 different blocks.

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.

6.8 in3-server - key management Major  Pending

Resolution

The breakdown of the fixes addressed with git.slock.it/PR/13 are as follows:

  • Keys should never be stored or accepted in plaintext format Keys should only be accepted in an encrypted and protected format

The private key in code/in3-server/config.json has been removed. The repository still contains private keys at least in the following locations: - package.json - vscode/launch.json - example_docker-compose.yml

Note that private keys indexed by a git repository can be restored from the repository history.

The following statement has been provided to address this issue:

We have removed all examples and usage of plain private keys and replaced them with json-keystore files. Also in the documentation we added warnings on how to deal with keys, especially with hints to the bash history or enviroment


  • A single key should be used for only one purpose. Keys should not be shared.

The following statement has been provided to address this issue:

This is why we seperated the owner and signer-key. This way you can use a multisig to securly protect the owner-key. The signer-key is used to sign blocks (and convict) and is not able to do anything else (not even changing its own url)


  • 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

Addressed by wrapping the private key in an object that stores the key in encrypted form and only decrypts it when signing. The key is cleared after usage. The IN3-server still allows raw private keys to be configured. A warning is printed if that is the case. The loaded raw private key is temporarily assigned to a local variable and not explicitly cleared by the method.

While we used to keep the unlocked key as part of the config, we have now removed the key from the config and store them in a special signer-function.
https://git.slock.it/in3/ts/in3-server/merge_requests/113


  • 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

see previous remediation note.

After unlocking the signer key, we encrypt it again and keep it encrypted only decrypting it when signing. This way the raw private key only exist for a very short time in memory and will be filled with 0 right after. ( https://git.slock.it/in3/ts/in3-server/merge_requests/113/diffs#653b04fa41e35b55181776b9f14620b661cff64c_54_73 )


  • Use standard libraries for cryptographic operations

The following statement has been provided to address this issue

We are using ethereumjs-libs.


  • Use the system keystore and API to sign and avoid to store key material at all

The following statement has been provided to address this issue

We are looking into using different signer-apis, even supporting hardware-modules like HSMs. But this may happen in future releases.


  • The application should store the keys eth-address (util.getAddress()) instead of re-calculating it multiple times from the private key.

Fixed by generating the address for a private key once and storing it in a private key wrapper object.


  • Do not leak credentials and key material in debug-mode, to local log-output or external log aggregators.

txArgs still contains a field privateKey as outlined in the issue description. However, this privateKey now represents the wrapper object noted in a previous comment which only provides access to the ETH address generated from the raw private key.

The following statement has been provided to address this issue:

since the private key and the passphrase are actually deleted from the config, logoutputs or even debug will not be able to leak this information.

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.

6.9 NodeRegistry - Multiple nodes can share slightly different RPC URL Major ✓ Fixed

Resolution

Same mitigation as issue 6.4.

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.

6.10 in3-server - should enforce safe settings for minBlockHeight Medium  Won't Fix

Resolution

The default block is changed to 10 and minBlockHeight is added to the registry (as part of the properties) in 8c72633e, but allow the user to define a minBlockHeight lower than this number. The client is responsible to review the settings depending on how secure they want their nodes to be.

Client response:

We have discussed this, but decided to keep it flexible. This means: 1. We have put the minBlockHeight into the registry (as part of the properties). Because these properties indicate the limit and capabilities of the node and give the client a chance to filter out nodes if they don’t match the requirements. So each client is able to filter out node who are not willing to take the risk and sign for example latest-6. Of course these nodes will most likely only store a low deposit ( you can not have a signature of a young block and a high deposit), but if you need a high security the nodes with a deposit will propably wait at least 10 or more blocks. In order to protect the owner of a node of using insecure settings, we will use our wizard to check the deposit and minBlockHeights and warn or educate the user. The reason why this flexibility is important, is because there use cases where dapps will not accept the let user wait 10 blocks before confirming a transaction. If the dapp developer needs a signature of a younger block, he will need to live with the fact, that he won’t be able to find a high deposit to secure it. 2. We also changed the default to 10 blocks, but allow the user to define a minBlockHeight lower than this number. In this case the node would write a warning in the logfile, but still accepts the user configuration. This allows to use incubed also on different chains other than the mainnet. 3. The safeMinBlockHeight is now dependend on different chains, which is one single function, so we don’t have hardcoded values in different places anymore.

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.

6.11 in3-server - rpc proof handler specification inconsistency Medium ✓ Fixed

Resolution

Addressed with https://git.slock.it/in3/ts/in3-server/issues/100. Checks for proof and proofWithSignature are more strict now, never is not checked and assumed to be the default. Falling back to no security as a default is not considered best practice. signatures has been renamed to the more accurate name signers. The client now allows both signers and signatures and we suggest already to start planning to phase-out this ambiguity, strictly enforce the specified protocol and reduce special cases and complexity in future iterations.

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.

6.12 in3-server - hardcoded gas limit could result in failed transactions/requests Medium ✓ Fixed

Resolution

Fixed by using web3 eth_estimateGas in merge_requests/109 to dynamically price the gas according to the network state.

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)

6.13 in3-server - handleRecreation tries to recreate blockchain if no block is available to recreate it from Medium ✓ Fixed

Resolution

Mitigated by 502b5528 by falling back to using the current block in case searchForAvailableBlock returns 0. Costs can be zero, but cannot be negative anymore.

The behaviour of the IN3-server code is outside the scope of this audit. However, while verifying the fixes for this specific issue it was observed that the watch.ts:handleConvict() relies on a static hardcoded cost calculation. We further note that the cost calculation formula has an error and is missing parentheses to avoid that costs can be zero. We did not see a reason for the costs not to be allowed to be zero. Furthermore, costs are calculated based on the difference of the conviction block to the latest block. Actual recreation costs can be less if there is an available block in blockhashRegistry to recreate it from that is other than the latest block.

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.

6.14 Impossible to remove malicious nodes after the initial period Medium ✓ Fixed

Resolution

This issue has been addressed with a large change-set that splits the NodeRegistry into two contracts, which results in a code flow that mitigates this issue by making the logic contract upgradable (after 47 days of notice). The resolution adds more complexity to the system, and this complexity is not covered by the original audit. Splitting up the contracts has the side-effect of events being emitted by two different contracts, requiring nodes to subscribe to both contracts’ events.

The need for removing malicious nodes from the registry, arises from the design decision to allow anyone to register any URL. These URLs might not actually belong to the registrar of the URL and might not be IN3 nodes. This is partially mitigated by a centralization feature introduced in the mitigation phase that implements whitelist functionality for adding nodes.

We generally advocate against adding complexity, centralization and upgrading mechanisms that can allow one party to misuse functionalities of the contract system for their benefit (e.g. adminSetNodeDeposit is only used to reset the deposit but allows the Logic contract to set any deposit; the logic contract is set by the owner and there is a 47 day timelock).

We believe the solution to this issue, should have not been this complex. The trust model of the system is changed with this solution, now the logic contract can allow the admin a wide range of control over the system state and data.

The following statement has been provided with the change-set:

During the 1st year, we will keep the current mechanic even though it’s a centralized approach. However, we changed the structure of the smart contracts and separated the NodeRegistry into two different smart contracts: NodeRegistryLogic and NodeRegistryData. After a successful deployment only the NodeRegistryLogic-contract is able to write data into the NodeRegistryData-contract. This way, we can keep the stored data (e.g. the nodeList) in the NodeRegistryData-contract while changing the way the data gets added/updated/removed is handled in the NodeRegistryLogic-contract. We also provided a function to update the NodeRegistryLogic-contract, so that we are able to change to a better solution for removing nodes in an updated contract.

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).

6.15 NodeRegistry.registerNodeFor() no replay protection and expiration Medium  Won't Fix

Resolution

This issue was addressed with the following statement:

In our understanding of the relationship between node-owner and signer the owner both are controlled by the very same entity, thus the owner should always know the privateKey of the signer. With this in mind a replay-protection would be useless, as the owner could always sign the necessary message. The reason why we separated the signer from the owner was to enable the possibility of owning an in3-node as with a multisig-account, as due to the nature of the exposal of the signer-key the possibility of it being leaked somehow is given (e.g. someone “hacks” the server), making the signer-key more unsecure. In addition, even though it’s possible to replay the register as an owner it would unfeasable, as the owner would have to pay for the deposit anyway thus rendering the attack useless as there would be no benefit for an owner to do it.

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.

6.16 BlockhashRegistry - Structure of provided blockheaders should be validated Medium ✓ Fixed

Resolution

Mitigated by:

  • 99f35fce - validating the block number in the provided RLP encoded input data
  • 79e5a302 - fixes the potential out of bounds access for parentHash by requiring the the input to contain at least data up until including the parentHash in the user provided RLP blob. However, this check does not enforce that the minimum amount of data is available to extract the blockNumber

Additionally we would like to note the following: * While the code decodes the RLPLongList structure that contains the blockheader fields it does not decode the RLPLongString parentHash and just assumes one length-byte for it. * The length of the RLPLongString parentHash is never used but skipped instead. * The decoding is incomplete and fragile. The method does not attempt to decode other fields in the struct to verify that they are indeed valid RLP data. For the blockNumber extraction a fixed offset of 444 is assumed to access the difficulty RLP field (this might through as the minimum input length up to this field is not enforced). difficulty is then skipped and the blockNumber is accessed. * The minimum input data length enforced is shorter than a typical blockheader. * The code relies on implicit exceptions for out of bounds array access instead of verifying early on that enough input bytes are available to extract the required data.

We would also like to note that the commit referenced as mitigation does not appear to be based on the audit code.

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.

6.17 Registries - Incomplete input validation and inconsistent order of validations Medium  Pending

Resolution

This issue describes general inconsistencies of the smart contract code base. The inconsistencies have been addressed with multiple change-sets:

Issues that have been addressed by the development team:

  • BlockhashRegistry.reCalculateBlockheaders - bhash can be zero; blockheaders can be empty

    Fixed in 8d2bfa40 by adding the missing checks.

  • BlockhashRegistry.recreateBlockheaders - blockheaders can be empty; Arguments should be validated before calculating values that depend on them.

    Fixed in 8d2bfa40 by adding the missing checks.

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

    Fixed in 47255587 by adding the missing checks.

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

    The fix in 47255587 introduced a serious typo (v != _v) that has been fixed with 4a0377c5 .

  • NodeRegistry.revealConvict - unchecked signer

    Addressed with the comment that signer gets checked by ecrecover (slock.it/issue/10).

  • NodeRegistry.revealConvict - signer status can be checked earlier. Addressed with the following comment (slock.it/issue/10):

Due to the seperation of the contracts we will now check check the signatures and whether the blockhash is right. Only after this steps we will call into the NodeRegistryData contracts, thus potentially saving gas

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

    Fixed in 4786a966.

  • BlockhashRegistry.getParentAndBlockhash- blockheader structure can be random as long as parenthash can be extracted

    This issue has been reviewed as part of issue 6.16 (99f35fce).

Issues that have not been addressed by the development team and still persist:

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

    This issue has not been addressed.

General Notes:

  • Ideally commits directly reference issues that were raised during the audit. During the review of the mitigations provided with the change-sets for the listed issues we observed that change-sets contain changes that are not directly related to the issues. (e.g. 79e5a302)

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.

6.18 BlockhashRegistry - recreateBlockheaders allows invalid parent hashes for intermediary blocks Medium ✓ Fixed

Resolution

Fixed by requiring valid parent hashes for blockheaders.

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 issue 6.29).

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.

6.19 BlockhashRegistry - recreateBlockheaders succeeds and emits an event even though no blockheaders have been provided Medium ✓ Fixed

Resolution

Fixed the vulnerable scenarios by adding proper checks to:

  • Prevent passing empty _blockheaders in 8d2bfa40
  • Prevent storing the same blockhash twice in 80bb6ecf

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.

6.20 NodeRegistry.updateNode replaces signer with owner and emits inconsistent events Medium ✓ Fixed

Resolution

Reviewed merged changes at in3-contracts/5cb54165. - The method now emits a distinct event twice when node properties are updated. - The event correctly emits the signer. - When updating a node URL, the new URLInformation now correctly sets the signer.

However, there is a discrepancy between the process of registering a node and updating node’s properties. When registering a node the owner has to provide a signed message containing the registration properties from the signer. Once the node is registered it can be unilaterally updated by the owner without requiring the signers permission to do so. According to slock.it it is assumed that the node owner and the signer are in control of the same entity and therefore this is not a concern.

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 issue 6.9).

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.

6.21 NodeRegistry - In3Node memory n is never used Minor ✓ Fixed

Resolution

Fixed by removing the modifier and move the node-signer check to functions in f1fd7943

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.

6.22 NodeRegistry - returnDeposit and transferOwnership should emit an event Minor ✓ Fixed

Resolution

Fixed in f1fd7943 by adding new events (LogDepositReturned, LogNodeRemoved, LogNodeConvicted, LogOwnershipChanged, LogNodeUpdated, LogNodeRegistered)

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.

6.23 in3-server - in3_stats leaks information Minor  Won't Fix

Resolution

There is a config-option -profile.noStats to deactivate stats, which defaults to True in the current release.

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 (issue 6.1).

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.

6.24 NodeRegistry - removeNode unnecessarily casts the nodeIndex to uint64 potentially truncating its value Minor ✓ Fixed

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.

6.25 Registries - general inconsistencies Minor  Pending

Resolution

The breakdown of the fixes are as follows:

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

The proper checks for registry addresses are added in 4786a966.

  • NodeRregistry - unnecessary payable

Removed payable modifier everywhere, as ERC20 support is added to the system. ERC20 support is not part of this audit.

  • 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) .

Fixed and variables renamed.

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

Added proper checks in c9e75b35.

  • 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.

Although the index is not set to 0, this issue is not yet fixed (Follow up here).

  • 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.

timeout has been removed, so this is obsolete as it is not in the new code anymore.

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

Fixed as recommended timestampAdminKeyActive = block.timestamp + YEAR_DEFINITION;

  • NodeRegistry - inefficient re-calculation of hash

Fixed (issues/16).

  • NodeRegistry - weight should be part of proofHash

Added in 9fa5548d.

  • 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.

Fixed by removing timeout variable (issues/16).

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

Renamed all admin function in both contracts with prefix admin.

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) .

6.26 BlockhashRegistry- assembly code can be optimized Minor ✓ Fixed

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)
            )
        }

6.27 Experimental Compiler features are enabled - ABIEncoderV2 Minor  Won't Fix

Resolution

This issue has been addressed with the following statement: > In order to pass structs between contracts we need that new ABIEncoder. [..] The old NodeRegistry did not require the ABIEncoderV2. [..] But due to the separation of the contracts in Logic and Data we are passing certain data-structures between contracts.

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;

6.28 BlockhashRegistry - recreateBlockheaders() should use the evm provided blockhash when applicable Minor  Pending

Resolution

The provided code-change at 79fa3ef1 is not addressing the raised concerns. As noted in the recommendation it is suggested to completely skip the recreation routine if the target blockhash (_blockNumber.sub(_blockheaders.length)) is available to the evm. The method should call saveBlockNumber(_blockNumber) instead.

The commit attempts to add a verification for extracted blockhashes from the user provided RLP data if the blockhash for the block is available. However, the variable name currentBlock is misleading making it hard to follow the authors intent.

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.

6.29 BlockhashRegistry - Existing blockhashes can be overwritten Minor ✓ Fixed

Resolution

Addressed with 80bb6ecf and 17d450cf by checking if blockhash exists and changing the assert to require.

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");

7 Tool-Based Analysis

Several tools were used to perform automated analysis of the reviewed contracts. These issues were reviewed by the audit team, and relevant issues are listed in the Issue Details section.

7.1 MythX

MythX is a security analysis API for Ethereum smart contracts. It performs multiple types of analysis, including fuzzing and symbolic execution, to detect many common vulnerability types. The tool was used for automated vulnerability discovery for all audited contracts and libraries. More details on MythX can be found at mythx.io.

Below is the raw output of the MythX vulnerability scan:

[
    {
        "issues": [
            {
                "swcID": "SWC-127",
                "swcTitle": "",
                "description": {
                    "head": "jump to arbitrary destination",
                    "tail": "A caller can trigger a jump to an arbitrary destination. Make sure this does not enable unintended control flow."
                },
                "severity": "High",
                "locations": [
                    {
                        "sourceMap": "20901:1:1",
                        "sourceType": "raw-bytecode",
                        "sourceFormat": "evm-byzantium-bytecode",
                        "sourceList": [
                            "0x63ad5e3d2c8551cf64f6d0425940efdeb79801907fcad157d1c82922919c13cb",
                            "0xd8d70c0998b3293c364b1cde922c80081d70d02726e74091c8aae6fa6a10b892"
                        ]
                    },
                    {
                        "sourceMap": "23263:248:-1",
                        "sourceType": "solidity-file",
                        "sourceFormat": "text",
                        "sourceList": [
                            "NodeRegistry.sol"
                        ]
                    }
                ],
                "extra": {
                    "discoveryTime": 5593089348,
                    "testCases": [
                        {
                            "initialState": {
                                "accounts": {
                                    "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa0": {
                                        "nonce": 0,
                                        "balance": "0x00000000000000000000000000000000000000ffffffffffffffffffffffffff",
                                        "code": "",
                                        "storage": {}
                                    },
                                    "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa1": {
                                        "nonce": 1,
                                        "balance": "0x0000000000000000000000000000000000000000000000000000000000000000",
                                        "code": "",
                                        "storage": {}
                                    },
                                    "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa2": {
                                        "nonce": 1,
                                        "balance": "0x00000000000000000000000000000000000000ffffffffffffffffffffffffff",
                                        "code": "",
                                        "storage": {}
                                    },
                                    "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa3": {
                                        "nonce": 1,
                                        "balance": "0x00000000000000000000000000000000000000ffffffffffffffffffffffffff",
                                        "code": "0x00",
                                        "storage": {}
                                    },
                                    "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa4": {
                                        "nonce": 1,
                                        "balance": "0x00000000000000000000000000000000000000ffffffffffffffffffffffffff",
                                        "code": "0xfd",
                                        "storage": {}
                                    },
                                    "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa5": {
                                        "nonce": 1,
                                        "balance": "0x00000000000000000000000000000000000000ffffffffffffffffffffffffff",
                                        "code": "0x608060405260005600a165627a7a72305820466f8a1bdae15c60b8e998fe04836ef505803cfbd8edd29bd4679531357576530029",
                                        "storage": {}
                                    },
                                    "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa6": {
                                        "nonce": 1,
                                        "balance": "0x00000000000000000000000000000000000000ffffffffffffffffffffffffff",
                                        "code": "0x608060405273aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa63081146038578073ffffffffffffffffffffffffffffffffffffffff16ff5b5000fea165627a7a723058205e8b906b72ad42c69b05acf4542283b6080ae82562bc74baac467daac2fb0e0e0029",
                                        "storage": {}
                                    },
                                    "0xaffeaffeaffeaffeaffeaffeaffeaffeaffeaffe": {
                                        "nonce": 0,
                                        "balance": "0x0000000000000000000000000000000000ffffffffffffffffffffffffffffff",
                                        "code": "",
                                        "storage": {}
                                    }
                                }
                            },
                            "steps": [
                                {
                                    "address": "",
                                    "gasLimit": "0xffffff",
                                    "gasPrice": "0x773594000",
                                    "input": REMOVED,
                                    "origin": "0xaffeaffeaffeaffeaffeaffeaffeaffeaffeaffe",
                                    "value": "0x0",
                                    "blockCoinbase": "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa0",
                                    "blockDifficulty": "0xa7d7343662e26",
                                    "blockGasLimit": "0xffffff",
                                    "blockNumber": "0x661a55",
                                    "blockTime": "0x5be99aa8"
                                },
                                {
                                    "address": "0x0901d12ebe1b195e5aa8748e62bd7734ae19b51f",
                                    "gasLimit": "0x7d00",
                                    "gasPrice": "0x773594000",
                                    "input": "0xac48987300000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
                                    "origin": "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa0",
                                    "value": "0x9",
                                    "blockCoinbase": "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa0",
                                    "blockDifficulty": "0x52c054bfb494c",
                                    "blockGasLimit": "0x7d0000",
                                    "blockNumber": "0x661a55",
                                    "blockTime": "0x5be99aa8"
                                }
                            ]
                        }
                    ],
                    "toolName": "harvey"
                }
            },
            {
                "swcID": "SWC-120",
                "swcTitle": "Weak Sources of Randomness from Chain Attributes",
                "description": {
                    "head": "Potential use of a weak source of randomness \"blockhash\".",
                    "tail": "Using past or the current block hashes through \"blockhash\" as a source of randomness is predictable. The issue can be ignored if this is unrelated to randomness or if the usage is related to a secure implementation of a commit/reveal scheme."
                },
                "severity": "Medium",
                "locations": [
                    {
                        "sourceMap": "6746:25:0",
                        "sourceType": "solidity-file",
                        "sourceFormat": "text",
                        "sourceList": [
                            "NodeRegistry.sol"
                        ]
                    }
                ],
                "extra": {
                    "discoveryTime": 1666071716,
                    "toolName": "maru"
                }
            },
            {
                "swcID": "SWC-120",
                "swcTitle": "Weak Sources of Randomness from Chain Attributes",
                "description": {
                    "head": "Potential use of a weak source of randomness \"blockhash\".",
                    "tail": "Using past or the current block hashes through \"blockhash\" as a source of randomness is predictable. The issue can be ignored if this is unrelated to randomness or if the usage is related to a secure implementation of a commit/reveal scheme."
                },
                "severity": "Medium",
                "locations": [
                    {
                        "sourceMap": "13158:23:0",
                        "sourceType": "solidity-file",
                        "sourceFormat": "text",
                        "sourceList": [
                            "NodeRegistry.sol"
                        ]
                    }
                ],
                "extra": {
                    "discoveryTime": 1666159516,
                    "toolName": "maru"
                }
            },
            {
                "swcID": "SWC-120",
                "swcTitle": "Weak Sources of Randomness from Chain Attributes",
                "description": {
                    "head": "Potential use of a weak source of randomness \"block.number\".",
                    "tail": "Using past or the current block hashes through \"block.number\" as a source of randomness is predictable. The issue can be ignored if this is unrelated to randomness or if the usage is related to a secure implementation of a commit/reveal scheme."
                },
                "severity": "Medium",
                "locations": [
                    {
                        "sourceMap": "6756:12:0",
                        "sourceType": "solidity-file",
                        "sourceFormat": "text",
                        "sourceList": [
                            "NodeRegistry.sol"
                        ]
                    }
                ],
                "extra": {
                    "discoveryTime": 1666984722,
                    "toolName": "maru"
                }
            },
            {
                "swcID": "SWC-120",
                "swcTitle": "Weak Sources of Randomness from Chain Attributes",
                "description": {
                    "head": "Potential use of a weak source of randomness \"block.number\".",
                    "tail": "Using past or the current block hashes through \"block.number\" as a source of randomness is predictable. The issue can be ignored if this is unrelated to randomness or if the usage is related to a secure implementation of a commit/reveal scheme."
                },
                "severity": "Medium",
                "locations": [
                    {
                        "sourceMap": "7532:12:0",
                        "sourceType": "solidity-file",
                        "sourceFormat": "text",
                        "sourceList": [
                            "NodeRegistry.sol"
                        ]
                    }
                ],
                "extra": {
                    "discoveryTime": 1667012822,
                    "toolName": "maru"
                }
            },
            {
                "swcID": "SWC-120",
                "swcTitle": "Weak Sources of Randomness from Chain Attributes",
                "description": {
                    "head": "Potential use of a weak source of randomness \"block.number\".",
                    "tail": "Using past or the current block hashes through \"block.number\" as a source of randomness is predictable. The issue can be ignored if this is unrelated to randomness or if the usage is related to a secure implementation of a commit/reveal scheme."
                },
                "severity": "Medium",
                "locations": [
                    {
                        "sourceMap": "13711:12:0",
                        "sourceType": "solidity-file",
                        "sourceFormat": "text",
                        "sourceList": [
                            "NodeRegistry.sol"
                        ]
                    }
                ],
                "extra": {
                    "discoveryTime": 1667019122,
                    "toolName": "maru"
                }
            }
        ],
        "sourceType": "raw-bytecode",
        "sourceFormat": "evm-byzantium-bytecode",
        "sourceList": [
            "0x63ad5e3d2c8551cf64f6d0425940efdeb79801907fcad157d1c82922919c13cb",
            "0xd8d70c0998b3293c364b1cde922c80081d70d02726e74091c8aae6fa6a10b892"
        ],
        "meta": {
            "selectedCompiler": "Unknown",
            "logs": [],
            "toolName": "maru",
            "coveredPaths": 91,
            "coveredInstructions": 7058
        }
    }
]

7.2 Ethlint

Ethlint is an open source project for linting Solidity code. Only security-related issues were reviewed by the audit team.

Below is the raw output of the Ethlint vulnerability scan:

contracts/BlockhashRegistry.sol
  21:0     warning    Avoid using experimental features in production code    no-experimental
  46:12    warning    Line exceeds the limit of 145 characters                max-len
  61:1     error      Line contains trailing whitespace                       no-trailing-whitespace
  79:4     warning    Line exceeds the limit of 145 characters                max-len
  81:1     error      Line contains trailing whitespace                       no-trailing-whitespace
  98:4     warning    Line exceeds the limit of 145 characters                max-len
  134:4    warning    Line exceeds the limit of 145 characters                max-len
  142:1    error      Line contains trailing whitespace                       no-trailing-whitespace

contracts/NodeRegistry.sol
  21:0     warning    Avoid using experimental features in production code    no-experimental
  117:1    error      Line contains trailing whitespace                       no-trailing-whitespace
  123:1    error      Line contains trailing whitespace                       no-trailing-whitespace
  128:8    warning    Line exceeds the limit of 145 characters                max-len
  143:1    error      Line contains trailing whitespace                       no-trailing-whitespace
  143:8    warning    Line exceeds the limit of 145 characters                max-len
  152:1    error      Line contains trailing whitespace                       no-trailing-whitespace
  152:4    warning    Line exceeds the limit of 145 characters                max-len
  197:1    error      Line contains trailing whitespace                       no-trailing-whitespace
  200:1    error      Line contains trailing whitespace                       no-trailing-whitespace
  215:1    error      Line contains trailing whitespace                       no-trailing-whitespace
  224:1    error      Line contains trailing whitespace                       no-trailing-whitespace
  324:1    error      Line contains trailing whitespace                       no-trailing-whitespace
  342:1    error      Line contains trailing whitespace                       no-trailing-whitespace
  448:1    error      Line contains trailing whitespace                       no-trailing-whitespace
  555:2    error      Line contains trailing whitespace                       no-trailing-whitespace
  555:8    warning    Line exceeds the limit of 145 characters                max-len
  568:1    error      Line contains trailing whitespace                       no-trailing-whitespace
  571:1    error      Line contains trailing whitespace                       no-trailing-whitespace
  602:1    error      Line contains trailing whitespace                       no-trailing-whitespace
  615:1    error      Line contains trailing whitespace                       no-trailing-whitespace

✖ 19 errors, 10 warnings found.

7.3 Surya

Surya is an utility tool for smart contract systems. It provides a number of visual outputs and information about structure of smart contracts. It also supports querying the function call graph in multiple ways to aid in the manual inspection and control flow analysis of contracts.

Below is a complete list of functions with their visibility and modifiers:

Contract Type Bases
Function Name Visibility Mutability Modifiers
NodeRegistry Implementation
<Constructor> Public ❗️ 🛑
convict External ❗️ 🛑 NO❗️
registerNode External ❗️ 💵 NO❗️
registerNodeFor External ❗️ 💵 NO❗️
removeNodeFromRegistry External ❗️ 🛑 onlyActiveState
returnDeposit External ❗️ 🛑 NO❗️
revealConvict External ❗️ 🛑 NO❗️
transferOwnership External ❗️ 🛑 onlyActiveState
unregisteringNode External ❗️ 🛑 onlyActiveState
updateNode External ❗️ 💵 onlyActiveState
totalNodes External ❗️ NO❗️
calcProofHash Internal 🔒
checkNodeProperties Internal 🔒
registerNodeInternal Internal 🔒 🛑
unregisterNodeInternal Internal 🔒 🛑
removeNode Internal 🔒 🛑
BlockhashRegistry Implementation
<Constructor> Public ❗️ 🛑
searchForAvailableBlock External ❗️ NO❗️
recreateBlockheaders Public ❗️ 🛑 NO❗️
saveBlockNumber Public ❗️ 🛑 NO❗️
snapshot Public ❗️ 🛑 NO❗️
getParentAndBlockhash Public ❗️ NO❗️
reCalculateBlockheaders Public ❗️ NO❗️
Legend
Symbol Meaning
🛑 Function can modify state
💵 Function is payable

7.4 Other Tools

Other security tools such as Slither was also used to identify problems in the smart contract.

7.5 Test Coverage

Code coverage metrics indicate the amount of lines/statements/branches that are covered by the test-suite. It’s important to note that “100% test coverage” does not indicate the code has no vulnerabilities. Be aware that code coverage does not provide information about the individual test-cases quality.

A fork of the Solidity-Coverage tool was used to measure the portion of the code base exercised by the test suite, and identify areas with little or no coverage. Specific sections of the code where necessary test coverage is missing are included in the Issue Details section.

The project is using the automated testing framework provided by Truffle. The test-suite is evaluating 62 individual tests and the test-suite passed without errors. The corresponding console output can be found here.

A code coverage report was generated and is provided along other tool output. The test coverage results for NodeRegistry.sol can be viewed here. The test coverage results for BlockhashRegistry.sol can be viewed here. Please find a summary of the coverage results below.

Code coverage report for All files

File Statements Branches Functions Lines
BlockhashRegistry.sol
100% 3030 92.86% 1314 100% 77 100% 3131
NodeRegistry.sol
100% 123123 95.45% 6366 100% 1717 100% 129129

Appendix 1 - Disclosure

ConsenSys Diligence (“CD”) typically receives compensation from one or more clients (the “Clients”) for performing the analysis contained in these reports (the “Reports”). The Reports may be distributed through other means, including via ConsenSys publications and other distributions.

The Reports are not an endorsement or indictment of any particular project or team, and the Reports do not guarantee the security of any particular project. This Report does not consider, and should not be interpreted as considering or having any bearing on, the potential economics of a token, token sale or any other product, service or other asset. Cryptographic tokens are emergent technologies and carry with them high levels of technical risk and uncertainty. No Report provides any warranty or representation to any Third-Party in any respect, including regarding the bugfree nature of code, the business model or proprietors of any such business model, and the legal compliance of any such business. No third party should rely on the Reports in any way, including for the purpose of making any decisions to buy or sell any token, product, service or other asset. Specifically, for the avoidance of doubt, this Report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project. CD owes no duty to any Third-Party by virtue of publishing these Reports.

PURPOSE OF REPORTS The Reports and the analysis described therein are created solely for Clients and published with their consent. The scope of our review is limited to a review of Solidity code and only the Solidity code we note as being within the scope of our review within this report. The Solidity language itself remains under development and is subject to unknown risks and flaws. The review does not extend to the compiler layer, or any other areas beyond Solidity that could present security risks. Cryptographic tokens are emergent technologies and carry with them high levels of technical risk and uncertainty.

CD makes the Reports available to parties other than the Clients (i.e., “third parties”) – on its website. CD hopes that by making these analyses publicly available, it can help the blockchain ecosystem develop technical best practices in this rapidly evolving area of innovation.

LINKS TO OTHER WEB SITES FROM THIS WEB SITE You may, through hypertext or other computer links, gain access to web sites operated by persons other than ConsenSys and CD. Such hyperlinks are provided for your reference and convenience only, and are the exclusive responsibility of such web sites’ owners. You agree that ConsenSys and CD are not responsible for the content or operation of such Web sites, and that ConsenSys and CD shall have no liability to you or any other person or entity for the use of third party Web sites. Except as described below, a hyperlink from this web Site to another web site does not imply or mean that ConsenSys and CD endorses the content on that Web site or the operator or operations of that site. You are solely responsible for determining the extent to which you may use any content at any other web sites to which you link from the Reports. ConsenSys and CD assumes no responsibility for the use of third party software on the Web Site and shall have no liability whatsoever to any person or entity for the accuracy or completeness of any outcome generated by such software.

TIMELINESS OF CONTENT The content contained in the Reports is current as of the date appearing on the Report and is subject to change without notice. Unless indicated otherwise, by ConsenSys and CD.