Skip to content

Commit

Permalink
Check that spent amounts and utxos are consistent before we sign a PSBT
Browse files Browse the repository at this point in the history 
PSBT utxo fields include the amount that are being spent by the PSBT inputs, but there is a "fee attack"
where using amounts that are lower than what is actually spent may make us sign a tx that spends much more
in fees than we think.
  • Loading branch information
@sstone
sstone committed Mar 20, 2023
1 parent a78616a commit 33c1efc
Show file tree
Hide file tree
Showing 4 changed files with 48 additions and 54 deletions.
4 changes: 2 additions & 2 deletions eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/rpc/BitcoinCoreClient.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ class BitcoinCoreClient(val rpcClient: BitcoinJsonRPCClient, val onchainKeyManag
fundTransaction(tx, FundTransactionOptions(feeRate, replaceable, inputWeights = externalInputsWeight.map { case (outpoint, weight) => InputWeight(outpoint, weight) }.toSeq))
}

private def processPsbt(psbt: Psbt, sign: Boolean = true, sighashType: Option[Int] = None)(implicit ec: ExecutionContext): Future[ProcessPsbtResponse] = {
def processPsbt(psbt: Psbt, sign: Boolean = true, sighashType: Option[Int] = None)(implicit ec: ExecutionContext): Future[ProcessPsbtResponse] = {
// we use an explicit map here because this RPC calls takes a String for the "sighashtype" parameter with an explicitly limited list of valid values
val sighashStrings = Map(
SigHash.SIGHASH_DEFAULT -> "DEFAULT",
Expand All @@ -254,7 +254,7 @@ class BitcoinCoreClient(val rpcClient: BitcoinJsonRPCClient, val onchainKeyManag
})
}

private def utxoUpdatePsbt(psbt: Psbt)(implicit ec: ExecutionContext): Future[Psbt] = {
def utxoUpdatePsbt(psbt: Psbt)(implicit ec: ExecutionContext): Future[Psbt] = {
val encoded = Base64.getEncoder.encodeToString(Psbt.write(psbt).toByteArray)

rpcClient.invoke("utxoupdatepsbt", encoded).map(json => {
Expand Down
21 changes: 18 additions & 3 deletions eclair-core/src/main/scala/fr/acinq/eclair/crypto/keymanager/LocalOnchainKeyManager.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,9 @@ package fr.acinq.eclair.crypto.keymanager
import fr.acinq.bitcoin.ScriptWitness
import fr.acinq.bitcoin.psbt.{Psbt, SignPsbtResult}
import fr.acinq.bitcoin.scalacompat.DeterministicWallet._
import fr.acinq.bitcoin.scalacompat.{Block, ByteVector32, DeterministicWallet, MnemonicCode}
import fr.acinq.bitcoin.scalacompat.{Block, ByteVector32, DeterministicWallet, MnemonicCode, Satoshi, Script}
import fr.acinq.bitcoin.utils.EitherKt
import grizzled.slf4j.Logging
import scodec.bits.ByteVector

import scala.jdk.CollectionConverters.MapHasAsScala
Expand All @@ -13,7 +14,7 @@ object LocalOnchainKeyManager {
def descriptorChecksum(span: String): String = fr.acinq.bitcoin.Descriptor.checksum(span)
}

class LocalOnchainKeyManager(entropy: ByteVector, chainHash: ByteVector32, passphrase: String = "") extends OnchainKeyManager {
class LocalOnchainKeyManager(entropy: ByteVector, chainHash: ByteVector32, passphrase: String = "") extends OnchainKeyManager with Logging {

import LocalOnchainKeyManager._

Expand Down Expand Up @@ -68,13 +69,18 @@ class LocalOnchainKeyManager(entropy: ByteVector, chainHash: ByteVector32, passp
}

override def signPsbt(psbt: Psbt, ourInputs: Seq[Int], ourOutputs: Seq[Int]): Psbt = {
import fr.acinq.bitcoin.scalacompat.KotlinUtils._

val spent = ourInputs.map(i => kmp2scala(psbt.getInput(i).getWitnessUtxo.amount)).sum
val backtous = ourOutputs.map(i => kmp2scala(psbt.getGlobal.getTx.txOut.get(i).amount)).sum

logger.info(s"signing ${psbt.getGlobal.getTx.txid} fees ${psbt.computeFees()} spent $spent to_us $backtous")
ourOutputs.foreach(i => isOurOutput(psbt, i))
ourInputs.foldLeft(psbt) { (p, i) => sigbnPsbtInput(p, i) }
}

// check that an output belongs to us i.e. we can recompute its public from its bip32 path
private def isOurOutput(psbt: Psbt, outputIndex: Int) = {

val output = psbt.getOutputs.get(outputIndex)
val txout = psbt.getGlobal.getTx.txOut.get(outputIndex)
output.getDerivationPaths.size() match {
Expand All @@ -83,6 +89,7 @@ class LocalOnchainKeyManager(entropy: ByteVector, chainHash: ByteVector32, passp
val priv = fr.acinq.bitcoin.DeterministicWallet.derivePrivateKey(master.priv, keypath.getKeyPath).getPrivateKey
val check = priv.publicKey()
require(pub == check, s"cannot compute public key for $txout")
require(txout.publicKeyScript.contentEquals(fr.acinq.bitcoin.Script.write(fr.acinq.bitcoin.Script.pay2wpkh(pub))), s"output pubkeyscript does not match ours for $txout")
}
case _ => throw new IllegalArgumentException(s"cannot verify that $txout sends to us")
}
Expand All @@ -93,6 +100,14 @@ class LocalOnchainKeyManager(entropy: ByteVector, chainHash: ByteVector32, passp

val input = psbt.getInput(pos)

// For each wallet input, Bitcoin Core will provide
// - the output that was spent, in the PSBT's witness utxo field
// - the actual transaction that was spent, in the PSBT's non-witness utxo field
// we check that this fields are consistent and match the outpoint that is spent in the PSBT
// this prevents attacks where bitcoin core would lie about the amount being spent and make us pay very high fees
require(input.getNonWitnessUtxo.txid == psbt.getGlobal.getTx.txIn.get(pos).outPoint.txid, "utxo txid mismatch")
require(input.getNonWitnessUtxo.txOut.get(psbt.getGlobal.getTx.txIn.get(pos).outPoint.index.toInt) == input.getWitnessUtxo, "utxo mismatch")

// check that we're signing a p2wpkh input and that the keypath is provided and correct
require(Script.isPay2wpkh(input.getWitnessUtxo.publicKeyScript.toByteArray), "spent input is not p2wpkh")
require(input.getDerivationPaths.size() == 1, "invalid bip32 path")
Expand Down
6 changes: 2 additions & 4 deletions eclair-core/src/test/scala/fr/acinq/eclair/blockchain/bitcoind/BitcoinCoreClientSpec.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1185,7 +1185,7 @@ class BitcoinCoreClientWithExternalSignerSpec extends BitcoinCoreClientSpec {
}
}

test("use eclair manage onchain keys") {
test("use eclair to manage onchain keys") {
val sender = TestProbe()

(1 to 10).foreach { _ =>
Expand Down Expand Up @@ -1213,9 +1213,7 @@ class BitcoinCoreClientWithExternalSignerSpec extends BitcoinCoreClientSpec {
val error = sender.expectMsgType[Failure]
assert(error.cause.getMessage.contains("Private keys are disabled for this wallet"))

wallet1.rpcClient.invoke("estimatesmartfee", 1).map(BitcoinCoreFeeProvider.parseFeeEstimate).pipeTo(sender.ref)
val feeratePerKb = sender.expectMsgType[FeeratePerKB]
wallet1.sendToPubkeyScript(Script.write(addressToPublicKeyScript(address, Block.RegtestGenesisBlock.hash)), 50_000.sat, FeeratePerKw(feeratePerKb)).pipeTo(sender.ref)
wallet1.sendToPubkeyScript(Script.write(addressToPublicKeyScript(address, Block.RegtestGenesisBlock.hash)), 50_000.sat, FeeratePerKw(FeeratePerByte(5.sat))).pipeTo(sender.ref)
sender.expectMsgType[ByteVector32]
}
}
Expand Down
71 changes: 26 additions & 45 deletions ...ir-core/src/test/scala/fr/acinq/eclair/crypto/keymanager/LocalOnchainKeyManagerSpec.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,76 +48,57 @@ class LocalOnchainKeyManagerSpec extends AnyFunSuite {
new KeyPathWithMaster(onchainKeyManager.getOnchainMasterMasterFingerprint, new fr.acinq.bitcoin.KeyPath("m/84'/1'/0'/0/2")),
)

val tx = Transaction(version = 2,
txIn = utxos.map(tx => TxIn(OutPoint(tx, 0), Nil, fr.acinq.bitcoin.TxIn.SEQUENCE_FINAL)),
txOut = TxOut(Satoshi(1000_000), Script.pay2wpkh(getPublicKey(0))) :: Nil, lockTime = 0)
val psbt: Psbt = {
val p0 = new Psbt(tx).updateWitnessInput(OutPoint(utxos(0), 0), utxos(0).txOut(0), null, Script.pay2pkh(getPublicKey(0)).map(scala2kmp).asJava, null, java.util.Map.of(getPublicKey(0), bip32paths(0)))
val p1 = EitherKt.flatMap(p0, (psbt: Psbt) => psbt.updateWitnessInput(OutPoint(utxos(1), 0), utxos(1).txOut(0), null, Script.pay2pkh(getPublicKey(1)).map(scala2kmp).asJava, null, java.util.Map.of(getPublicKey(1), bip32paths(1))))
val p2 = EitherKt.flatMap(p1, (psbt: Psbt) => psbt.updateWitnessInput(OutPoint(utxos(2), 0), utxos(2).txOut(0), null, Script.pay2pkh(getPublicKey(2)).map(scala2kmp).asJava, null, java.util.Map.of(getPublicKey(2), bip32paths(2))))
val p3 = EitherKt.flatMap(p2, (psbt: Psbt) => psbt.updateNonWitnessInput(utxos(0), 0, null, null, java.util.Map.of()))
val p4 = EitherKt.flatMap(p3, (psbt: Psbt) => psbt.updateNonWitnessInput(utxos(1), 0, null, null, java.util.Map.of()))
val p5 = EitherKt.flatMap(p4, (psbt: Psbt) => psbt.updateNonWitnessInput(utxos(2), 0, null, null, java.util.Map.of()))
val p6 = EitherKt.flatMap(p5, (psbt: Psbt) => psbt.updateWitnessOutput(0, null, null, java.util.Map.of(getPublicKey(0), bip32paths(0))))
p6.getRight
}

{
// sign all inputs and outputs
val tx = Transaction(version = 2,
txIn = utxos.map(tx => TxIn(OutPoint(tx, 0), Nil, fr.acinq.bitcoin.TxIn.SEQUENCE_FINAL)),
txOut = TxOut(Satoshi(1000_000), Script.pay2wpkh(getPublicKey(0))) :: Nil, lockTime = 0)
val psbt = new Psbt(tx)
val updated = {
val p = psbt.updateWitnessInput(OutPoint(utxos(0), 0), utxos(0).txOut(0), null, Script.pay2pkh(getPublicKey(0)).map(scala2kmp).asJava, null, java.util.Map.of(getPublicKey(0), bip32paths(0)))
val p1 = EitherKt.flatMap(p, (psbt: Psbt) => psbt.updateWitnessInput(OutPoint(utxos(1), 0), utxos(1).txOut(0), null, Script.pay2pkh(getPublicKey(1)).map(scala2kmp).asJava, null, java.util.Map.of(getPublicKey(1), bip32paths(1))))
val p2 = EitherKt.flatMap(p1, (psbt: Psbt) => psbt.updateWitnessInput(OutPoint(utxos(2), 0), utxos(2).txOut(0), null, Script.pay2pkh(getPublicKey(2)).map(scala2kmp).asJava, null, java.util.Map.of(getPublicKey(2), bip32paths(2))))
val p3 = EitherKt.flatMap(p2, (psbt: Psbt) => psbt.updateWitnessOutput(0, null, null, java.util.Map.of(getPublicKey(0), bip32paths(0))))
p3.getRight
}
val psbt1 = onchainKeyManager.signPsbt(updated, Seq(0, 1, 2), Seq(0))
val psbt1 = onchainKeyManager.signPsbt(psbt, Seq(0, 1, 2), Seq(0))
val signedTx = psbt1.extract().getRight
Transaction.correctlySpends(signedTx, utxos, ScriptFlags.STANDARD_SCRIPT_VERIFY_FLAGS)
}
{
// sign the first 2 inputs only
val tx = Transaction(version = 2,
txIn = utxos.map(tx => TxIn(OutPoint(tx, 0), Nil, fr.acinq.bitcoin.TxIn.SEQUENCE_FINAL)),
txOut = TxOut(Satoshi(1000_000), Script.pay2wpkh(getPublicKey(0))) :: Nil, lockTime = 0)
val psbt = new Psbt(tx)
val updated = {
val p = psbt.updateWitnessInput(OutPoint(utxos(0), 0), utxos(0).txOut(0), null, Script.pay2pkh(getPublicKey(0)).map(scala2kmp).asJava, null, java.util.Map.of(getPublicKey(0), bip32paths(0)))
val p1 = EitherKt.flatMap(p, (psbt: Psbt) => psbt.updateWitnessInput(OutPoint(utxos(1), 0), utxos(1).txOut(0), null, Script.pay2pkh(getPublicKey(1)).map(scala2kmp).asJava, null, java.util.Map.of(getPublicKey(1), bip32paths(1))))
val p2 = EitherKt.flatMap(p1, (psbt: Psbt) => psbt.updateWitnessOutput(0, null, null, java.util.Map.of(getPublicKey(0), bip32paths(0))))
p2.getRight
}
val psbt1 = onchainKeyManager.signPsbt(updated, Seq(0, 1), Seq(0))
val psbt1 = onchainKeyManager.signPsbt(psbt, Seq(0, 1), Seq(0))
// extracting the final tx fails because no all inputs as signed
assert(psbt1.extract().isLeft)
assert(psbt1.getInput(2).getScriptWitness == null)
}
{
// provide a wrong derivation path for the first input
val tx = Transaction(version = 2,
txIn = utxos.map(tx => TxIn(OutPoint(tx, 0), Nil, fr.acinq.bitcoin.TxIn.SEQUENCE_FINAL)),
txOut = TxOut(Satoshi(1000_000), Script.pay2wpkh(getPublicKey(0))) :: Nil, lockTime = 0)
val psbt = new Psbt(tx)
val updated = {
val p = psbt.updateWitnessInput(OutPoint(utxos(0), 0), utxos(0).txOut(0), null, Script.pay2pkh(getPublicKey(0)).map(scala2kmp).asJava, null, java.util.Map.of(getPublicKey(0), bip32paths(2))) // wrong bip32 path
val p1 = EitherKt.flatMap(p, (psbt: Psbt) => psbt.updateWitnessInput(OutPoint(utxos(1), 0), utxos(1).txOut(0), null, Script.pay2pkh(getPublicKey(1)).map(scala2kmp).asJava, null, java.util.Map.of(getPublicKey(1), bip32paths(1))))
val p2 = EitherKt.flatMap(p1, (psbt: Psbt) => psbt.updateWitnessInput(OutPoint(utxos(2), 0), utxos(2).txOut(0), null, Script.pay2pkh(getPublicKey(2)).map(scala2kmp).asJava, null, java.util.Map.of(getPublicKey(2), bip32paths(2))))
val p3 = EitherKt.flatMap(p2, (psbt: Psbt) => psbt.updateWitnessOutput(0, null, null, java.util.Map.of(getPublicKey(0), bip32paths(0))))
p3.getRight
}
val updated = psbt.updateWitnessInput(OutPoint(utxos(0), 0), utxos(0).txOut(0), null, Script.pay2pkh(getPublicKey(0)).map(scala2kmp).asJava, null, java.util.Map.of(getPublicKey(0), bip32paths(2))).getRight // wrong bip32 path
val error = intercept[IllegalArgumentException] {
onchainKeyManager.signPsbt(updated, Seq(0, 1, 2), Seq(0))
}
assert(error.getMessage.contains("cannot compute private key"))
}
{
// provide a wrong derivation path for the first output
val tx = Transaction(version = 2,
txIn = utxos.map(tx => TxIn(OutPoint(tx, 0), Nil, fr.acinq.bitcoin.TxIn.SEQUENCE_FINAL)),
txOut = TxOut(Satoshi(1000_000), Script.pay2wpkh(getPublicKey(0))) :: Nil, lockTime = 0)
val psbt = new Psbt(tx)
val updated = {
val p = psbt.updateWitnessInput(OutPoint(utxos(0), 0), utxos(0).txOut(0), null, Script.pay2pkh(getPublicKey(0)).map(scala2kmp).asJava, null, java.util.Map.of(getPublicKey(0), bip32paths(0)))
val p1 = EitherKt.flatMap(p, (psbt: Psbt) => psbt.updateWitnessInput(OutPoint(utxos(1), 0), utxos(1).txOut(0), null, Script.pay2pkh(getPublicKey(1)).map(scala2kmp).asJava, null, java.util.Map.of(getPublicKey(1), bip32paths(1))))
val p2 = EitherKt.flatMap(p1, (psbt: Psbt) => psbt.updateWitnessInput(OutPoint(utxos(2), 0), utxos(2).txOut(0), null, Script.pay2pkh(getPublicKey(2)).map(scala2kmp).asJava, null, java.util.Map.of(getPublicKey(2), bip32paths(2))))
val p3 = EitherKt.flatMap(p2, (psbt: Psbt) => psbt.updateWitnessOutput(0, null, null, java.util.Map.of(getPublicKey(0), bip32paths(1)))) // wrong path
p3.getRight
}
val updated = psbt.updateWitnessOutput(0, null, null, java.util.Map.of(getPublicKey(0), bip32paths(1))).getRight // wrong path
val error = intercept[IllegalArgumentException] {
onchainKeyManager.signPsbt(updated, Seq(0, 1, 2), Seq(0))
}
assert(error.getMessage.contains("cannot compute public key"))
}
{
// lie about the amount being spent
val updated = psbt.updateWitnessInput(OutPoint(utxos(0), 0), utxos(0).txOut(0).copy(amount = Satoshi(10)), null, Script.pay2pkh(getPublicKey(0)).map(scala2kmp).asJava, null, java.util.Map.of(getPublicKey(0), bip32paths(0))).getRight
val error = intercept[IllegalArgumentException] {
onchainKeyManager.signPsbt(updated, Seq(0, 1, 2), Seq(0))
}
assert(error.getMessage.contains("utxo mismatch"))
}
}

test("compute descriptor checksums") {
Expand Down

0 comments on commit 33c1efc

Please sign in to comment.