Check that fundrawtransaction does not add more than 1 change output

ACINQ · Apr 25, 2023 · 205e8a3 · 205e8a3
1 parent af09ef7
commit 205e8a3
Show file tree

Hide file tree

Showing 2 changed files with 55 additions and 2 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
@@ -223,6 +223,13 @@ class BitcoinCoreClient(val rpcClient: BitcoinJsonRPCClient, val onchainKeyManag
  val JDecimal(fee) = json \ "fee"
  val fundedTx = Transaction.read(hex)
  val changePos_opt = if (changePos >= 0) Some(changePos.intValue) else None
+
+ val addedOutputs = fundedTx.txOut.size - tx.txOut.size
+ 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)
  })
  }

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
@@ -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,51 @@ 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)
+ sender.expectMsgType[FundTransactionResponse]
+ }
+ }
  }
 
  test("fund transactions with external inputs") {
@@ -307,8 +352,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"))