What was the second vulnerability used in The DAO attack on 17 June 2016?

Question

From The big theDAO heist FAQ by koeppelmann:

How did the attack worked exactly? The attacker managed to combine 2 exploits. The first exploit was to call the split DAO function recursively. That means the first regular call would trigger a second (irregular) call of the function and the second call would trigger another call and so on. The following calls are done in a state before the balance of the attacker is set back to 0. This allowed the attacker to split 20 times (have to look up the exact number) per transaction. He could not do more - otherwise the transactions would have gotten too big and eventually would have reached the block limite. This attack would already have been painful. However - what made it really painful is that the attacked managed to replicate this attack from the same two addresses with the same tokens over and over again (roughly 250 times from 2 addresses each). So the attacker found a second exploit that allowed to split without destroying the tokens in the main DAO. They managed to transfer the tokens away before they get sent to address 0x0 and only after this they are sent back) The combination of both attacks multiplied the effect. Attack one on its one would have been very capital intensive (you need to bring up 1/20 of the stolen amount upfront) - the attack two would have taken a long time.

The first vulnerability is discussed in What is a recursive calling vulnerability?.

Q: What is the second vulnerability that allowed the attacker to "replicate this attack from the same two addresses with the same tokens over and over again (roughly 250 times from 2 addresses each)"?

From The DAO code, the splitDAO(...) function code follows:

function splitDAO(
    uint _proposalID,
    address _newCurator
) noEther onlyTokenholders returns (bool _success) {

    Proposal p = proposals[_proposalID];

    // Sanity check

    if (now < p.votingDeadline  // has the voting deadline arrived?
        //The request for a split expires XX days after the voting deadline
        || now > p.votingDeadline + splitExecutionPeriod
        // Does the new Curator address match?
        || p.recipient != _newCurator
        // Is it a new curator proposal?
        || !p.newCurator
        // Have you voted for this split?
        || !p.votedYes[msg.sender]
        // Did you already vote on another proposal?
        || (blocked[msg.sender] != _proposalID && blocked[msg.sender] != 0) )  {

        throw;
    }

    // If the new DAO doesn't exist yet, create the new DAO and store the
    // current split data
    if (address(p.splitData[0].newDAO) == 0) {
        p.splitData[0].newDAO = createNewDAO(_newCurator);
        // Call depth limit reached, etc.
        if (address(p.splitData[0].newDAO) == 0)
            throw;
        // should never happen
        if (this.balance < sumOfProposalDeposits)
            throw;
        p.splitData[0].splitBalance = actualBalance();
        p.splitData[0].rewardToken = rewardToken[address(this)];
        p.splitData[0].totalSupply = totalSupply;
        p.proposalPassed = true;
    }

    // Move ether and assign new Tokens
    uint fundsToBeMoved =
        (balances[msg.sender] * p.splitData[0].splitBalance) /
        p.splitData[0].totalSupply;
    if (p.splitData[0].newDAO.createTokenProxy.value(fundsToBeMoved)(msg.sender) == false)
        throw;

    // Assign reward rights to new DAO
    uint rewardTokenToBeMoved =
        (balances[msg.sender] * p.splitData[0].rewardToken) /
        p.splitData[0].totalSupply;

    uint paidOutToBeMoved = DAOpaidOut[address(this)] * rewardTokenToBeMoved /
        rewardToken[address(this)];

    rewardToken[address(p.splitData[0].newDAO)] += rewardTokenToBeMoved;
    if (rewardToken[address(this)] < rewardTokenToBeMoved)
        throw;
    rewardToken[address(this)] -= rewardTokenToBeMoved;

    DAOpaidOut[address(p.splitData[0].newDAO)] += paidOutToBeMoved;
    if (DAOpaidOut[address(this)] < paidOutToBeMoved)
        throw;
    DAOpaidOut[address(this)] -= paidOutToBeMoved;

    // Burn DAO Tokens
    Transfer(msg.sender, 0, balances[msg.sender]);
    withdrawRewardFor(msg.sender); // be nice, and get his rewards
    totalSupply -= balances[msg.sender];
    balances[msg.sender] = 0;
    paidOut[msg.sender] = 0;
    return true;
}

The statement balances[msg.sender] = 0; at the bottom of splitDAO(...) should have prevented the same address from calling the splitDAO(...) function to successfully transfer funds multiple times.

And from the Q&A Is there any way to determine how long it took for the DAO attacker to deploy the attack?, each of the transactions (the first and second at least from my manual counting) called splitDAO(...) 29 times. But the 29 x splitDAO(...) calls were repeatedly called, creating 27996 internal transactions, 13996 were non-zero internal transfers. Calculation: 13996 transactions x 258.05656476 ETH = 3,611,759.68038 ethers, which is approximately the 3,641,694.241898506 Ether ($59,578,117.80) was moved to the account 0x304a554a310c7e546dfe434669c62820b7d83490.

Answer 1

It's not so much a vulnerability, but the attack cleverly transferred its DAO tokens between 2 accounts, by using function transfer(address _to, uint256 _amount).

So the fallback function of the attacking contract looks like:

function() {
  transfer DAO tokens to other attacking contract
  invoke splitDAO
}

There were 2 attacking contracts that transferred DAO tokens to each other. When one attacking contract's transaction finished, balances[msg.sender] = 0 would be correctly set, but the tokens had been transferred to the other contract. Now the other contract performs the attack until it's transaction finishes. The attacking contracts alternate.

Source

@Roland's answer mentions how TheDAO could have prevented this.

Answer 2

// Burn DAO Tokens
    Transfer(msg.sender, 0, balances[msg.sender]);

The intention of Christoph Jentzsch seems have to been to burn the token. Instead he called an Event. Just because the Event was called Transfer instead of LogTransfer??
Explained in the excellent Peter Vessenes piece:

instead of the logging function, we should have:

if (!transfer(0 , balances[msg.sender])) { throw; }

This would keep the recursive call attacks from functioning, but also reduce the tokens available to the user later on.