[PVM] Add syntactic check to Lock to/change args (#397)

chain4travel · Nov 19, 2024 · e94d848 · e94d848
1 parent da9e010
commit e94d848
Show file tree

Hide file tree

Showing 2 changed files with 74 additions and 45 deletions.
diff --git a/vms/platformvm/utxo/camino_locked.go b/vms/platformvm/utxo/camino_locked.go
@@ -26,28 +26,29 @@ import (
 )
 
 var (
-	errInvalidTargetLockState       = errors.New("invalid target lock state")
-	errLockingLockedUTXO            = errors.New("utxo consumed for locking are already locked")
-	errUnlockingUnlockedUTXO        = errors.New("utxo consumed for unlocking are already unlocked")
-	errInsufficientBalance          = errors.New("insufficient balance")
-	errWrongInType                  = errors.New("wrong input type")
-	errWrongOutType                 = errors.New("wrong output type")
-	errWrongUTXOOutType             = errors.New("wrong utxo output type")
-	errWrongProducedAmount          = errors.New("produced more tokens, than input had")
-	errInputsCredentialsMismatch    = errors.New("number of inputs is different from number of credentials")
-	errInputsUTXOsMismatch          = errors.New("number of inputs is different from number of utxos")
-	errBadCredentials               = errors.New("bad credentials")
-	errNotBurnedEnough              = errors.New("burned less tokens, than needed to")
-	errAssetIDMismatch              = errors.New("utxo/input/output assetID is different from expected asset id")
-	errLockIDsMismatch              = errors.New("input lock ids is different from utxo lock ids")
-	errFailToGetDeposit             = errors.New("couldn't get deposit")
-	errLockedUTXO                   = errors.New("can't spend locked utxo")
-	errNotLockedUTXO                = errors.New("can't spend unlocked utxo")
-	errUTXOOutTypeOrAmtMismatch     = errors.New("inner out isn't *secp256k1fx.TransferOutput or inner out amount != input.Amt")
-	errCantSpend                    = errors.New("can't spend utxo with given credential and input")
-	errCompositeMultisigChangeOwner = errors.New("change owner can't be nested multisig owner")
-	errCompositeMultisigToOwner     = errors.New("to-owner can't be nested multisig owner")
-	errNewBondOwner                 = errors.New("can't create bond for new owner")
+	errInvalidTargetLockState    = errors.New("invalid target lock state")
+	errLockingLockedUTXO         = errors.New("utxo consumed for locking are already locked")
+	errUnlockingUnlockedUTXO     = errors.New("utxo consumed for unlocking are already unlocked")
+	errInsufficientBalance       = errors.New("insufficient balance")
+	errWrongInType               = errors.New("wrong input type")
+	errWrongOutType              = errors.New("wrong output type")
+	errWrongUTXOOutType          = errors.New("wrong utxo output type")
+	errWrongProducedAmount       = errors.New("produced more tokens, than input had")
+	errInputsCredentialsMismatch = errors.New("number of inputs is different from number of credentials")
+	errInputsUTXOsMismatch       = errors.New("number of inputs is different from number of utxos")
+	errBadCredentials            = errors.New("bad credentials")
+	errNotBurnedEnough           = errors.New("burned less tokens, than needed to")
+	errAssetIDMismatch           = errors.New("utxo/input/output assetID is different from expected asset id")
+	errLockIDsMismatch           = errors.New("input lock ids is different from utxo lock ids")
+	errFailToGetDeposit          = errors.New("couldn't get deposit")
+	errLockedUTXO                = errors.New("can't spend locked utxo")
+	errNotLockedUTXO             = errors.New("can't spend unlocked utxo")
+	errUTXOOutTypeOrAmtMismatch  = errors.New("inner out isn't *secp256k1fx.TransferOutput or inner out amount != input.Amt")
+	errCantSpend                 = errors.New("can't spend utxo with given credential and input")
+	errCompositeMultisig         = errors.New("owner can't be nested multisig owner")
+	errInvalidToOwner            = errors.New("invalid to-owner")
+	errInvalidChangeOwner        = errors.New("invalid change owner")
+	errNewBondOwner              = errors.New("can't create bond for new owner")
 )
 
 // Creates UTXOs from [outs] and adds them to the UTXO set.
@@ -245,13 +246,17 @@ func (h *handler) Lock(
 		id         *ids.ID
 	}
 
-	setOwner := func(secpOwner *secp256k1fx.OutputOwners, owner *Owner, errNested error) error {
+	setOwner := func(secpOwner *secp256k1fx.OutputOwners, owner *Owner) error {
 		if secpOwner == nil {
 			return nil
 		}
 
+		if err := secpOwner.Verify(); err != nil {
+			return fmt.Errorf("invalid owner: %w", err)
+		}
+
 		if err := h.fx.VerifyMultisigOwner(secpOwner, utxoDB); err != nil {
-			return fmt.Errorf("%w: %v", errNested, err)
+			return fmt.Errorf("%w: %v", errCompositeMultisig, err)
 		}
 
 		id, err := txs.GetOwnerID(secpOwner)
@@ -263,13 +268,13 @@ func (h *handler) Lock(
 	}
 
 	newOwner := Owner{}
-	if err := setOwner(to, &newOwner, errCompositeMultisigToOwner); err != nil {
-		return nil, nil, nil, nil, fmt.Errorf("failed to set to-owner: %w", err)
+	if err := setOwner(to, &newOwner); err != nil {
+		return nil, nil, nil, nil, fmt.Errorf("%w: %s", errInvalidToOwner, err)
 	}
 
 	changeOwner := Owner{}
-	if err := setOwner(change, &changeOwner, errCompositeMultisigChangeOwner); err != nil {
-		return nil, nil, nil, nil, fmt.Errorf("failed to set change owner: %w", err)
+	if err := setOwner(change, &changeOwner); err != nil {
+		return nil, nil, nil, nil, fmt.Errorf("%w: %s", errInvalidChangeOwner, err)
 	}
 
 	addrs, signer := secp256k1fx.ExtractFromAndSigners(keys)

diff --git a/vms/platformvm/utxo/camino_locked_test.go b/vms/platformvm/utxo/camino_locked_test.go
@@ -205,6 +205,18 @@ func TestLock(t *testing.T) {
 		return state
 	}
 
+	noOpState := func(
+		t *testing.T,
+		ctrl *gomock.Controller,
+		utxos []*avax.UTXO,
+		keys []*secp256k1.PrivateKey,
+		to *secp256k1fx.OutputOwners,
+		change *secp256k1fx.OutputOwners,
+	) *state.MockState {
+		t.Helper()
+		return state.NewMockState(ctrl)
+	}
+
 	tests := map[string]struct {
 		state func(
 			t *testing.T,
@@ -229,21 +241,27 @@ func TestLock(t *testing.T) {
 		expectedErr        error
 	}{
 		"New bond owner": {
-			state: func(
-				t *testing.T,
-				ctrl *gomock.Controller,
-				_ []*avax.UTXO,
-				_ []*secp256k1.PrivateKey,
-				_ *secp256k1fx.OutputOwners,
-				_ *secp256k1fx.OutputOwners,
-			) *state.MockState {
-				t.Helper()
-				return state.NewMockState(ctrl)
-			},
+			state:            noOpState,
 			appliedLockState: locked.StateBonded,
 			to:               &recipientOwner,
 			expectedErr:      errNewBondOwner,
 		},
+		"Syntactically invalid change owner": {
+			state: noOpState,
+			change: &secp256k1fx.OutputOwners{
+				Addrs:     []ids.ShortID{{100}},
+				Threshold: 2,
+			},
+			expectedErr: errInvalidChangeOwner,
+		},
+		"Syntactically invalid to-owner": {
+			state: noOpState,
+			to: &secp256k1fx.OutputOwners{
+				Addrs:     []ids.ShortID{{100}},
+				Threshold: 2,
+			},
+			expectedErr: errInvalidToOwner,
+		},
 		"Nested multisig change owner": {
 			state: func(
 				t *testing.T,
@@ -269,7 +287,7 @@ func TestLock(t *testing.T) {
 				return state
 			},
 			change:      &changeOwner,
-			expectedErr: errCompositeMultisigChangeOwner,
+			expectedErr: errInvalidChangeOwner,
 		},
 		"Composite multisig change owner": {
 			state: func(
@@ -290,8 +308,11 @@ func TestLock(t *testing.T) {
 				)
 				return state
 			},
-			change:      &secp256k1fx.OutputOwners{Addrs: []ids.ShortID{{100}, {101}}},
-			expectedErr: errCompositeMultisigChangeOwner,
+			change: &secp256k1fx.OutputOwners{
+				Addrs:     []ids.ShortID{{100}, {101}},
+				Threshold: 1,
+			},
+			expectedErr: errInvalidChangeOwner,
 		},
 		"Nested multisig to-owner": {
 			state: func(
@@ -318,7 +339,7 @@ func TestLock(t *testing.T) {
 				return state
 			},
 			to:          &recipientOwner,
-			expectedErr: errCompositeMultisigToOwner,
+			expectedErr: errInvalidToOwner,
 		},
 		"Composite multisig to-owner": {
 			state: func(
@@ -339,8 +360,11 @@ func TestLock(t *testing.T) {
 				)
 				return state
 			},
-			to:          &secp256k1fx.OutputOwners{Addrs: []ids.ShortID{{100}, {101}}},
-			expectedErr: errCompositeMultisigToOwner,
+			to: &secp256k1fx.OutputOwners{
+				Addrs:     []ids.ShortID{{100}, {101}},
+				Threshold: 1,
+			},
+			expectedErr: errInvalidToOwner,
 		},
 		"Bond unlocked utxo: not enough balance": {
 			state:              defaultState,