<p>The <a href="https://github.com/BitcoinUnlimited/BitcoinUnlimited/commit/eee6a2daeb560f26061535695fc0f7de168ffe32" rel="nofollow noreferrer">commit implementing the fix</a> was issued Mar 14, 2017, 3:16 PM GMT. A Github <a href="https://github.com/BitcoinUnlimited/BitcoinUnlimited/releases/tag/1.0.1.1" rel="nofollow noreferrer">release</a> was made at 7:39 PM GMT. It was <a href="https://www.reddit.com/r/btc/comments/5zei9k/bu_1011_hotfix_released/" rel="nofollow noreferrer">announced on Reddit</a> at 7:59 PM GMT. The vulnerability was discovered sometime before the git commit, but it's unclear how long before.</p>

<p>According to nodecounter, things were fine at about 6 PM GMT, when there were 776 nodes. That rapidly dropped, with 696 nodes at 7 PM GMT. It bottomed out at 11 PM, with 182 nodes. At 9 AM GMT the following day, things were mostly back to normal, with 626 nodes. I don't think it's very accurate to say that BU reacted abnormally quickly, or that 100% of nodes came back.</p>

<blockquote>
  <p>Where was the hack in the code exactly?</p>
</blockquote>

<p>There were two unrelated issues. Both issues were in how Bitcoin Unlimited responds to network packets asking for thinblocks. </p>

<ol>
<li><p>Reachable assert.</p>

<p>BU had this code:</p>

<pre><code>void SendXThinBlock(CBlock &amp;block, CNode* pfrom, const CInv &amp;inv)
{
    if (inv.type == MSG_XTHINBLOCK)
    {
        // code
    }
    else if (inv.type == MSG_THINBLOCK)
    {
        // code code code
    }
    else
    {
        assert(0);
    }
    // more code
}
</code></pre>

