wormchain: Audit items (#11)

* changed minimum governance vaa length check

* remove double write of chain ID in token attest

* removed panics from vaa processing in token bridge

* added non-negative check to fee in transfer tokens

* simplify non-negative check

* make fee sign check nicer in msg_server_execute_vaa

Operationally identical

* 14->13 in proofOfAuthority.md

Co-authored-by: chase-45 <chasemoran45@gmail.com>
Co-authored-by: Csongor Kiss <ckiss@jumptrading.com>

wormhole_chain/design/proofOfAuthority.md

@@ -5,7 +5,7 @@ The Wormhole Chain is intended to operate via the same PoA mechanism as the rest
 - Two thirds of the Consensus Guardian Set are required for consensus. (In this case, block production.)
 - Guardian Sets are upgraded via processing Guardian Set Upgrade Governance VAAs.
 
-As such, the intent is that the 19 guardians will validate for Wormhole Chain, and Wormhole Chain consensus will be achieved when 14 Guardians vote to approve a block (via Tendermint). This means that we will need to hand-roll a PoA mechanism in the Cosmos-SDK on top of Tendermint and the normal Cosmos Staking module.
+As such, the intent is that the 19 guardians will validate for Wormhole Chain, and Wormhole Chain consensus will be achieved when 13 Guardians vote to approve a block (via Tendermint). This means that we will need to hand-roll a PoA mechanism in the Cosmos-SDK on top of Tendermint and the normal Cosmos Staking module.
 
 ## High-Level PoA Design Overview
 
@@ -42,14 +42,14 @@ Guardian Set upgrades are the trickiest operation to handle. When processing the
 ## Benefits of this implementation:
 
 - Adequately meets the requirement that Guardians are responsible for consensus and block production on Wormhole Chain.
-- Relatively robust with regard to chain 'bricks'. If at any point in the life of Wormhole Chain less than 14 of the Guardians in the Consensus Set are registered, the network will deadlock. There will not be enough Guardians registered to produce a block, and because no blocks are being produced, no registrations can be completed. This design does not change the Consensus Set unless a sufficient amount of Guardians are registered.
+- Relatively robust with regard to chain 'bricks'. If at any point in the life of Wormhole Chain less than 13 of the Guardians in the Consensus Set are registered, the network will deadlock. There will not be enough Guardians registered to produce a block, and because no blocks are being produced, no registrations can be completed. This design does not change the Consensus Set unless a sufficient amount of Guardians are registered.
 - Can swap out a massive set of Guardians all at once. Many other (simpler) designs for Guardian set swaps limit the number of Guardians which can be changed at once to only 6 to avoid network deadlocks. This design does not have this problem.
 - No modifications to Cosmos SDK validator bonding.
 
 ### Cons
 
 - Moderate complexity. This is more complicated than the most straightforward implementations, but gains important features and protections to prevent deadlocks.
-- Not 100% immune to deadlocks. If less than 14 Guardians have valid registrations, the chain will permanently halt. This is prohibitively difficult to prevent with on-chain mechanisms, and unlikely to occur. Performing a simple hard fork in the scenario of a maimed Guardian Validator set is likely the safer and simpler option.
+- Not 100% immune to deadlocks. If less than 13 Guardians have valid registrations, the chain will permanently halt. This is prohibitively difficult to prevent with on-chain mechanisms, and unlikely to occur. Performing a simple hard fork in the scenario of a maimed Guardian Validator set is likely the safer and simpler option.
 - Avoids some DOS scenarios by only allowing validator registrations for known Guardian Keys.
 
 ## Terms & Phrases:

wormhole_chain/x/tokenbridge/keeper/msg_server_attest_token.go

@@ -61,7 +61,6 @@ func (k msgServer) AttestToken(goCtx context.Context, msg *types.MsgAttestToken)
 	buf.Write(tokenAddress[:])
 	// TokenChain
 	MustWrite(buf, binary.BigEndian, tokenChain)
-	MustWrite(buf, binary.BigEndian, uint16(wormholeConfig.ChainId))
 	// Decimals
 	MustWrite(buf, binary.BigEndian, exponent)
 	// Symbol

wormhole_chain/x/tokenbridge/keeper/msg_server_execute_vaa.go

@@ -147,10 +147,10 @@ func (k msgServer) ExecuteVAA(goCtx context.Context, msg *types.MsgExecuteVAA) (
 
 		txSender, err := sdk.AccAddressFromBech32(msg.Creator)
 		if err != nil {
-			panic(err)
+			return nil, err
 		}
 		// Transfer fee to tx sender if it is not 0
-		if fee.Sign() != 0 {
+		if fee.Sign() == 1 {
 			err = k.bankKeeper.SendCoins(ctx, moduleAccount, txSender, sdk.Coins{
 				{
 					Denom:  identifier,
@@ -158,7 +158,7 @@ func (k msgServer) ExecuteVAA(goCtx context.Context, msg *types.MsgExecuteVAA) (
 				},
 			})
 			if err != nil {
-				panic(err)
+				return nil, err
 			}
 		}
 
@@ -172,7 +172,7 @@ func (k msgServer) ExecuteVAA(goCtx context.Context, msg *types.MsgExecuteVAA) (
 			LocalDenom:   identifier,
 		})
 		if err != nil {
-			panic(err)
+			return nil, err
 		}
 
 	case PayloadIDAssetMeta:
@@ -233,7 +233,7 @@ func (k msgServer) ExecuteVAA(goCtx context.Context, msg *types.MsgExecuteVAA) (
 			Decimals:     uint32(decimals),
 		})
 		if err != nil {
-			panic(err)
+			return nil, err
 		}
 	default:
 		return nil, types.ErrUnknownPayloadType

wormhole_chain/x/tokenbridge/keeper/msg_server_transfer.go

@@ -22,7 +22,7 @@ func (k msgServer) Transfer(goCtx context.Context, msg *types.MsgTransfer) (*typ
 
 	userAcc, err := sdk.AccAddressFromBech32(msg.Creator)
 	if err != nil {
-		panic(err)
+		return nil, err
 	}
 
 	meta, found := k.bankKeeper.GetDenomMetaData(ctx, msg.Amount.Denom)
@@ -39,8 +39,8 @@ func (k msgServer) Transfer(goCtx context.Context, msg *types.MsgTransfer) (*typ
 
 	// Parse fees
 	feeBig, ok := new(big.Int).SetString(msg.Fee, 10)
-	if !ok {
-		panic("invalid fee")
+	if !ok || feeBig.Sign() == -1 {
+		return nil, types.ErrInvalidFee
 	}
 
 	bridgeBalance := new(big.Int).Set(k.bankKeeper.GetBalance(ctx, k.accountKeeper.GetModuleAddress(types.ModuleName), msg.Amount.Denom).Amount.BigInt())

wormhole_chain/x/wormhole/keeper/msg_server_execute_governance_vaa.go

@@ -70,7 +70,7 @@ func (k msgServer) ExecuteGovernanceVAA(goCtx context.Context, msg *types.MsgExe
 	// Execute action
 	switch action {
 	case ActionGuardianSetUpdate:
-		if len(payload) < 3 {
+		if len(payload) < 5 {
 			return nil, types.ErrInvalidGovernancePayloadLength
 		}
 		// Update guardian set