wallet: make SignTx more precise and accurate

* each account must have an appropriate signer, if there is no signer for
   this account in the tx it's an error
 * we can only safely append to Scripts when account belongs to the next
   signer (we don't have appropriate verification scripts for other signers)
 * when contract has one parameter, the signature shouldn't be appended to
   other data

I think these rules allow to handle more cases and do that safer. We have more
complex scenarios though, like non-signature parameters or mixed-parameter
invocation scripts, but that's out of scope for now.

pkg/wallet/account.go

@@ -84,13 +84,38 @@ func NewAccount() (*Account, error) {
 
 // SignTx signs transaction t and updates it's Witnesses.
 func (a *Account) SignTx(net netmode.Magic, t *transaction.Transaction) error {
+	var (
+		haveAcc bool
+		pos     int
+		accHash util.Uint160
+		err     error
+	)
 	if a.Contract == nil {
 		return errors.New("account has no contract")
 	}
-	if len(a.Contract.Parameters) == 0 {
+	accHash, err = address.StringToUint160(a.Address)
-		if len(t.Signers) != len(t.Scripts) { // Sequential signing vs. existing scripts.
+	if err != nil {
-			t.Scripts = append(t.Scripts, transaction.Witness{})
+		return err
+	}
+	for i := range t.Signers {
+		if t.Signers[i].Account.Equals(accHash) {
+			haveAcc = true
+			pos = i
+			break
 		}
+	}
+	if !haveAcc {
+		return errors.New("transaction is not signed by this account")
+	}
+	if len(t.Scripts) < pos {
+		return errors.New("transaction is not yet signed by the previous signer")
+	}
+	if len(t.Scripts) == pos {
+		t.Scripts = append(t.Scripts, transaction.Witness{
+			VerificationScript: a.Contract.Script, // Can be nil for deployed contract.
+		})
+	}
+	if len(a.Contract.Parameters) == 0 {
 		return nil
 	}
 	if a.privateKey == nil {
@@ -98,18 +123,12 @@ func (a *Account) SignTx(net netmode.Magic, t *transaction.Transaction) error {
 	}
 	sign := a.privateKey.SignHashable(uint32(net), t)
 
-	verif := a.GetVerificationScript()
 	invoc := append([]byte{byte(opcode.PUSHDATA1), 64}, sign...)
-	for i := range t.Scripts {
+	if len(a.Contract.Parameters) == 1 {
-		if bytes.Equal(t.Scripts[i].VerificationScript, verif) {
+		t.Scripts[pos].InvocationScript = invoc
-			t.Scripts[i].InvocationScript = append(t.Scripts[i].InvocationScript, invoc...)
+	} else {
-			return nil
+		t.Scripts[pos].InvocationScript = append(t.Scripts[pos].InvocationScript, invoc...)
-		}
 	}
-	t.Scripts = append(t.Scripts, transaction.Witness{
-		InvocationScript:   invoc,
-		VerificationScript: verif,
-	})
 
 	return nil
 }

pkg/wallet/account_test.go

@@ -6,8 +6,10 @@ import (
 	"testing"
 
 	"github.com/nspcc-dev/neo-go/internal/keytestcases"
+	"github.com/nspcc-dev/neo-go/pkg/core/transaction"
 	"github.com/nspcc-dev/neo-go/pkg/crypto/hash"
 	"github.com/nspcc-dev/neo-go/pkg/crypto/keys"
+	"github.com/nspcc-dev/neo-go/pkg/smartcontract"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -81,6 +83,75 @@ func TestContract_MarshalJSON(t *testing.T) {
 	require.Error(t, json.Unmarshal(data, &c))
 }
 
+func TestContractSignTx(t *testing.T) {
+	acc, err := NewAccount()
+	require.NoError(t, err)
+
+	accNoContr := *acc
+	accNoContr.Contract = nil
+	tx := &transaction.Transaction{
+		Script: []byte{1, 2, 3},
+		Signers: []transaction.Signer{{
+			Account: acc.Contract.ScriptHash(),
+			Scopes:  transaction.CalledByEntry,
+		}},
+	}
+	require.Error(t, accNoContr.SignTx(0, tx))
+
+	acc2, err := NewAccount()
+	require.NoError(t, err)
+
+	require.Error(t, acc2.SignTx(0, tx))
+
+	pubs := keys.PublicKeys{acc.privateKey.PublicKey(), acc2.privateKey.PublicKey()}
+	multiS, err := smartcontract.CreateDefaultMultiSigRedeemScript(pubs)
+	require.NoError(t, err)
+	multiAcc := NewAccountFromPrivateKey(acc.privateKey)
+	require.NoError(t, multiAcc.ConvertMultisig(2, pubs))
+	multiAcc2 := NewAccountFromPrivateKey(acc2.privateKey)
+	require.NoError(t, multiAcc2.ConvertMultisig(2, pubs))
+
+	tx = &transaction.Transaction{
+		Script: []byte{1, 2, 3},
+		Signers: []transaction.Signer{{
+			Account: acc2.Contract.ScriptHash(),
+			Scopes:  transaction.CalledByEntry,
+		}, {
+			Account: acc.Contract.ScriptHash(),
+			Scopes:  transaction.None,
+		}, {
+			Account: hash.Hash160(multiS),
+			Scopes:  transaction.None,
+		}},
+	}
+	require.Error(t, acc.SignTx(0, tx)) // Can't append, no witness for acc2.
+
+	require.NoError(t, acc2.SignTx(0, tx)) // Append script for acc2.
+	require.Equal(t, 1, len(tx.Scripts))
+	require.Equal(t, 66, len(tx.Scripts[0].InvocationScript))
+
+	require.NoError(t, acc2.SignTx(0, tx)) // Sign again, effectively a no-op.
+	require.Equal(t, 1, len(tx.Scripts))
+	require.Equal(t, 66, len(tx.Scripts[0].InvocationScript))
+
+	acc2.privateKey = nil
+	require.Error(t, acc2.SignTx(0, tx)) // No private key.
+
+	tx.Scripts = append(tx.Scripts, transaction.Witness{
+		VerificationScript: acc.Contract.Script,
+	})
+	require.NoError(t, acc.SignTx(0, tx)) // Add invocation script for existing witness.
+	require.Equal(t, 66, len(tx.Scripts[1].InvocationScript))
+
+	require.NoError(t, multiAcc.SignTx(0, tx))
+	require.Equal(t, 3, len(tx.Scripts))
+	require.Equal(t, 66, len(tx.Scripts[2].InvocationScript))
+
+	require.NoError(t, multiAcc2.SignTx(0, tx)) // Append to existing script.
+	require.Equal(t, 3, len(tx.Scripts))
+	require.Equal(t, 132, len(tx.Scripts[2].InvocationScript))
+}
+
 func TestContract_ScriptHash(t *testing.T) {
 	script := []byte{0, 1, 2, 3}
 	c := &Contract{Script: script}