Skip to content

Commit

Permalink
multi: re-audit contracts after confirmation, before acting
Browse files Browse the repository at this point in the history 
There is a very slight possibility that contract outputs that pass
audit while in mempool or in some cases before they are broadcasted
(rawtx audit) may differ from the output later observed in a block
for the same coin.

This risk is higher for spv wallets that will mostly only perform
audits on rawtxs before broadcasting the txs, without a guarantee
that the tx is accepted to the mempool. A malicious actor could
broadcast a different tx with same hash (theoretically possible)
but with a different output at the expected vout index.

There is risk of funds if clients only later check that the hash for
the earlier-audited tx is found in a block and proceed to send their
counter swap or expose their contract secret via a redemption. This
commit aims to mitigate that risk by repeating contract audits after
the initial tx hash is observed on the blockchain, ensuring that the
tx now observed on the blockchain is as desired.
  • Loading branch information
@itswisdomagain
itswisdomagain committed Jul 31, 2021
1 parent d4fcfd3 commit 9b51633
Show file tree
Hide file tree
Showing 6 changed files with 319 additions and 88 deletions.
214 changes: 160 additions & 54 deletions client/asset/dcr/dcr.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1625,13 +1625,24 @@ func (dcr *ExchangeWallet) SignMessage(coin asset.Coin, msg dex.Bytes) (pubkeys,
return pubkeys, sigs, nil
}

