From 7bb823645f3250b6ba344e4167fc7b12e099bacf Mon Sep 17 00:00:00 2001 From: sstone Date: Tue, 11 Apr 2023 09:37:49 +0200 Subject: [PATCH] Check that fundrawtransaction does not add more than 1 change output --- .../bitcoind/rpc/BitcoinCoreClient.scala | 19 +++++-- .../bitcoind/BitcoinCoreClientSpec.scala | 51 ++++++++++++++++++- 2 files changed, 65 insertions(+), 5 deletions(-) diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/rpc/BitcoinCoreClient.scala b/eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/rpc/BitcoinCoreClient.scala index e3eafbc626..ab4cb503f6 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/rpc/BitcoinCoreClient.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/rpc/BitcoinCoreClient.scala @@ -215,15 +215,28 @@ class BitcoinCoreClient(val rpcClient: BitcoinJsonRPCClient, val onchainKeyManag } //------------------------- FUNDING -------------------------// - def fundTransaction(tx: Transaction, options: FundTransactionOptions)(implicit ec: ExecutionContext): Future[FundTransactionResponse] = { - rpcClient.invoke("fundrawtransaction", tx.toString(), options).map(json => { + rpcClient.invoke("fundrawtransaction", tx.toString(), options).flatMap(json => { val JString(hex) = json \ "hex" val JInt(changePos) = json \ "changepos" val JDecimal(fee) = json \ "fee" val fundedTx = Transaction.read(hex) val changePos_opt = if (changePos >= 0) Some(changePos.intValue) else None - FundTransactionResponse(fundedTx, toSatoshi(fee), changePos_opt) + + val walletInputs = fundedTx.txIn.map(_.outPoint).toSet -- tx.txIn.map(_.outPoint).toSet + val addedOutputs = fundedTx.txOut.size - tx.txOut.size + Try { + require(addedOutputs <= 1, "more than one change output added") + require(addedOutputs == 0 || changePos >= 0, "change output added, but position not returned") + require(changePos < 0 || !tx.txOut.contains(fundedTx.txOut(changePos.intValue)), "existing output returned as change output") + require(options.changePosition.isEmpty || changePos == options.changePosition.get || changePos == -1, "change position added at wrong position") + + FundTransactionResponse(fundedTx, toSatoshi(fee), changePos_opt) + } + match { + case Success(response) => Future.successful(response) + case Failure(error) => unlockOutpoints(walletInputs.toSeq).flatMap(_ => Future.failed(error)) + } }) } diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/blockchain/bitcoind/BitcoinCoreClientSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/blockchain/bitcoind/BitcoinCoreClientSpec.scala index 313d3aa225..f014a36dac 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/blockchain/bitcoind/BitcoinCoreClientSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/blockchain/bitcoind/BitcoinCoreClientSpec.scala @@ -30,7 +30,7 @@ import fr.acinq.eclair.blockchain.WatcherSpec.{createSpendManyP2WPKH, createSpen import fr.acinq.eclair.blockchain.bitcoind.BitcoindService.{BitcoinReq, SignTransactionResponse} import fr.acinq.eclair.blockchain.bitcoind.rpc.BitcoinCoreClient._ import fr.acinq.eclair.blockchain.bitcoind.rpc.BitcoinJsonRPCAuthMethod.UserPassword -import fr.acinq.eclair.blockchain.bitcoind.rpc.{BasicBitcoinJsonRPCClient, BitcoinCoreClient, JsonRPCError} +import fr.acinq.eclair.blockchain.bitcoind.rpc.{BasicBitcoinJsonRPCClient, BitcoinCoreClient, BitcoinJsonRPCClient, JsonRPCError} import fr.acinq.eclair.blockchain.fee.{FeeratePerByte, FeeratePerKw} import fr.acinq.eclair.crypto.keymanager.LocalOnchainKeyManager import fr.acinq.eclair.transactions.{Scripts, Transactions} @@ -151,6 +151,52 @@ class BitcoinCoreClientSpec extends TestKitBaseClass with BitcoindService with A bitcoinClient.rollback(fundTxResponse.tx).pipeTo(sender.ref) sender.expectMsg(true) } + { + // check that bitcoin core is not lying to us + val txNotFunded = Transaction(2, Nil, TxOut(150000 sat, Script.pay2wpkh(randomKey().publicKey)) :: Nil, 0) + + def makeEvilBitcoinClient(changePosMod: (Int) => Int, txMod: Transaction => Transaction): BitcoinCoreClient = { + val badRpcClient = new BitcoinJsonRPCClient { + override def invoke(method: String, params: Any*)(implicit ec: ExecutionContext): Future[JValue] = method match { + case "fundrawtransaction" => bitcoinClient.rpcClient.invoke(method, params: _*)(ec).map(json => json.mapField { + case ("changepos", JInt(pos)) => ("changepos", JInt(changePosMod(pos.toInt))) + case ("hex", JString(hex)) => ("hex", JString(txMod(Transaction.read(hex)).toString())) + case x => x + })(ec) + case _ => bitcoinClient.rpcClient.invoke(method, params: _*)(ec) + } + } + new BitcoinCoreClient(badRpcClient, if (useEclairSigner) Some(onchainKeyManager) else None) + } + + // verify that bitcoin core is not lying to us + { + val bitcoinClient1 = makeEvilBitcoinClient((pos: Int) => -1, (tx: Transaction) => tx) + bitcoinClient1.fundTransaction(txNotFunded, FundTransactionOptions(TestConstants.feeratePerKw)).pipeTo(sender.ref) + sender.expectMsgType[Failure] + } + { + val bitcoinClient1 = makeEvilBitcoinClient((pos: Int) => pos, (tx: Transaction) => tx.copy(txOut = tx.txOut.reverse)) + bitcoinClient1.fundTransaction(txNotFunded, FundTransactionOptions(TestConstants.feeratePerKw)).pipeTo(sender.ref) + sender.expectMsgType[Failure] + } + { + val bitcoinClient1 = makeEvilBitcoinClient((pos: Int) => pos, (tx: Transaction) => tx.copy(txOut = tx.txOut.head +: tx.txOut)) + bitcoinClient1.fundTransaction(txNotFunded, FundTransactionOptions(TestConstants.feeratePerKw)).pipeTo(sender.ref) + sender.expectMsgType[Failure] + } + { + val bitcoinClient1 = makeEvilBitcoinClient((pos: Int) => 1, (tx: Transaction) => tx.copy(txOut = tx.txOut.reverse)) + bitcoinClient1.fundTransaction(txNotFunded, FundTransactionOptions(TestConstants.feeratePerKw, changePosition = Some(0))).pipeTo(sender.ref) + sender.expectMsgType[Failure] + } + { + val bitcoinClient1 = makeEvilBitcoinClient((pos: Int) => pos, (tx: Transaction) => tx) + bitcoinClient1.fundTransaction(txNotFunded, FundTransactionOptions(TestConstants.feeratePerKw, changePosition = Some(0))).pipeTo(sender.ref) + bitcoinClient.rollback(sender.expectMsgType[FundTransactionResponse].tx).pipeTo(sender.ref) + sender.expectMsg(true) + } + } } test("fund transactions with external inputs") { @@ -307,8 +353,9 @@ class BitcoinCoreClientSpec extends TestKitBaseClass with BitcoindService with A } test("absence of rounding") { - val txIn = Transaction(1, Nil, Nil, 42) val hexOut = "02000000013361e994f6bd5cbe9dc9e8cb3acdc12bc1510a3596469d9fc03cfddd71b223720000000000feffffff02c821354a00000000160014b6aa25d6f2a692517f2cf1ad55f243a5ba672cac404b4c0000000000220020822eb4234126c5fc84910e51a161a9b7af94eb67a2344f7031db247e0ecc2f9200000000" + val fundedTx = Transaction.read(hexOut) + val txIn = fundedTx.copy(txIn = Nil, txOut = fundedTx.txOut(0) :: Nil) (0 to 9).foreach { satoshi => val apiAmount = JDecimal(BigDecimal(s"0.0000000$satoshi"))