<p>(I've removed irrelevant parts to make it issue easier to see.)</p>

<p>This means, </p>

<ul>
<li>If the request is a thinblock, reply in this way,</li>
<li>if the request is an xthinblock, reply in that way,</li>
<li>if it's neither, crash.</li>
</ul>

<p>Why does it do that? I assume that the person who wrote this code assumed that this function would only be called for one of those two types of requests. This function is called in two places. In the first place where this function is called, that assumption is correct. In the second place, it is wrong. This bug was introduced May 2016 <a href="https://github.com/BitcoinUnlimited/BitcoinUnlimited/commit/4770e765a63252cb7a5e35d637b3b7cba4035264" rel="nofollow noreferrer">in this commit</a>.</p>

<p>The fix is to DoS ban the node that sent the request, rather than crashing.</p></li>
<li><p>Unchecked return value</p>

<p>BU had this code:</p>

<pre><code>BlockMap::iterator mi = mapBlockIndex.find(inv.hash);
// code code
if (!ReadBlockFromDisk(block, (*mi).second, consensusParams))
    assert(!"cannot load block from disk");
</code></pre>

<p>The problem here is that if <code>inv.hash</code> does not exist in <code>mapBlockIndex</code>, the first line will return <code>mapBlockIndex.end()</code>. The next line will then pass a null pointer to <code>ReadBlockFromDisk</code>. Then, the node will crash. This bug was introduced Jan 2016 <a href="https://github.com/BitcoinUnlimited/BitcoinUnlimited/commit/2c9e2c62f5bf4369f4bff7ba783ae32432de4f68" rel="nofollow noreferrer">in this commit</a>.</p></li>
</ol>

CC BY-SA 3.0
2017-03-16T15:06:16.360Z
How many developers reviewed this part of the code? It's always looks simple after the fact to spot this. But as a non-code reviewer, was this a hard bug to spot?
Patoshi パトシ
CC BY-SA 3.0
2017-03-16T17:46:57.640Z
@duckx I don't think either bug was obvious without the benefit of hindsight. `SendXThinBlock` was correct at the time it was written, and only became vulnerable after a second path to it was created.
Nick ODell
CC BY-SA 3.0
2017-03-16T17:56:34.087Z
@duckx The second was a find() that was not followed by a check for whether it was in the map. But Bitcoin does this in other places, and it's perfectly OK: https://github.com/bitcoin/bitcoin/blob/44f2df613f23d0a095a6e47b3d9c498b9df3055b/src/policy/rbf.cpp#L38 Is there an obvious way to distinguish these two cases? I'm not sure.
Nick ODell
CC BY-SA 3.0
2017-03-15T22:59:53.290Z
2017-03-16T16:22:55.603Z
2017-03-16T16:22:55.603Z
Nick ODell
<p>I've been trying to follow the Bitcoin Unlimited hack timeline (2017-Mar) and there seems to be conflicting stories on both sides. It's obviously to confuse the 3rd party into finding the truth. My main questions are:</p>

<ul>
<li><strong>When</strong> was the exploit discovered and by whom?</li>
<li>When was the fix issued after the discovery of the exploit?</li>
<li>When did the hack occur to knock out over 700+ Bitcoin Unlimited nodes?</li>
<li>There were reports of these 700+ nodes going offline and then within a few hours, it all came back online. If this is the case, how does one conclude that these 700+ nodes are controlled by a few people?</li>
<li>Who are the main characters on both sides in this hack to affect news stories and perception?</li>
<li>Where was the hack in the code exactly?</li>
</ul>

<hacking><bitcoin-unlimited><hack>
What was the timeline of the Bitcoin Unlimited Hack as of 2017-March and where was it in the code?
Patoshi パトシ
# What was the timeline of the Bitcoin Unlimited Hack as of 2017-March and where was it in the code?

(Asked by: username_0 on 2017-03-15)

I've been trying to follow the Bitcoin Unlimited hack timeline (2017-Mar) and there seems to be conflicting stories on both sides. It's obviously to confuse the 3rd party into finding the truth. My main questions are:

* **When** was the exploit discovered and by whom?
* When was the fix issued after the discovery of the exploit?
* When did the hack occur to knock out over 700+ Bitcoin Unlimited nodes?
* There were reports of these 700+ nodes going offline and then within a few hours, it all came back online. If this is the case, how does one conclude that these 700+ nodes are controlled by a few people?
* Who are the main characters on both sides in this hack to affect news stories and perception?
* Where was the hack in the code exactly?

**Tags:** hacking, bitcoin-unlimited, hack


---

## 1 answers

### Answer by username_1 on 2017-03-15. Score: 4

The [commit implementing the fix](https://github.com/BitcoinUnlimited/BitcoinUnlimited/commit/eee6a2daeb560f26061535695fc0f7de168ffe32) was issued Mar 14, 2017, 3:16 PM GMT. A Github [release](https://github.com/BitcoinUnlimited/BitcoinUnlimited/releases/tag/1.0.1.1) was made at 7:39 PM GMT. It was [announced on Reddit](https://www.reddit.com/r/btc/comments/5zei9k/bu_1011_hotfix_released/) at 7:59 PM GMT. The vulnerability was discovered sometime before the git commit, but it's unclear how long before.

According to nodecounter, things were fine at about 6 PM GMT, when there were 776 nodes. That rapidly dropped, with 696 nodes at 7 PM GMT. It bottomed out at 11 PM, with 182 nodes. At 9 AM GMT the following day, things were mostly back to normal, with 626 nodes. I don't think it's very accurate to say that BU reacted abnormally quickly, or that 100% of nodes came back.

> Where was the hack in the code exactly?

There were two unrelated issues. Both issues were in how Bitcoin Unlimited responds to network packets asking for thinblocks.

1. Reachable assert.

   BU had this code:

   ```
   void SendXThinBlock(CBlock &block, CNode* pfrom, const CInv &inv)
   {
       if (inv.type == MSG_XTHINBLOCK)
       {
           // code
       }
       else if (inv.type == MSG_THINBLOCK)
       {
           // code code code
       }
       else
       {
           assert(0);
       }
       // more code
   }

   ```

   (I've removed irrelevant parts to make it issue easier to see.)

   This means,

   * If the request is a thinblock, reply in this way,
   * if the request is an xthinblock, reply in that way,
   * if it's neither, crash.

   Why does it do that? I assume that the person who wrote this code assumed that this function would only be called for one of those two types of requests. This function is called in two places. In the first place where this function is called, that assumption is correct. In the second place, it is wrong. This bug was introduced May 2016 [in this commit](https://github.com/BitcoinUnlimited/BitcoinUnlimited/commit/4770e765a63252cb7a5e35d637b3b7cba4035264).

   The fix is to DoS ban the node that sent the request, rather than crashing.
2. Unchecked return value

   BU had this code:

   ```
   BlockMap::iterator mi = mapBlockIndex.find(inv.hash);
   // code code
   if (!ReadBlockFromDisk(block, (*mi).second, consensusParams))
       assert(!"cannot load block from disk");

   ```

   The problem here is that if `inv.hash` does not exist in `mapBlockIndex`, the first line will return `mapBlockIndex.end()`. The next line will then pass a null pointer to `ReadBlockFromDisk`. Then, the node will crash. This bug was introduced Jan 2016 [in this commit](https://github.com/BitcoinUnlimited/BitcoinUnlimited/commit/2c9e2c62f5bf4369f4bff7ba783ae32432de4f68).

> **Comments:**
> **username_0**: How many developers reviewed this part of the code? It's always looks simple after the fact to spot this. But as a non-code reviewer, was this a hard bug to spot?  
> **username_1**: @duckx I don't think either bug was obvious without the benefit of hindsight. `SendXThinBlock` was correct at the time it was written, and only became vulnerable after a second path to it was created.  
> **username_1**: @duckx The second was a find() that was not followed by a check for whether it was in the map. But Bitcoin does this in other places, and it's perfectly OK: https://github.com/bitcoin/bitcoin/blob/44f2df613f23d0a095a6e47b3d9c498b9df3055b/src/policy/rbf.cpp#L38 Is there an obvious way to distinguish these two cases? I'm not sure.  

---