// AuditContract retrieves information about a swap contract from the provided
// txData if the provided txData
// - represents a valid transaction that pays to the provided contract at the
// specified coinID and
// - can be broadcasted or is already broadcasted to the blockchain network.
// This information would be used to verify the counter-party's contract during
// a swap.
// AuditContract retrieves information about a swap contract from the blockchain
// (if possible) or from the provided txData if the provided txData represents a
// valid transaction that pays to the provided contract at the specified coinID
// and (for full node-backed wallets) can be broadcasted to the blockchain network.
//
// The information returned would be used to verify the counter-party's contract
// during a swap.
//
// NOTE: For SPV wallets, a successful audit response is no gaurantee that the
// txData provided to this method was actually broadcasted to the blockchain.
// An error may have occured while trying to broadcast the txData or even if
// there was no broadcast error, the tx might still not enter mempool or get
// mined e.g. if the tx references invalid or already spent inputs.
//
// Granted, clients wait for the contract tx to be included in a block before
// taking further actions on a match; but it is generally safer to repeat this
// audit after the contract tx is mined to ensure that the tx observed on the
// blockchain is as expected.
func (dcr *ExchangeWallet) AuditContract(coinID, contract, txData dex.Bytes) (*asset.AuditInfo, error) {
txHash, vout, err := decodeCoinID(coinID)
if err != nil {
Expand All @@ -1644,7 +1655,33 @@ func (dcr *ExchangeWallet) AuditContract(coinID, contract, txData dex.Bytes) (*a
return nil, fmt.Errorf("error extracting swap addresses: %w", err)
}

// Validate the provided txData against the provided coinID (hash and vout).
// If this coin can be located on the blockchain, audit this contract
// using the output script gotten from the blockchain. It is especially
// important for contracts that have been mined to audit the tx that was
// mined rather than the txData provided to this method.
contractCoin, contractOutputScript, scriptVer, err := dcr.findContractInBlockchain(txHash, vout)
if err != nil {
return nil, err
}
if contractCoin != nil {
// Found the contract on the blockchain. Audit using the output script
// gotten from the blockchain.
if err = dcr.validateContractOutputScript(contractOutputScript, scriptVer, contract); err != nil {
return nil, err
}
dcr.log.Debugf("Audited contract coin %s:%d using tx data gotten from the blockchain. SPV mode = %t",
txHash, vout, dcr.spvMode)
return &asset.AuditInfo{
Coin: contractCoin,
Contract: contract,
SecretHash: secretHash,
Recipient: receiver.String(),
Expiration: time.Unix(int64(stamp), 0).UTC(),
}, nil
}

// Assume tx has not been broadcasted. Audit the provided txData
// and broadcast it.
contractTx := wire.NewMsgTx()
if err := contractTx.Deserialize(bytes.NewReader(txData)); err != nil {
return nil, fmt.Errorf("invalid contract tx data: %w", err)
Expand All @@ -1655,9 +1692,6 @@ func (dcr *ExchangeWallet) AuditContract(coinID, contract, txData dex.Bytes) (*a
if int(vout) >= len(contractTx.TxOut) {
return nil, fmt.Errorf("invalid contract tx data: no output at %d", vout)
}

// Verify that the output of interest pays to the hash of the provided contract.
// Output script must be P2SH, with 1 address and 1 required signature.
contractTxOut := contractTx.TxOut[vout]
if err = dcr.validateContractOutputScript(contractTxOut.PkScript, contractTxOut.Version, contract); err != nil {
return nil, err
Expand All @@ -1678,40 +1712,38 @@ func (dcr *ExchangeWallet) AuditContract(coinID, contract, txData dex.Bytes) (*a
dcr.log.Errorf("Error rebroadcasting contract tx %v: %v", txData, translateRPCCancelErr(err))

if !dcr.spvMode {
// The broadcast may have failed because the tx was already broadcasted (and maybe
// even mined). Full node wallets can verify this by calling gettxout. If the tx
// is not found by gettxout, it's possible that the tx isn't yet broadcasted. An
// asset.CoinNotFoundError is returned which signals callers to repeat this audit
// (including the tx rebroadcast attempt) until the tx is found or the match is
// revoked by the server.
dcr.log.Debugf("Attempting to find and audit contract using dcrd.")
txOut, _, err := dcr.getTxOut(txHash, vout, true)
if err != nil {
return nil, fmt.Errorf("error finding unspent contract: %w", translateRPCCancelErr(err))
}
if txOut == nil {
return nil, asset.CoinNotFoundError
}
pkScript, err := hex.DecodeString(txOut.ScriptPubKey.Hex)
if err != nil {
return nil, fmt.Errorf("error decoding pubkey script from hex '%s': %w",
txOut.ScriptPubKey.Hex, err)
}
if err = dcr.validateContractOutputScript(pkScript, txOut.ScriptPubKey.Version, contract); err != nil {
return nil, err
}
// The broadcast may have failed because the tx was already
// broadcasted (and maybe mined or even spent), or some other
// unexpected error occurred. Return asset.CoinNotFoundError
// to signal the caller to repeat this audit (including the
// tx rebroadcast attempt) until the tx is found, the raw tx
// is broadcasted successfully or the match is revoked by the
// server.

// TODO: It'd be unnecessary to continue trying to find this
// contract or to broadcast the rawtx if the tx was already
// broadcasted, mined and spent.
// Consider modifying dcr.findContractInBlockchain to check if
// the tx exists on the blockchain using the gettransaction rpc
// if gettxout returns a nil response. If the gettransaction rpc
// confirms the existence of the tx output, return a CoinSpent
// error to the caller.

return nil, asset.CoinNotFoundError
}

// SPV wallets do not produce sendrawtransaction errors for already broadcasted
// txs. This must be some other unexpected/undesired error.
// Do NOT return an asset.CoinNotFoundError so callers do not recall this method
// as there's no gaurantee that re-attempting the broadcast will succeed.
// Return a successful audit response because it is possible that the tx was
// already broadcasted and the caller can safely begin waiting for confirmations.
// Granted, this wait would be futile if the tx was never broadcasted but as
// explained above, retrying the broadcast isn't a better course of action, neither
// is returning an error here because that would cause the caller to potentially
// give up on this match prematurely.
// SPV wallets do not produce sendrawtransaction errors for already
// broadcasted txs. This must be some other unexpected error.
// Do NOT return an asset.CoinNotFoundError so callers do not recall
// this method as there's no gaurantee that the broadcast will succeed
// on subsequent attempts.
// Return a successful audit response because it is possible that the
// tx was already broadcasted and the caller can safely begin waiting
// for confirmations.
// Granted, this wait would be futile if the tx was never broadcasted
// but as explained above, retrying the broadcast isn't a better course
// of action, neither is returning an error here because that would cause
// the caller to potentially give up on this match prematurely.
}

if err == nil && !finalTxHash.IsEqual(txHash) {
Expand All @@ -1725,6 +1757,7 @@ func (dcr *ExchangeWallet) AuditContract(coinID, contract, txData dex.Bytes) (*a
dcr.trackExternalTx(txHash, contractTxOut.PkScript)
}

dcr.log.Debugf("Audited contract coin %s:%d using raw tx data. SPV mode = %t", txHash, vout, dcr.spvMode)
return &asset.AuditInfo{
Coin: newOutput(txHash, vout, uint64(contractTxOut.Value), determineTxTree(contractTx)),
Contract: contract,
Expand All @@ -1734,7 +1767,68 @@ func (dcr *ExchangeWallet) AuditContract(coinID, contract, txData dex.Bytes) (*a
}, nil
}

// findContractInBlockchain attempts to locate the specified contract output in
// the blockchain. If found and unspent, the output is returned along with its
// pkScript. Returns an error if the output is spent. It is not an error if the
// tx cannot be located, a nil response and nil error is returned.
func (dcr *ExchangeWallet) findContractInBlockchain(txHash *chainhash.Hash, vout uint32) (*output, []byte, uint16, error) {
if dcr.spvMode {
// SPV wallets can only locate contract outputs on the blockchain
// using cfilters.
txOut, txTree, isSpent, err := dcr.externalTxOut(txHash, vout)
if err != nil {
if errors.Is(err, asset.CoinNotFoundError) {
return nil, nil, 0, nil
}
return nil, nil, 0, fmt.Errorf("error checking if contract is mined or spent: %v", err)
}
if isSpent {
return nil, nil, 0, fmt.Errorf("contract output %s:%d is spent", txHash, vout)
}
// Extract relevant info from the tx data retrieved from the blockchain.
contractOutput := newOutput(txHash, vout, uint64(txOut.Value), txTree)
return contractOutput, txOut.PkScript, txOut.Version, nil
}

// Full-node wallets can locate 'unspent' contract outputs using gettxout.
txOut, txTree, err := dcr.getTxOut(txHash, vout, true)
if err != nil {
return nil, nil, 0, fmt.Errorf("error finding unspent contract: %w", translateRPCCancelErr(err))
}
if txOut == nil {
// Output does not exist or has been spent. Use getrawtransaction
// to confirm if this tx exists and has an output at `vout`. If it
// does, then it must have been spent for gettxout to return a nil
// response.
tx, err := dcr.node.GetRawTransactionVerbose(dcr.ctx, txHash)
if err != nil {
if isTxNotFoundErr(err) {
return nil, nil, 0, nil
}
return nil, nil, 0, fmt.Errorf("error looking up contract tx %s: %w", txHash, translateRPCCancelErr(err))
}
if len(tx.Vout) <= int(vout) {
return nil, nil, 0, fmt.Errorf("tx %s has no output at index %d", txHash, vout)
}
// Tx found and contains the requested output.
return nil, nil, 0, fmt.Errorf("contract output %s:%d is spent", txHash, vout)
}

txOutAmt, err := dcrutil.NewAmount(txOut.Value)
if err != nil {
return nil, nil, 0, fmt.Errorf("error parsing output amount %f: %w", txOut.Value, err)
}
contractOutput := newOutput(txHash, vout, uint64(txOutAmt), txTree)
contractOutputScript, err := hex.DecodeString(txOut.ScriptPubKey.Hex)
if err != nil {
return nil, nil, 0, fmt.Errorf("error decoding pubkey script from hex '%s': %w",
txOut.ScriptPubKey.Hex, err)
}
return contractOutput, contractOutputScript, txOut.ScriptPubKey.Version, nil
}

func (dcr *ExchangeWallet) validateContractOutputScript(pkScript []byte, scriptVer uint16, contract []byte) error {
// Output script must be P2SH, with 1 address and 1 required signature.
scriptClass, addrs, sigsReq, err := txscript.ExtractPkScriptAddrs(scriptVer, pkScript, dcr.chainParams, false)
if err != nil {
return fmt.Errorf("error extracting script addresses from '%x': %w", pkScript, err)
Expand Down Expand Up @@ -2392,14 +2486,22 @@ func (dcr *ExchangeWallet) Confirmations(ctx context.Context, id dex.Bytes) (con
return uint32(txOut.Confirmations), false, nil
}

// Unspent output not found. Check if the transaction exists.
// If it exists, then the output must either be spent or we
// are dealing with an invalid vout. Regardless, assume spent.
// Unspent output not found. Using wallet tx lookup for spv
// wallets and getrawtransaction for non-spv wallets, check
// if the transaction exists and has an output at `vout`.

if dcr.spvMode {
// SPV wallets can only look up wallet transactions. Check
// if this tx is indexed by the wallet.
tx, err := dcr.node.GetTransaction(ctx, txHash)
if err == nil {
// Tx found, check if it contains the requested output.
msgTx, err := msgTxFromHex(tx.Hex)
if err != nil {
return 0, false, fmt.Errorf("invalid hex for tx %s: %v", txHash, err)
}
if len(msgTx.TxOut) <= int(vout) {
return 0, false, fmt.Errorf("tx %s has no output at index %d", txHash, vout)
}
// Tx found and contains the requested output.
return uint32(tx.Confirmations), true, nil
}
if !isTxNotFoundErr(err) {
Expand All @@ -2415,15 +2517,19 @@ func (dcr *ExchangeWallet) Confirmations(ctx context.Context, id dex.Bytes) (con
}

// Node-backed wallets can look up any transaction. Check if this
// tx exists on the blockchain.
// tx output can be found.
tx, err := dcr.node.GetRawTransactionVerbose(ctx, txHash)
if err == nil {
return uint32(tx.Confirmations), true, nil
}
if !isTxNotFoundErr(err) {
if err != nil {
if isTxNotFoundErr(err) {
return 0, false, asset.CoinNotFoundError
}
return 0, false, translateRPCCancelErr(err)
}
return 0, false, asset.CoinNotFoundError
if len(tx.Vout) <= int(vout) {
return 0, false, fmt.Errorf("tx %s has no output at index %d", txHash, vout)
}
// Tx found and contains the requested output.
return uint32(tx.Confirmations), true, nil
}

// addInputCoins adds inputs to the MsgTx to spend the specified outputs.
Expand Down
4 changes: 3 additions & 1 deletion client/asset/dcr/dcr_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2083,7 +2083,9 @@ func TestConfirmations(t *testing.T) {
if wallet.spvMode {
node.walletTx = &walletjson.GetTransactionResult{}
} else {
node.rawTx = &chainjson.TxRawResult{}
node.rawTx = &chainjson.TxRawResult{
Vout: []chainjson.Vout{{}}, // rawTx must have an output at index 0 to be considered spent
}
}
_, spent, err = wallet.Confirmations(context.Background(), coinID)
if err != nil {
Expand Down
34 changes: 34 additions & 0 deletions client/asset/dcr/externaltx.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,40 @@ func (dcr *ExchangeWallet) externalTx(hash *chainhash.Hash) (*externalTx, bool)
return tx, tracked
}

func (dcr *ExchangeWallet) externalTxOut(hash *chainhash.Hash, vout uint32) (*wire.TxOut, int8, bool, error) {
tx, tracked := dcr.externalTx(hash)
if !tracked {
return nil, 0, false, asset.CoinNotFoundError
}

// Lock the scanMtx to prevent attempted rescans from
// mutating the tx block, outputs map or tree field.
tx.scanMtx.RLock()
txBlockHash, err := tx.relevantBlockHash(dcr.getBlockHash)
if err != nil {
tx.scanMtx.RUnlock()
return nil, 0, false, err
}
if txBlockHash == nil {
tx.scanMtx.RUnlock()
return nil, 0, false, asset.CoinNotFoundError
}
output := tx.outputs[vout]
txTree := tx.tree
tx.scanMtx.RUnlock()

if output == nil {
return nil, 0, false, fmt.Errorf("tx %s has no output at index %d", hash, vout)
}

isSpent, err := dcr.findExternalTxOutputSpender(output, txBlockHash)
if err != nil {
return nil, 0, false, err
}

return output.TxOut, txTree, isSpent, nil
}

// externalTxOutConfirmations uses the script(s) associated with an externalTx
// to find the block in which the tx is mined and checks if the specified tx
// output is spent by a mined transaction. This tx must have been previously
Expand Down
23 changes: 18 additions & 5 deletions client/asset/interface.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,11 +122,24 @@ type Wallet interface {
// specified Coin. A slice of pubkeys required to spend the Coin and a
// signature for each pubkey are returned.
SignMessage(Coin, dex.Bytes) (pubkeys, sigs []dex.Bytes, err error)
// AuditContract retrieves information about a swap contract on the
// blockchain. This would be used to verify the counter-party's contract
// during a swap. If the coin cannot be found for the coin ID, the
// ExchangeWallet should return CoinNotFoundError. This enables the client
// to properly handle network latency.
// AuditContract retrieves information about a swap contract from the
// blockchain (where possible) or from the provided txData (if valid).
// The information returned would be used to verify the counter-party's
// contract during a swap. If the coin cannot be found on the blockchain
// and the provided txData cannot be broadcasted, a CoinNotFoundError
// may be returned. This enables the client to properly handle network
// latency where appropriate.
//
// NOTE: For SPV wallets, a successful audit response is no gaurantee that
// the txData provided was actually broadcasted to the blockchain. An error
// may have occured while trying to broadcast the txData or even if there
// was no broadcast error, the tx might still not enter mempool or get mined
// e.g. if the tx references invalid or already spent inputs.
//
// Granted, clients wait for the contract tx to be included in a block before
// taking further actions on a match; but it is generally safer to repeat this
// audit after the contract tx is mined to ensure that the tx observed on the
// blockchain is as expected.
AuditContract(coinID, contract, txData dex.Bytes) (*AuditInfo, error)
// LocktimeExpired returns true if the specified contract's locktime has
// expired, making it possible to issue a Refund. The contract expiry time
Expand Down
Loading

0 comments on commit 9b51633

Please sign in to comment.