rpc: verify address is for correct net

[torkelrogstad]

Change Description

Verify that the addresses we're decoding when sending coins onchain are
for the correct network. Without this check we'll convert the users
addresses to their equivalent on other networks, which is a gross
violation of the principle of least astonishment.

Steps to Test

Prior to this commit:

1. Run a node on regtest
2. Issue either a sendmany or sendcoins RPC, with a mainnet or testnet address
3. Observe that the address is accepted, and converted into a regtest address.

After this commit, step 2 fails.

Pull Request Checklist

Testing

• Your PR passes all CI checks.
• Tests covering the positive and negative (error paths) are included.
• Bug fixes contain tests triggering the bug to prevent regressions.
  There doesn't seem to be any tests for this, at least not based on a quick code search. Would be happy to implement if I'm wrong.

Code Style and Documentation

• The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
• Commits follow the Ideal Git Commit Structure.
• Any new logging statements use an appropriate subsystem and logging level.
• There is a change description in the release notes, or [skip ci] in the commit message for small changes.

📝 Please see our Contribution Guidelines for further guidance.

[guggero]

Thanks for the fix! LGTM 🎉

[guggero]

Actually, while we're at it, we should probably also add the check here:

• https://github.com/lightningnetwork/lnd/blob/master/lnrpc/walletrpc/walletkit_server.go#L1030
• https://github.com/lightningnetwork/lnd/blob/master/lnrpc/invoicesrpc/addinvoice.go#L277
• https://github.com/lightningnetwork/lnd/blob/master/rpcserver.go#L2474

Or, to fix the underlying assumption that (because it takes the net params as the second parameter) DecodeAddress actually checks that the address belongs to the correct network, we should perhaps add the check there?
Looking at the code, this seems to only be possible with non-Segwit (non-bech32) addresses in the first place.

[torkelrogstad]

Or, to fix the underlying assumption that (because it takes the net params as the second parameter) DecodeAddress actually checks that the address belongs to the correct network, we should perhaps add the check there?

Do you mean changing btcutil? That sounds like a pretty big behavioral change in how DecodeAddress works

[guggero]

Do you mean changing btcutil? That sounds like a pretty big behavioral change in how DecodeAddress works

Yes, maybe it's too big of a change. But maybe it would be worth adding a comment in the btcutil library that for legacy addresses the network isn't checked.

[torkelrogstad]

Found another place, in chancloser

lnd/lnwallet/chancloser/chancloser.go

Lines 710 to 728 in f13399b

	// ParseUpfrontShutdownAddress attempts to parse an upfront shutdown address.
	// If the address is empty, it returns nil. If it successfully decoded the
	// address, it returns a script that pays out to the address.
	func ParseUpfrontShutdownAddress(address string,
	params *chaincfg.Params) (lnwire.DeliveryAddress, error) {
	if len(address) == 0 {
	return nil, nil
	}
	addr, err := btcutil.DecodeAddress(
	address, params,
	)
	if err != nil {
	return nil, fmt.Errorf("invalid address: %v", err)
	}
	return txscript.PayToAddrScript(addr)
	}

For the sake of returning errors that look the same in all places, and ergonomics, would it be an idea to introduce a new wallet in btcutil? Filed an issue in btcd, figured that was a better place to discuss: btcsuite/btcd#1846

[guggero]

Looking at the code, this seems to only be possible with non-Segwit (non-bech32) addresses in the first place.

Oops, I was wrong there. I read the if chaincfg.IsBech32SegwitPrefix(prefix) { incorrectly. It doesn't check that the prefix is for the given network, it just checks if it's any valid prefix...

I like your idea of adding DecodeAddressForNet in btcd. Since we'll be bumping the dependency to that project anyway, I'd rather use that method everywhere in lnd (after the PR there was merged) than add the IsForNet check everywhere.

[arshbot]

tACK on the bug to be fixed, agree with all that the fix should be included in DecodeAddressForNet, that way it can trickle into other projects.

Friend at mitbitcoin actually ran into this same issue, so there is real need for this fix.

[lightninglabs-deploy]

@torkelrogstad, remember to re-request review from reviewers when ready

[guggero]

Replaced by/included in #7689, original commit author credits are preserved.