And another bug...
The topic this time: Transaction timestamps
Let's have Bob do some malicious things again
It's Bob's turn to forge a block. He decides to hold back that block a little, not too much, so that there aren't 2 blocks generated by other people in the meantime and his block still has the highest cumulated difficulty.
Let's say this gives him an extra minute of time. (In reality, at the moment this would be even higher)
He now goes on to include incoming transactions in the block, even those with a creation timestamp
after his block's timestamp.
After he's done, he sends that block to the peers.
The peers should see the better cumulated difficulty and then hopefully check, whether it only contains transactions up to it's creation time.
So let's take a look in the code, if they actually do that...
We are somewhere in pushBlock and have just extracted the transactions contained in the block. Now for every transaction, we check the following:
if (transaction.timestamp > curTime + 15
|| transaction.deadline < 1
|| (transaction.timestamp + transaction.deadline * 60 < blockTimestamp && getLastBlock().height > 303)
|| transaction.feeNxt <= 0
|| !transaction.validateAttachment()
|| Nxt.transactions.get(block.transactions[i]) != null
|| (transaction.referencedTransaction != 0
&& Nxt.transactions.get(transaction.referencedTransaction) == null && blockTransactions
.get(transaction.referencedTransaction) == null)
|| (Nxt.unconfirmedTransactions.get(block.transactions[i]) == null && !transaction.verify())) {
break;
}
The transaction timestamp is checked in 2 occasions:
First:
(transaction.timestamp > curTime + 15) causes the block to be invalid. So what's curTime? curTime = getEpochTime(System.currentTimeMillis()), so current system time, basically.
That one is fine, the transaction was issued in the very recent past.
Second:
(transaction.timestamp + transaction.deadline * 60 < blockTimestamp)
Our block also doesn't contains transactions whose deadline has already passed, so we're fine, too in that regard.
... There are no other checks for transaction timestamps ...
So Bob's block goes thru with some extra transactions in it for some extra cash... *yay*
How to avoid that:
change (transaction.timestamp > curTime + 15) to (transaction.timestamp > blockTimestamp)
And also make sure that the generation logic for blocks doesn't have the same flaw.
[edit]
The generation logic has the same flaw, the timestamp is not checked at all. And I also don't see a check whether the unconfirmed transaction is still within deadline in generateBlock() or am I blind?
