Check that fundrawtransaction does not add more than 1 change output

ACINQ · Apr 26, 2023 · 7bb8236 · 7bb8236
1 parent af09ef7
commit 7bb8236
Show file tree

Hide file tree

Showing 2 changed files with 65 additions and 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
@@ -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
@@ -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"))