[rekey] `goal multisig sign` fails if sender rekeyed to multisig address

ryanRfox commented 4 years ago

Unable to sign a transaction sending from address following a rekey to multisig address.

Given:

Accounts A, B, C and BC_T1 (a multisig of A,B with threshold 1)
Account A successfully rekey-to B
Account A desires to rekey-to BC_T1 (requiring a signature from B or C)
- goal clerk multisig sign --address B --tx rekey.utxn
- Couldn't sign tx with kmd: multisig information (pks, threshold) for address does not exist in this wallet

Expected Behavior: Either B or C should be able to use goal multisig sign to add their signature to the transaction.

Environment

Software version: algod -v 2.0.77719.dev [master] (commit #171c4afd+)
Network: private network for testing rekey-to functionality

Steps to reproduce

#!/bin/bash

# origianl code supplied by @algobolson
# https://github.com/justicz/go-algorand/blob/maxj/applications/test/scripts/e2e_subs/rekey.sh

set -e
set -x
set -o pipefail
export SHELLOPTS

# ensure you pass the wallet name as arg1 when executing the script
WALLET=$1

# set the path for your temporary directory used to store account keys
TEMPDIR="/TEMPDIR"

# Set the appropriate algod values for your environment. These are the defaults for Sandbox on TestNet
ALGOD_HOST_PORT="localhost:4001"
ALGOD_HEADER="x-algo-api-token:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"

# ensure your PATH is set properly, else modify the following and set /path/to/goal and /path/to/algokey
GOAL_CMD="goal -d $DATA -w ${WALLET}"
ALGOKEY_CMD="algokey"

# load *Default account from your wallet as ACCOUNT_MASTER. It must be funded and will dispense funds to newly accounts for testing. MASTER_ACCOUNT will NOT be rekeyed.
ACCOUNT_MASTER=$(${GOAL_CMD} account list | awk '$6 == "*Default" { print $3 }')

# create a new account as ACCOUNT_A using goal
ACCOUNT_A=$(${GOAL_CMD} account new | awk '{ print $6 }')

echo "Fund ACCOUNT_A from ACCOUNT_MASTER"
${GOAL_CMD} clerk send --from ${ACCOUNT_MASTER} --to ${ACCOUNT_A} --amount 1000000

# generate a new account as ACCOUNT_B using algokey
${ALGOKEY_CMD} generate > ${TEMPDIR}/account_b
MNEMONIC_ACCOUNT_B=$(grep 'Private key mnemonic:' < ${TEMPDIR}/account_b | sed 's/Private key mnemonic: //')
ACCOUNT_B=$(grep 'Public key:' < ${TEMPDIR}/account_b | sed 's/Public key: //')

# import ACCOUNT_B into your wallet using the mnemonic passphrase
${GOAL_CMD} account import -m "${MNEMONIC_ACCOUNT_B}"

# create a new account as ACCOUNT_C using goal
ACCOUNT_C=$(${GOAL_CMD} account new | awk '{ print $6 }')

echo "Initial account object for ACCOUNT_A:\n"
curl "$ALGOD_HOST_PORT/v2/accounts/$ACCOUNT_A" -H $ALGOD_HEADER

# TEST_1: send from ACCOUNT_A to ACCOUNT_B
echo "TEST_1:\n"
${GOAL_CMD} clerk send --from ${ACCOUNT_A} --to ${ACCOUNT_B} --amount 100000
# EXPECTED RESULT: Pass, the transaction will confirm.

# REKEY_TO_1: ACCOUNT_A rekey-to ACCOUNT_B
echo "Rekeying ACCOUNT_A to ACCOUNT_B\n"
${GOAL_CMD} clerk send --from ${ACCOUNT_A} --to ${ACCOUNT_B} --amount 0 --rekey-to ${ACCOUNT_B}
echo "Updated account object for ACCOUNT_A:\n"
curl "$ALGOD_HOST_PORT/v2/accounts/$ACCOUNT_A" -H $ALGOD_HEADER

# TEST_2: send from ACCOUNT_A to ACCOUNT_B signed by ACCOUNT_A (default behavior)
echo "TEST_2:\n"
# create, sign and send transaction
${GOAL_CMD} clerk send --from ${ACCOUNT_A} --to ${ACCOUNT_B} --amount 100000
# EXPECTED RESULT: Fail, the transaction will not confirm because should have been authorized by ACCOUNT_B

# TEST_3: send from ACCOUNT_A to ACCOUNT_B signed by ACCOUNT_B
echo "TEST_3:\n"
# create unsigned transaction
${GOAL_CMD} clerk send --from ${ACCOUNT_A} --to ${ACCOUNT_B} --amount 100000 --out ${TEMPDIR}/test3.utxn
# sign transaction
${GOAL_CMD} clerk sign --signer ${ACCOUNT_B} --infile ${TEMPDIR}/test3.utxn --outfile ${TEMPDIR}/test3.stxn
# send signed transaction
${GOAL_CMD} clerk rawsend -f ${TEMPDIR}/test3.stxn
# EXPECTED RESULT: Pass, the transaction will confirm because ACCOUNT_B is the authorized address.

# Create a multisig account MSIG_BC_T_1 with threshold 1
MSIG_BC_T_1=$(${GOAL_CMD} account multisig new $ACCOUNT_B $ACCOUNT_C --threshold 1 | awk '{ print $6 }')

# rekey ACCOUNT_A to MSIG_BC_T_1
echo "Rekeying ACCOUNT_A to MSIG_BC_T_1\n"
${GOAL_CMD} clerk send --from ${ACCOUNT_A} --to ${ACCOUNT_B} --amount 0 --rekey-to ${MSIG_BC_T_1} --out ${TEMPDIR}/rekey-to-msig_bc.utxn
${GOAL_CMD} clerk sign --signer ${ACCOUNT_B} --infile ${TEMPDIR}/rekey-to-msig_bc.utxn --outfile ${TEMPDIR}/rekey-to-msig_bc.stxn
${GOAL_CMD} clerk rawsend -f ${TEMPDIR}/rekey-to-msig_bc.stxn
echo "Updated account object for ACCOUNT_A:\n"
curl "$ALGOD_HOST_PORT/v2/accounts/$ACCOUNT_A" -H $ALGOD_HEADER

# TEST_4: send from ACCOUNT_A to ACCOUNT_B signed by ACCOUNT_B
echo "TEST_4:\n"
${GOAL_CMD} clerk send --from ${ACCOUNT_A} --to ${ACCOUNT_B} --amount 100000 --out ${TEMPDIR}/test4.utxn
${GOAL_CMD} clerk multisig sign --address ${ACCOUNT_B} --tx ${TEMPDIR}/test4.utxn
${GOAL_CMD} clerk rawsend -f ${TEMPDIR}/test4.utxn
# EXPECTED RESULT: Pass, the transaction will confirm because ACCOUNT_B is sufficient threshold of MSIG_BC_T_1.

ryanRfox commented 4 years ago

In the test script above, TESTS 1-3 pass as expected, as do both rekey-to assignments. However, TEST 4 fails on signing with "Couldn't sign tx with kmd: multisig information (pks, threshold) for address does not exist in this wallet"

All accounts are in a single wallet:

goal account list
[offline]       MASTER  LOWE5DE25WOXZB643JSNWPE6MGIJNBLRPU2RBAVUNI4ZU22E3N7PHYYHSY      18999362 microAlgos     *Default
[offline]       A       WIOPRMRT4GSNFFT43B55DEQCVG67EQ2HEKPV4EULQ55YXBWMBTYUCPDUY4      796000 microAlgos
[offline]       B       TZIWJ3BKGLGTFDA4YAZPHIXGIEOVA5XLHWOCINUQY2PPHZ6XVFGKGMWVGA      200000 microAlgos
[offline]       C       5XWY6RBNYHCSY2HK5HCTO62DUJJ4PT3G4L77FQEBUKE6ZYRGQAFTLZSQQ4      0 microAlgos
[offline]       BC_T1   TIGOHKLBABQTPPCJQTDNLMT2FFHJGBDK37GRNIL4JVHAQZJJRMMJKFTQD4      0 microAlgos    [1/2 multisig]

Here is the unsigned transaction goal clerk inspect ${TEMPDIR}/test4.utxn

{
  "txn": {
    "amt": 100000,
    "fee": 1000,
    "fv": 857,
    "gen": "REKEY-v1",
    "gh": "aT+oawSj/oxSWPNILM7O7EkuVE1rPxD5lLTgY+kvVCA=",
    "lv": 1857,
    "note": "9S8IGfbAmSY=",
    "rcv": "TZIWJ3BKGLGTFDA4YAZPHIXGIEOVA5XLHWOCINUQY2PPHZ6XVFGKGMWVGA",
    "snd": "WIOPRMRT4GSNFFT43B55DEQCVG67EQ2HEKPV4EULQ55YXBWMBTYUCPDUY4",
    "type": "pay"
  }
}

ryanRfox commented 4 years ago

I believe goal clerk multisig sign is checking the snd field , expecting it to be an msig. In this case, the sender is a standard single key address, but has rekeyed and set the auth-addr field to a multisig address. Therefore, I believe goal should first check the auth-addr field (if it exists), else check snd.

brianolson commented 4 years ago

Confirming your hunch https://github.com/algorand/go-algorand/blob/master/cmd/goal/multisig.go#L111 Yeah, that needs a way to know that "Sender" isn't always the addr it needs to look up.

ian-algorand commented 4 years ago

@ryanRfox, Rotem is going to take a look and try to identify the core issue here.

algorotem commented 4 years ago

The problem stems from the multisig design. Our multisig's sub-keys are not Algogrand accounts/addresses but simply public keys, therefore you can't use rekeyed accounts. Unfortunately, there is no good solution for it at this point besides creating a new msig address.

JasonWeathersby commented 4 years ago

Does this mean no rekeying of multisigs?

algorotem commented 4 years ago

No, you can rekey msig to another msig (which, in practice, allows you to swap one of the subkeys) but you cannot rekey one of the subkeys of a msig account.

JasonWeathersby commented 4 years ago

so its all or none?

algorotem commented 4 years ago

Well, you can still add the same old accounts to the new one msig but yes, you'll need to create a completely new msig address first.

JasonWeathersby commented 4 years ago

ok

brianolson commented 4 years ago

@algorotem I think this case is simpler than that, it's just that goal multisig sign has no facility for passing in a signing address, the feature in single-key goal clerk sign is here https://github.com/algorand/go-algorand/blob/master/cmd/goal/clerk.go#L694 vs https://github.com/algorand/go-algorand/blob/master/cmd/goal/multisig.go#L111 which only references txn.Sender()

ryanRfox commented 4 years ago

I modified TEST4 to use algokey per a conversation with @algorotem.

# TEST_4: send from ACCOUNT_A to ACCOUNT_B signed by ACCOUNT_B
echo "TEST_4:\n"
${GOAL_CMD} clerk send --from ${ACCOUNT_A} --to ${ACCOUNT_B} --amount 100000 --out ${TEMPDIR}/test4.utxn

# ALTERNATIVE: use algokey to sign
algokey sign --txfile ${TEMPDIR}/test4.utxn --outfile ${TEMPDIR}/test4.stxn --keyfile ${TEMPDIR}/account_b
cat $TEMPDIR/test4.stxn | msgpacktool -d -b32 > $TEMPDIR/test4.json

I then used msgpacktool and vi to build the multisig.

# create dummy transaction from ${MSIG_BC_T_1} to construct the msig structure
${GOAL_CMD} clerk send --from ${MSIG_BC_T_1} --to ${ACCOUNT_A} --amount 0 --out ${TEMPDIR}/dummy-msig.utxn
cat $TEMPDIR/dummy-msig.utxn | msgpacktool -d -b32 > $TEMPDIR/dummy-msig.json

# copy the msig template from the dummy into the signed transaction, then move the signature to the proper signer within the msig
# vi $TEMPDIR/dummy-msig.json
# vi $TEMPDIR/test4.json

# encode the modified json with the proper msig structure and signature
cat $TEMPDIR/test4.json | msgpacktool -e -b32 > $TEMPDIR/test4.stxn

It looks like a valid signed transaction to me: goal clerk inspect $TEMPDIR/test4.stxn

${TEMPDIR}/test4.stxn[0]
{
  "msig": {
    "subsig": [
      {
        "pk": "TZIWJ3BKGLGTFDA4YAZPHIXGIEOVA5XLHWOCINUQY2PPHZ6XVFGKGMWVGA",
        "s": "XyaoJIeUfjyn8dr0jIU+mlXF7xySQhKGodBjxAlKDGCoxKXEG4TqBuy5q3VtcYpzWOiDxIaSJA4Mj+dLoI/LCg=="
      },
      {
        "pk": "5XWY6RBNYHCSY2HK5HCTO62DUJJ4PT3G4L77FQEBUKE6ZYRGQAFTLZSQQ4"
      }
    ],
    "thr": 1,
    "v": 1
  },
  "txn": {
    "amt": 100000,
    "fee": 1000,
    "fv": 132001,
    "gen": "REKEY-v1",
    "gh": "aT+oawSj/oxSWPNILM7O7EkuVE1rPxD5lLTgY+kvVCA=",
    "lv": 133001,
    "note": "OOwU2KSnKJc=",
    "rcv": "TZIWJ3BKGLGTFDA4YAZPHIXGIEOVA5XLHWOCINUQY2PPHZ6XVFGKGMWVGA",
    "snd": "WIOPRMRT4GSNFFT43B55DEQCVG67EQ2HEKPV4EULQ55YXBWMBTYUCPDUY4",
    "type": "pay"
  }
}

However, it is rejected: ${GOAL_CMD} clerk rawsend --filename ${TEMPDIR}/test4.stxn

Warning: Couldn't broadcast tx with algod: HTTP 400 Bad Request: multisig validation failed
Encountered errors in sending 1 transactions:
  PWBFKMWCZYE7JRD4QEPMX6P4W5K4U66FR6OO4TOU63F7DTTLBHRA: HTTP 400 Bad Request: multisig validation failed

ryanRfox commented 4 years ago

For completeness: algokey export --keyfile ${TEMPDIR}/account_b

Private key mnemonic: <passphrase>
Public key: TZIWJ3BKGLGTFDA4YAZPHIXGIEOVA5XLHWOCINUQY2PPHZ6XVFGKGMWVGA

brianolson commented 4 years ago

looks like rekeyed signed transactions need a 'AuthAddr' field "sgnr" in json/msgp https://github.com/algorand/go-algorand/blob/master/data/transactions/signedtxn.go#L38

ryanRfox commented 4 years ago

see: https://github.com/algorand/go-algorand/pull/1288#issuecomment-662719250

algorand / go-algorand